Date: Thu, 30 May 2013 09:52:43 -0400 From: Mark Johnston <markj@freebsd.org> To: Brendan Gregg <brendan.gregg@joyent.com> Cc: rstone@freebsd.org, freebsd-dtrace@freebsd.org Subject: Re: adding SDT probe functions with 6+ DTrace arguments Message-ID: <20130530135243.GA1406@gloom.sandvine.com> In-Reply-To: <CA%2BXzFFjOE7y4QueSuqw8zcWMWk7pVyGKifZwgbZcANXYpKrVyA@mail.gmail.com> References: <20130530040705.GA40320@gloom> <CALWP=0aRG5zjcNBJsgA0gwp9x8J_gxEtagfVy3JfK2ePC0J0jA@mail.gmail.com> <CA%2BXzFFjOE7y4QueSuqw8zcWMWk7pVyGKifZwgbZcANXYpKrVyA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, May 29, 2013 at 11:14:32PM -0700, Brendan Gregg wrote:
> G'Day Mark,
>
> As this might be interesting: to check that DTRACE_PROBE7 worked, in
> Solaris I had added an sdt:::test probe, which could be invoked by the
> DTrace test suite (sdt/tst.sdtargs.c) by calling uadmin() with A_SDTTEST:
>
> case A_SDTTEST:
> {
> DTRACE_PROBE7(test, int, 1, int, 2, int, 3, int, 4, int, 5,
> int, 6, int, 7);
> break;
> }
Yep, this is the test case that I mentioned below. I had to do a bit of
work to port it - uadmin doesn't exist on FreeBSD (and before yesterday,
I had never heard of it :).
I fixed the test in the patch below:
http://people.freebsd.org/~markj/patches/sdt-many-args.diff
As mentioned below, there are a couple of off-by-one bugs in the way
dtrace_getarg() gets extra arguments off the stack. One of them is
FreeBSD-specific (our SDT implementation doesn't set the aframes
parameter properly), and the other is an amd64-specific bug which I
think also exists in illumos; this is fixed in the change to
dtrace_isa.c in the above patch.
Here's a little diagram of how the local variables are set up at the end
of dtrace_getarg() when fetching the sixth argument of an SDT probe on
FreeBSD:
...
---------------
| arg 1 |
--------------- <----- stack
| arg 0 | stack[-1]
---------------
| ret addr |
---------------
| frame ptr |
--------------- <----- fp
This is the stack frame of dtrace_probe(). To get at arg 0 - which is
the sixth argument of a DTrace probe, the local variable 'arg' should be
-1; in the current code, it ends up being 0. Specifically, in this case
dtrace_getarg() is called with arg == 5. Then we increment it once a bit
later, and then we put
arg -= (inreg + 1);
so arg ends up being 0 when it should be -1. Then the seventh argument
is at stack[0], so 'arg' should be 0 but is 1, and so on.
I think this bug comes from looking at the i386 code; the difference
there is that all arguments to the probe are on the stack, and the first
argument is the probe ID. So we _want_ to skip it. Then 'stack' is
basically a pointer to the array of probe arguments.
-Mark
>
> Getting the tcp and ip providers working will be great.
>
> Brendan
>
> On Wed, May 29, 2013 at 11:02 PM, Adam Leventhal <ahl@delphix.com> wrote:
>
> > Hey Mark,
> >
> > I'm just now familiarizing myself with the way that FreeBSD DTrace
> > handles SDT probes, but certainly having more arguments (as needed)
> > makes sense, and the approach you've taken seems in keeping with the
> > existing methodology.
> >
> > Adam
> >
> > On Wed, May 29, 2013 at 9:07 PM, Mark Johnston <markj@freebsd.org> wrote:
> > > Hello,
> > >
> > > Does anyone have objections or comments on the patch below? It simply
> > > makes it possible to use the SDT_* macros to implement DTrace probes
> > > with more than 5 arguments. dtrace_probe() only takes 5 arguments, so
> > > some ugly casting is needed, but it works properly in my testing on
> > > amd64. In fact, there are a couple of bugs in the way that DTrace
> > > fetches the 6th argument (or the 7th, and so on) from the probe site,
> > > but I'll be fixing that soon as well, along with the test case that's
> > > supposed to detect this problem in the first place.
> > >
> > > I don't know of any existing SDT providers in FreeBSD that would use
> > > these macros, but the ip, iscsi and tcp providers will - hence my
> > > interest in making sure that this functionality works properly.
> > >
> > > Thanks!
> > > -Mark
> > >
> > > diff --git a/sys/sys/sdt.h b/sys/sys/sdt.h
> > > index 522e1f2..21edd53 100644
> > > --- a/sys/sys/sdt.h
> > > +++ b/sys/sys/sdt.h
> > > @@ -91,6 +91,10 @@
> > > #define SDT_PROBE_DEFINE3(prov, mod, func, name, sname, arg0,
> > arg1, arg2)
> > > #define SDT_PROBE_DEFINE4(prov, mod, func, name, sname, arg0,
> > arg1, arg2, arg3)
> > > #define SDT_PROBE_DEFINE5(prov, mod, func, name, sname, arg0,
> > arg1, arg2, arg3, arg4)
> > > +#define SDT_PROBE_DEFINE6(prov, mod, func, name, snamp, arg0,
> > arg1, arg2, \
> > > + arg3, arg4, arg5)
> > > +#define SDT_PROBE_DEFINE7(prov, mod, func, name, snamp, arg0,
> > arg1, arg2, \
> > > + arg3, arg4, arg5, arg6)
> > >
> > > #define SDT_PROBE0(prov, mod, func, name)
> > > #define SDT_PROBE1(prov, mod, func, name, arg0)
> > > @@ -98,6 +102,9 @@
> > > #define SDT_PROBE3(prov, mod, func, name, arg0, arg1, arg2)
> > > #define SDT_PROBE4(prov, mod, func, name, arg0, arg1, arg2, arg3)
> > > #define SDT_PROBE5(prov, mod, func, name, arg0, arg1, arg2,
> > arg3, arg4)
> > > +#define SDT_PROBE6(prov, mod, func, name, arg0, arg1, arg2,
> > arg3, arg4, arg5)
> > > +#define SDT_PROBE7(prov, mod, func, name, arg0, arg1, arg2,
> > arg3, arg4, arg5, \
> > > + arg6)
> > >
> > > #else
> > >
> > > @@ -233,6 +240,27 @@ struct sdt_provider {
> > > SDT_PROBE_ARGTYPE(prov, mod, func, name, 3, arg3); \
> > > SDT_PROBE_ARGTYPE(prov, mod, func, name, 4, arg4)
> > >
> > > +#define SDT_PROBE_DEFINE6(prov, mod, func, name, sname, arg0,
> > arg1, arg2, arg3,\
> > > + arg4, arg5) \
> > > + SDT_PROBE_DEFINE(prov, mod, func, name, sname); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 0, arg0); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 1, arg1); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 2, arg2); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 3, arg3); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 4, arg4); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 5, arg5);
> > > +
> > > +#define SDT_PROBE_DEFINE7(prov, mod, func, name, sname, arg0,
> > arg1, arg2, arg3,\
> > > + arg4, arg5, arg6) \
> > > + SDT_PROBE_DEFINE(prov, mod, func, name, sname); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 0, arg0); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 1, arg1); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 2, arg2); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 3, arg3); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 4, arg4); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 5, arg5); \
> > > + SDT_PROBE_ARGTYPE(prov, mod, func, name, 6, arg6);
> > > +
> > > #define SDT_PROBE0(prov, mod, func, name)
> > \
> > > SDT_PROBE(prov, mod, func, name, 0, 0, 0, 0, 0)
> > > #define SDT_PROBE1(prov, mod, func, name, arg0)
> > \
> > > @@ -245,6 +273,27 @@ struct sdt_provider {
> > > SDT_PROBE(prov, mod, func, name, arg0, arg1, arg2, arg3, 0)
> > > #define SDT_PROBE5(prov, mod, func, name, arg0, arg1, arg2,
> > arg3, arg4) \
> > > SDT_PROBE(prov, mod, func, name, arg0, arg1, arg2, arg3, arg4)
> > > +#define SDT_PROBE6(prov, mod, func, name, arg0, arg1, arg2,
> > arg3, arg4, arg5) \
> > > + do {
> > \
> > > + if (sdt_##prov##_##mod##_##func##_##name->id)
> > \
> > > + (*(void (*)(uint32_t, uintptr_t, uintptr_t,
> > uintptr_t, \
> > > + uintptr_t, uintptr_t,
> > uintptr_t))sdt_probe_func)( \
> > > + sdt_##prov##_##mod##_##func##_##name->id,
> > \
> > > + (uintptr_t)arg0, (uintptr_t)arg1,
> > (uintptr_t)arg2, \
> > > + (uintptr_t)arg3, (uintptr_t)arg4,
> > (uintptr_t)arg5);\
> > > + } while (0)
> > > +#define SDT_PROBE7(prov, mod, func, name, arg0, arg1, arg2,
> > arg3, arg4, arg5, \
> > > + arg6)
> > \
> > > + do {
> > \
> > > + if (sdt_##prov##_##mod##_##func##_##name->id)
> > \
> > > + (*(void (*)(uint32_t, uintptr_t, uintptr_t,
> > uintptr_t, \
> > > + uintptr_t, uintptr_t, uintptr_t, uintptr_t))
> > \
> > > + sdt_probe_func)(
> > \
> > > + sdt_##prov##_##mod##_##func##_##name->id,
> > \
> > > + (uintptr_t)arg0, (uintptr_t)arg1,
> > (uintptr_t)arg2, \
> > > + (uintptr_t)arg3, (uintptr_t)arg4,
> > (uintptr_t)arg5, \
> > > + (uintptr_t)arg6);
> > \
> > > + } while (0)
> > >
> > > typedef int (*sdt_argtype_listall_func_t)(struct sdt_argtype *, void *);
> > > typedef int (*sdt_probe_listall_func_t)(struct sdt_probe *, void *);
> > > _______________________________________________
> > > freebsd-dtrace@freebsd.org mailing list
> > > https://lists.freebsd.org/mailman/listinfo/freebsd-dtrace
> > > To unsubscribe, send any mail to "freebsd-dtrace-unsubscribe@freebsd.org
> > "
> >
> >
> >
> > --
> > Adam Leventhal
> > CTO, Delphix
> > http://blog.delphix.com/ahl
> > _______________________________________________
> > freebsd-dtrace@freebsd.org mailing list
> > https://lists.freebsd.org/mailman/listinfo/freebsd-dtrace
> > To unsubscribe, send any mail to "freebsd-dtrace-unsubscribe@freebsd.org"
> >
>
>
>
> --
> Brendan Gregg, Joyent http://dtrace.org/blogs/brendan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130530135243.GA1406>