trace_mark ugliness

trace_mark ugliness

[Steven Rostedt]

Mathieu,

I'm working back on ftrace and I see that you have a lot of updates with
the trace markers there. I'm going about cleaning up the code since some
of the changes broke some of the functionality of ftrace.

That's not a big deal, the breakage is easily fixed, and it's not taking
much of my time. But I've been discussing the trace_mark itself with Peter
Zijlstra and it still makes us cringe when we see it.

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).

I like the rest of trace_mark, it is just this print format ugliness that
boths me (and others). It is very clumsy to work with. There just has to
be a better way here.

As for LTT, you can make your own LTT wrapper to add the print formats for
userspace. Lets not force this on other traces like ftrace that just
wastes time hopping over the parameters that it doesn't need.

Thanks,

-- Steve

Re: trace_mark ugliness

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> Mathieu,
> 
> I'm working back on ftrace and I see that you have a lot of updates with
> the trace markers there. I'm going about cleaning up the code since some
> of the changes broke some of the functionality of ftrace.
> 
> That's not a big deal, the breakage is easily fixed, and it's not taking
> much of my time. But I've been discussing the trace_mark itself with Peter
> Zijlstra and it still makes us cringe when we see it.
> 

Hi Steven,

Thanks for the fix,

> 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,

Mathieu

> I like the rest of trace_mark, it is just this print format ugliness that
> boths me (and others). It is very clumsy to work with. There just has to
> be a better way here.
> 
> As for LTT, you can make your own LTT wrapper to add the print formats for
> userspace. Lets not force this on other traces like ftrace that just
> wastes time hopping over the parameters that it doesn't need.
> 
> Thanks,
> 
> -- Steve
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: trace_mark ugliness

[Steven Rostedt]

On Thu, 22 May 2008, 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

Great! This is what I wanted to hear. I sent this email out to start up
a brainstorming session.

> 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.

Interesting.

>
> 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.

Yep understood.

>
> 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,

Thanks for explaining this more. I'll try to think of some tricks to
handle this. It may end up that we have to have the format after all, but
it still just seems a bit messy. If we can encapsulate this into some
compile time tricks, without duplicating code all over the place, then
this that would be the way I would like to go.

I CC'd others in hoping that they too might have some clever tricks to
solve this.

Thanks Mathieu!

-- Steve

Re: trace_mark ugliness

[Peter Zijlstra]

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);

Re: trace_mark ugliness

[Mathieu Desnoyers]

* Peter Zijlstra (peterz@infradead.org) 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

Re: trace_mark ugliness

[Peter Zijlstra]

On Tue, 2008-05-27 at 09:36 -0400, Mathieu Desnoyers wrote:
> * Peter Zijlstra (peterz@infradead.org) 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).

Hmm, indeed it seems that isn't legal :-(

> 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.

Doesn't leave much room...

void _trace_mark(const struct marker *mdata, ...)
{
	va_list l;

	preempt_disable();
	for (iter = mdata->funcs; iter; iter = iter->next) {	
		va_start(l, mdata);
		iter->func(mdata, &l);
		va_end(l);
	}
	preempt_enable();
}

#define TRACE_FUNC4(name, type1, type2, type3, type4)			\
struct marker_##name {							\
	struct marker mdata;						\
	void (*func_##name)(type1, type2, type3, type4);		\
}									\
static inline void trace_##name##_func(const struct marker *mdata,	\
	type1 a1, type2 a2, type3 a3, type4 a4)				\
{									\
	_trace_mark(mdata, a1, a2, a3, a4);				\
}									\
static inline void _trace_##name##_call(const struct marker *mdata, 	\
	va_list *l)							\
{									\
	type1 a1 = va_arg(*l, type1);					\
	type2 a2 = va_arg(*l, type2);					\
	type3 a3 = va_arg(*l, type3);					\
	type4 a4 = va_arg(*l, type4);					\
									\
	((struct marker_##name *)mdata)->func_##name(a1, a2, a3, a4); 	\
}

The sad truth is that gcc is taking out most of the function forwarding
helpers in favour of libffi, and libffi is far from self hosting and
quite horrible to look at - so importing that into the kernel isn't
going to work either :-/

Re: trace_mark ugliness

[Anthony Green]

Peter Zijlstra <peterz <at> infradead.org> writes:
> The sad truth is that gcc is taking out most of the function forwarding
> helpers in favour of libffi, and libffi is far from self hosting and
> quite horrible to look at - so importing that into the kernel isn't
> going to work either :-/

Peter,

Either you're using the term "self hosting" in some way I don't understand, or
you meant to say that it doesn't build outside of the GCC tree.  Actually, it
does, if you use the upstream version here: http://sourceware.org/libffi.

Anthony Green