Date: Thu, 11 Dec 2008 13:54:38 +0100 From: Alexander Leidinger <Alexander@Leidinger.net> To: Marius =?utf-8?b?TsO8bm5lcmljaA==?= <marius@nuenneri.ch> Cc: jb@freebsd.org, FreeBSD Current <freebsd-current@freebsd.org>, phk@freebsd.org, freebsd-geom@freebsd.org Subject: Re: DTrace probes for geom_kern, geom_io and geom_event Message-ID: <20081211135438.52433nmj45ia112c@webmail.leidinger.net> In-Reply-To: <b649e5e0812101415x3ca00e03pe2be350141596a72@mail.gmail.com> References: <b649e5e0812041241y143254e0k5e1bae385fc9ae2b@mail.gmail.com> <b649e5e0812101415x3ca00e03pe2be350141596a72@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Quoting Marius N=C3=BCnnerich <marius@nuenneri.ch> (from Wed, 10 Dec 2008 = =20 23:15:43 +0100): > After some tips from Alexander Leidinger I updated the patch, new =20 > version here: > http://nuenneri.ch/freebsd/geom_probes2.patch Again: I just reviewed the patch, so I don't have the complete context =20 of the functions, just what I see in the patch (-> high level dtrace =20 review, not geom specific probe review). Still inconsistent locking probes. Lock is fired without the lock =20 held, unlock is fired with the lock held. Both should IMHO be fired =20 either with the lock held or without the lock held, but not with the =20 current mix. g_new_bio/g_io_schedule_up: the return probe has the name "entry" in =20 your patch. A msleep probe could give some more info (sometimes there are even =20 zero arguments, but there are 3-5 things which could be interesting to =20 know). Similar for tsleep (the time should be provided in the probe =20 arguments too). I don't think we need "loop" probes. Given that g_trace is a debugging function and that dtrace is =20 superior, I don't think you need to instrument g_trace with dtrace =20 probes. g_topology_lock/unlock should provide the lock in the probe arguments. =20 Again, see above, either both probes firing with the lock, or without =20 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 =20 be found in a lot of unrelated places, but "g_bioq_init" only in =20 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. =20 Dtrace is not expensive, having a lot of probes does not hurt (except =20 maybe in a critical path). You could fire an read_not_permitted probe, =20 or a write_not_permitted probe or whatever. This can be done =20 additionally to the return probe. This way it's very easy to see if =20 there's a permission problem, without the need to write errno checks =20 in dtrace. If you have a lot of returns but only a handful of =20 permission errors, it's better to have some specific probes which can =20 be fired. Keep in mind dtrace is designed to be used to debug problems =20 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 =20 people/sysadmins without much GEOM knowledge don't have a problem to =20 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 =20 can tell to give everything for geom, while with with the fbt =20 dtrace-provider you need to know the naming conventions in the kernel. =20 So while you have in fbt the possibility to get access to the probes, =20 the sysadmin which does not know much about GEOM can get a meaningful =20 overview in case entry and return probes available in the geom =20 dtrace-provider. A lot of places in the kernel do not have a naming =20 convention like GEOM, so when we handle them (e.g. the linuxulator), =20 we need to add entry/return probes so that sysadmins without knowledge =20 about kernel internals can search for solutions of their problems. We =20 should be consistent in our probe naming, else it's not easy to use =20 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 =20 stay synchronized with vendor code. Another one is complexity to =20 handle this (currently the fbt part is automatic, I don't see a way to =20 handle related stuff which is spread into several places to within the =20 same namespace without introducing hints into different places which =20 tells what belongs where). HTH, Alexander. --=20 "They shot him five times. But he's though." =09=09-- Santino Corleone, "Chapter 2", page 79 http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7 http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081211135438.52433nmj45ia112c>