lkml.org 
[lkml]   [2008]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: trace_mark ugliness
From
Date
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);



\
 
 \ /
  Last update: 2008-05-25 13:23    [from the cache]
©2003-2011 Jasper Spaans