"unstable" sdt probes

[Andriy Gapon]

on 31/10/2013 05:55 Mark Johnston said the following:
> On Tue, Oct 29, 2013 at 11:20:51PM +0200, Andriy Gapon wrote:
>> on 26/10/2013 21:06 Mark Johnston said the following:
>>> The patch here is what I had in mind:
>>> http://people.freebsd.org/~markj/patches/zfs_probes.diff
>>>
>>> I've only compile-tested it, but it should create illumos-compatible ZFS
>>> probes without changing any ZFS code, assuming I understand exactly how
>>> they're creating/naming probes. :)
>>
>> The simplicity and straightforwardness of your patch is very seducing! :-)
>> I think that you missed sys/cddl/compat/opensolaris/sys/sdt.h, but that's a very
>> a minor issue that is trivial to fix.
> 
> Oops. I didn't even know that file existed. :)
> 
>>
>> I had something a little bit more elaborate on my mind.  Something that would
>> allow DTRACE_PROBE macros to work without providing any additional code per each
>> probe.  And also something that would make DTRACE_PROBE macros appealing to use
>> in the FreeBSD code proper.
> 
> Those macros don't address the problem of setting argument types though.

I think nothing should prevent just explicitly using SDT_PROBE_ARGTYPE for
probes defined via DTRACE_PROBE.  If we want the types, that is.

> We could do what illumos does, i.e. have a single giant table in
> sdt_subr.c, but IMHO the SDT macros are more flexible. They let you
> create and modify probes in a kernel module without recompiling the
> kernel and rebooting, which something I've found handy in the past.

Additionally, I think that what SDT_PROBE_ARGTYPE does can also be done directly
in DTRACE_PROBE, because type information is provided to it.

> For the purpose of creating unstable providers or quick debugging, I
> guess it's ok to not define argument types, so I don't object to the
> DTRACE_PROBE interface itself, but it'd be nice to avoid having both
> SDT_* calls and DTRACE_* calls all over the tree. That'd be confusing.

I agree.. The current status quo would be SDT_* in the FreeBSD proper code and
DTRACE_* in the code with Solaris/illumos origins.

>> So, I got some time to hack on this and here is a result:
>> http://people.freebsd.org/~avg/dtrace-probe-macros.diff
>> This change depends upon another change that I've just posted.
> 
> Huh, for some reason I thought that DATA_SET() didn't work properly in a
> function body. I remember trying something similar when I changed sdt to
> use linker sets, and giving up because of some related problems, but I
> don't remember exactly why. If it works, then I guess it's ok

Seems to work just fine.
DATA_SET just places an address of a global or static object into a linker set
as far as I can see.

> so long as
> the sdt code handles duplicate entries in the probe set. :)

Where would those comes from?
Do we expect that there could be more than one SDT probe calls with the same
name?  But even if yes, I think that duplicates should be fine as long as they
are exact duplicates.

>> So, no surprise that I feel preference for my change, but I think that your
>> change has certain advantage as well (esp. brevity and clarity).
>>
>> What do you think?
> 
> The patch looks ok to me, but I can see that it won't apply to head/ - I
> think it'll need at least r257152 and r254468.

Yeah, I need to update my tree and rebase the patch.
Thank you for the review!

-- 
Andriy Gapon