Re: DTrace probes for geom_kern, geom_io and geom_event

Quoting Marius Nünnerich <marius_at_nuenneri.ch> (from Wed, 10 Dec 2008  
23:15:43 +0100):

> After some tips from Alexander Leidinger I updated the patch, new  
> version here:
> http://nuenneri.ch/freebsd/geom_probes2.patch

Again: I just reviewed the patch, so I don't have the complete context  
of the functions, just what I see in the patch (-> high level dtrace  
review, not geom specific probe review).

Still inconsistent locking probes. Lock is fired without the lock  
held, unlock is fired with the lock held. Both should IMHO be fired  
either with the lock held or without the lock held, but not with the  
current mix.

g_new_bio/g_io_schedule_up: the return probe has the name "entry" in  
your patch.

A msleep probe could give some more info (sometimes there are even  
zero arguments, but there are 3-5 things which could be interesting to  
know). Similar for tsleep (the time should be provided in the probe  
arguments too).

I don't think we need "loop" probes.

Given that g_trace is a debugging function and that dtrace is  
superior, I don't think you need to instrument g_trace with dtrace  
probes.

g_topology_lock/unlock should provide the lock in the probe arguments.  
Again, see above, either both probes firing with the lock, or without  
the lock, but not mixed as it is.

> There are some questions I'd like to discuss:

> 2. Should I use the full function name for the probes (with the g_
> prefix) even though it's defined under the provider geom

IMHO yes, it's more easy for the person grepping around, as "bioq" can  
be found in a lot of unrelated places, but "g_bioq_init" only in  
places where you want to know about.

> 3. Should there be a probe for every switch case in g_io_check? I
> think this won't work with the fall-through that is used right now

IMHO at least in every code block which is doing something sensible.  
Dtrace is not expensive, having a lot of probes does not hurt (except  
maybe in a critical path). You could fire an read_not_permitted probe,  
or a write_not_permitted probe or whatever. This can be done  
additionally to the return probe. This way it's very easy to see if  
there's a permission problem, without the need to write errno checks  
in dtrace. If you have a lot of returns but only a handful of  
permission errors, it's better to have some specific probes which can  
be fired. Keep in mind dtrace is designed to be used to debug problems  
on production systems.

> 4. Alexander proposed to change the module name kern to core. I'm not
> sure about this as kern refers to the filename, like io and event do

- core for kern
- core_io for io (maybe)
- core_event for event (maybe)

This way you can use gmirror, graid3, ... later as module names and  
people/sysadmins without much GEOM knowledge don't have a problem to  
see distinguish with real GEOM core stuff and stuff in GEOM providers.

> 7. What about g_bioq_(un)lock functions, I just added one probe for
> it, I do not really see a point in adding entry and return probes
> (they are there with FBT anyway). This is consistent with
> g_topology_lock etc. What about making macros of the two functions
> like g_topology_lock

Regarding FBT: the advantage of the geom dtrace-provider is that you  
can tell to give everything for geom, while with with the fbt  
dtrace-provider you need to know the naming conventions in the kernel.  
So while you have in fbt the possibility to get access to the probes,  
the sysadmin which does not know much about GEOM can get a meaningful  
overview in case entry and return probes available in the geom  
dtrace-provider. A lot of places in the kernel do not have a naming  
convention like GEOM, so when we handle them (e.g. the linuxulator),  
we need to add entry/return probes so that sysadmins without knowledge  
about kernel internals can search for solutions of their problems. We  
should be consistent in our probe naming, else it's not easy to use  
dtrace.

> 9. Writing hundreds of entry and return probes is boring, especially
> as there is the FBT provider. Maybe it's possible to give the FBT
> probes better names like geom:io:g_io_schedule_down:entry instead of
> fbt:kernel:g_io_schedule:entry. Every FBT probe has a provider of fbt
> und module of kernel right now. One also has to define the argument
> types which I think FBT figures out by itself. If this would work we
> could concentrate on adding SDT probes to interesting places inside of
> functions or macros

Both ways have good and bad parts. One argument against this is to  
stay synchronized with vendor code. Another one is complexity to  
handle this (currently the fbt part is automatic, I don't see a way to  
handle related stuff which is spread into several places to within the  
same namespace without introducing hints into different places which  
tells what belongs where).

HTH,
Alexander.

-- 
"They shot him five times. But he's though."
		-- Santino Corleone, "Chapter 2", page 79

http://www.Leidinger.net    Alexander _at_ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild _at_ FreeBSD.org  : PGP ID = 72077137