Re: trace_mark ugliness

From: Mathieu Desnoyers
Date: Tue May 27 2008 - 09:37:05 EST


* Peter Zijlstra (peterz@xxxxxxxxxxxxx) wrote:
> On Thu, 2008-05-22 at 13:16 -0400, Mathieu Desnoyers wrote:
>
> > > The thing that bothers us the most is the force use of the "pretty print"
> > > interface. There's got to be a better way. I'd much rather see a
> > > file_marker.h file that has the interfaces defined, like what we have for
> > > sched.c.
> > >
> > > Where we have a sched_trace.h that has the defined prototypes. That is
> > > what the tracers should use too.
> > >
> > > The trace_mark should just have the string to find the tracer, but get rid
> > > of the "pretty print" aspect of it. Sorry, but the more I think about it,
> > > the nastier it seems. It forces all the users to do a va_start.
> > >
> > > I know you developed trace_mark for LTT, and that's great. But where I'm
> > > disagreeing is that you should not force all other users of trace_mark to
> > > conform to the LTT way when it can be easier to have LTT conform to a more
> > > generic way.
> > >
> > > Hence, this is what I propose.
> > >
> > > Remove the format part altogether, the format should be checked via the
> > > prototype. I know that you are afraid of changes to markers and that
> > > breaking code, but honestly, that is up to the developers of the tracers
> > > to fix. This should not be placed in the code itself. The markers
> > > shouldn't change anyway. If there is to be a check, it should be a compile
> > > time check (i.e. prototype compare) not a runtime check (as it is now).
> > >
> >
> > Hrm, hrm, ok, let's brainstorm along these lines. So we would like to
> > have :
> > - Multiple tracers
> > - Each tracer can connect either to one or more different markers
> > - Each marker should support many tracers connected to it
> > - Checking for marker/tracer probe compatibility should be done via
> > function prototypes.
> >
> > The main issue here seems to be to support multiple probes connected at
> > once on a given marker. With the current markers, I deal with this by
> > taking a pointer on the va_list and go through as many va_start/va_end
> > as required (one pair for each connected probe). By the way, the probes
> > does not have to issue va_start/end; marker.c deals with this.
> >
> > Also, given that I want to support SystemTAP, it adds the following
> > constraint : we cannot expect the probes to be there at compile-time,
> > since they can be provided by modules built much later. Therefore, we
> > have to provide support for dynamic connection of an arbitrary number of
> > probes on any given marker.
> >
> > So while I *could* remove the format string easily, it's the variable
> > argument list which I don't see clearly how to drop while still
> > providing flexible argument types -and- compile-time type verification.
> >
> > What currently looks like (this is a simplified pseudo-code) :
> >
> > void marker_probe_cb(const struct marker *mdata, void *call_private, ...)
> > {
> > va_list args;
> > int i;
> >
> > preempt_disable();
> > for (i = 0; multi[i].func; i++) {
> > va_start(args, call_private);
> > multi[i].func(multi[i].probe_private, call_private,
> > mdata->format, &args);
> > va_end(args);
> > }
> > preempt_enable();
> > }
> >
> > Would have to be changed into specialized functions for each marker,
> > involving quite a lot of code to be generated, e.g. :
> >
> > void marker_XXnameXX_probe_cb(const struct marker *mdata,
> > int arg1, void *arg2, struct mystruct *arg3)
> > {
> > int i;
> >
> > preempt_disable();
> > for (i = 0; multi[i].func; i++)
> > multi[i].func(multi[i].probe_private, arg1, arg2, arg3);
> > preempt_enable();
> > }
> >
> > That would imply that the struct marker_probe_closure, currently defined
> > as :
> >
> > typedef void marker_probe_func(void *probe_private, void *call_private,
> > const char *fmt, va_list *args);
> >
> > struct marker_probe_closure {
> > marker_probe_func *func; /* Callback */
> > void *probe_private; /* Private probe data */
> > };
> >
> > Would have to be duplicated for each marker prototype so we can provide
> > compile-time check of these prototypes. The registration functions would
> > also have to be duplicated to take parameters which include all those
> > various prototypes. They are required so that kernel modules can provide
> > probes (e.g. systemtap and LTTng).
> >
> > I don't really see how your proposal deals with these constraints
> > without duplicating much of the marker code on a per marker basis.
> > However, if we can find a clever way to do it without the code
> > duplication, I'm all in.
> >
> > Ideas/insights are welcome,
>
> How about something like:
>
> marker.c:
>
> void __trace_mark(const struct marker *mdata, va_list *args)
> {
> int i;
>
> preempt_disable();
> for (i = 0; multi[i].func; i++) {
> va_list l;
>
> va_copy(l, *args);
> multi[i].func(multi[i].probe_private, &l);
> va_end(l);
> }
> preempt_enable();
> }
>
>
> marker.h:
>
> #define TRACE_FUNC(name, args...) \
> static inline void trace_##name(const struct marker *mdata, ## args) \
> { \
> va_list l; \
> va_start(l, mdata); \
> __trace_mark(mdata, &l); \
> va_end(l); \
> }
>
> #define TRACE_MARK(name, args...) \
> trace_##name(trace_##name##_data, ## args)
>
> TRACE_FUNC(sched_switch, const struct task_struct *prev, const struct task_struct *next)
>
>
> sched.c:
>
> TRACE_MARK(sched_switch, prev, next);
>

Hi Peter,

Thanks for looking into this. There seems that there are a few problems
with the solution you propose. The first problem being that there is a
va_start in a function taking fixed arguments (the generated
trace_##name function).

The second problem I see is that the callback registered to be called by
multi[i].func(multi[i].probe_private, &l); will have no type checking at
all, so the type checking problem is still present.

Mathieu



--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/