Problem with CREATE_TRACE_POINTS and recursion safety

Problem with CREATE_TRACE_POINTS and recursion safety

[Jeremy Fitzhardinge]

I'm having a problem with CREATE_TRACE_POINTS being too indiscriminate.  
The trouble is that it not only creates tracepoint definitions for the 
intended tracepoints, but any other tracepoint definitions which get 
included incidentally.

For example, I'm seeing my paravirt tracepoints being instantiated in 
both kernel/sched.o and kernel/irq/manage.o as side-effects of the 
scheduler and irq tracepoints being instantiated.

I'm experimenting with a different scheme, wherein a subsystem defines 
CREATE_FOO_TRACE_POINTS in the .c file where it wants to instantiate the 
tracepoints - rather than CREATE_TRACE_POINTS - and its 
trace/events/foo.h does:

    #ifdef CREATE_FOO_TRACE_POINTS
    #undef CREATE_FOO_TRACE_POINTS	/* avoid infinite recursion */
    #include <trace/instantiate_trace.h>
    #else
    #include <trace/define_trace.h>
    #endif

where instantiate_trace.h is:

    #define CREATE_TRACE_POINTS
    #include <trace/define_trace.h>
    #undef CREATE_TRACE_POINTS

(Just to prevent a bit more repeated boilerplate in each events 
definition file.)

This seems to work.  (Update: no it doesn't.  WTF has paravirt-trace.o 
got duplicate kmem tracepoints?!)

A related problem is that the TRACE_INCLUDE_FILE mechanism doesn't work, 
because if any tracepoint definition header sets it, it overrides the 
setting for all others, so, for example, arch/x86/include/asm/paravirt.h 
ends up being included as the source of definitions for events/sched.h.  
I gave up and moved paravirt-trace.h to trace/events/pvops.h rather than 
try to deal with it.

Lucky for you Steven, none of this is getting any more pretty, so you 
may yet get your prize.

    J

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Steven Rostedt]

On Wed, 15 Apr 2009, Jeremy Fitzhardinge wrote:

> I'm having a problem with CREATE_TRACE_POINTS being too indiscriminate.  The
> trouble is that it not only creates tracepoint definitions for the intended
> tracepoints, but any other tracepoint definitions which get included
> incidentally.
> 
> For example, I'm seeing my paravirt tracepoints being instantiated in both
> kernel/sched.o and kernel/irq/manage.o as side-effects of the scheduler and
> irq tracepoints being instantiated.
> 
> I'm experimenting with a different scheme, wherein a subsystem defines
> CREATE_FOO_TRACE_POINTS in the .c file where it wants to instantiate the
> tracepoints - rather than CREATE_TRACE_POINTS - and its trace/events/foo.h
> does:
> 
>    #ifdef CREATE_FOO_TRACE_POINTS
>    #undef CREATE_FOO_TRACE_POINTS	/* avoid infinite recursion */
>    #include <trace/instantiate_trace.h>
>    #else
>    #include <trace/define_trace.h>
>    #endif
> 
> where instantiate_trace.h is:
> 
>    #define CREATE_TRACE_POINTS
>    #include <trace/define_trace.h>
>    #undef CREATE_TRACE_POINTS
> 
> (Just to prevent a bit more repeated boilerplate in each events definition
> file.)
> 
> This seems to work.  (Update: no it doesn't.  WTF has paravirt-trace.o got
> duplicate kmem tracepoints?!)

kmem was a PITA.  Looks like we might need to do a:

#ifdef CREATE_FOO_TRACE_POINTS
#include <trace/define_trace.h>
#endif

type of thing, and have each user define their own CREATE_FOO_TRACE_POINTS 
that they want to instantiate. This should be a requirement on any 
trace point header that is used in other headers.

Does just adding the above to the kmem.h header and your header fix it for 
you?

-- Steve




> 
> A related problem is that the TRACE_INCLUDE_FILE mechanism doesn't work,
> because if any tracepoint definition header sets it, it overrides the setting
> for all others, so, for example, arch/x86/include/asm/paravirt.h ends up being
> included as the source of definitions for events/sched.h.  I gave up and moved
> paravirt-trace.h to trace/events/pvops.h rather than try to deal with it.

Those that define TRACE_INCLUDE_FILE need to undef it too. :-/

> 
> Lucky for you Steven, none of this is getting any more pretty, so you may yet
> get your prize.

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Jeremy Fitzhardinge]

Steven Rostedt wrote:
> On Wed, 15 Apr 2009, Jeremy Fitzhardinge wrote:
>
>   
>> I'm having a problem with CREATE_TRACE_POINTS being too indiscriminate.  The
>> trouble is that it not only creates tracepoint definitions for the intended
>> tracepoints, but any other tracepoint definitions which get included
>> incidentally.
>>
>> For example, I'm seeing my paravirt tracepoints being instantiated in both
>> kernel/sched.o and kernel/irq/manage.o as side-effects of the scheduler and
>> irq tracepoints being instantiated.
>>
>> I'm experimenting with a different scheme, wherein a subsystem defines
>> CREATE_FOO_TRACE_POINTS in the .c file where it wants to instantiate the
>> tracepoints - rather than CREATE_TRACE_POINTS - and its trace/events/foo.h
>> does:
>>
>>    #ifdef CREATE_FOO_TRACE_POINTS
>>    #undef CREATE_FOO_TRACE_POINTS	/* avoid infinite recursion */
>>    #include <trace/instantiate_trace.h>
>>    #else
>>    #include <trace/define_trace.h>
>>    #endif
>>
>> where instantiate_trace.h is:
>>
>>    #define CREATE_TRACE_POINTS
>>    #include <trace/define_trace.h>
>>    #undef CREATE_TRACE_POINTS
>>
>> (Just to prevent a bit more repeated boilerplate in each events definition
>> file.)
>>
>> This seems to work.  (Update: no it doesn't.  WTF has paravirt-trace.o got
>> duplicate kmem tracepoints?!)
>>     
>
> kmem was a PITA.  Looks like we might need to do a:
>
> #ifdef CREATE_FOO_TRACE_POINTS
> #include <trace/define_trace.h>
> #endif
>   

The sched and irq tracepoints have the same general problem.

> type of thing, and have each user define their own CREATE_FOO_TRACE_POINTS 
> that they want to instantiate. This should be a requirement on any 
> trace point header that is used in other headers.
>
> Does just adding the above to the kmem.h header and your header fix it for 
> you?
>   

No, I have a complete patch to do what I'm proposing here, and kmem went 
ahead and failed anyway.  I'll post a cleaned up set of RFC patches and 
try to track down what's happening later.

And there seems to be a secondary problem with kmem tracepoints being 
called without an explicit #include of <trace/events/kmem.h>, so I'm 
seeing compiler errors relating to that too:

  CC      arch/x86/kernel/paravirt-trace.o
In file included from /home/jeremy/git/linux/include/linux/slab.h:165,
                 from /home/jeremy/git/linux/include/linux/percpu.h:6,
                 from /home/jeremy/git/linux/include/linux/vmstat.h:6,
                 from /home/jeremy/git/linux/include/linux/mm.h:596,
                 from /home/jeremy/git/linux/include/linux/ring_buffer.h:6,
                 from /home/jeremy/git/linux/include/linux/ftrace_event.h:6,
                 from /home/jeremy/git/linux/include/trace/ftrace.h:20,
                 from /home/jeremy/git/linux/include/trace/define_trace.h:64,
                 from /home/jeremy/git/linux/include/trace/instantiate_trace.h:7,
                 from /home/jeremy/git/linux/include/trace/events/pvops.h:1186,
                 from /home/jeremy/git/linux/arch/x86/kernel/paravirt-trace.c:5:
/home/jeremy/git/linux/include/linux/slab_def.h: In function 'kmalloc':
/home/jeremy/git/linux/include/linux/slab_def.h:157: error: implicit declaration of function 'trace_kmalloc'


Also, the pvops trace stuff adds quite a lot of overhead to the kernel 
size - and probably runtime - so I think we'll need to have Kconfig 
switches for each set of trace events rather than a single fat 
CONFIG_TRACEPOINTS switch.

    J

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Steven Rostedt]

On Wed, 15 Apr 2009, Jeremy Fitzhardinge wrote:

> Steven Rostedt wrote:
> > On Wed, 15 Apr 2009, Jeremy Fitzhardinge wrote:
> > 
> >   
> > > I'm having a problem with CREATE_TRACE_POINTS being too indiscriminate.
> > > The
> > > trouble is that it not only creates tracepoint definitions for the
> > > intended
> > > tracepoints, but any other tracepoint definitions which get included
> > > incidentally.
> > > 
> > > For example, I'm seeing my paravirt tracepoints being instantiated in both
> > > kernel/sched.o and kernel/irq/manage.o as side-effects of the scheduler
> > > and
> > > irq tracepoints being instantiated.
> > > 
> > > I'm experimenting with a different scheme, wherein a subsystem defines
> > > CREATE_FOO_TRACE_POINTS in the .c file where it wants to instantiate the
> > > tracepoints - rather than CREATE_TRACE_POINTS - and its trace/events/foo.h
> > > does:
> > > 
> > >    #ifdef CREATE_FOO_TRACE_POINTS
> > >    #undef CREATE_FOO_TRACE_POINTS	/* avoid infinite recursion */
> > >    #include <trace/instantiate_trace.h>
> > >    #else
> > >    #include <trace/define_trace.h>
> > >    #endif
> > > 
> > > where instantiate_trace.h is:
> > > 
> > >    #define CREATE_TRACE_POINTS
> > >    #include <trace/define_trace.h>
> > >    #undef CREATE_TRACE_POINTS

Have you tried:

#ifdef CREATE_PVOPS_TRACE_POINTS
#define CREATE_TRACE_POINTS
#include <trace/define_trace.h>
#undef CREATE_TRACE_POINTS
#endif

And in your C file do:

#define CREATE_PVOPS_TRACE_POINTS
#include <trace/pvops.h>

??

-- Steve

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> On Wed, 15 Apr 2009, Jeremy Fitzhardinge wrote:
> 
> > Steven Rostedt wrote:
> > > On Wed, 15 Apr 2009, Jeremy Fitzhardinge wrote:
> > > 
> > >   
> > > > I'm having a problem with CREATE_TRACE_POINTS being too indiscriminate.
> > > > The
> > > > trouble is that it not only creates tracepoint definitions for the
> > > > intended
> > > > tracepoints, but any other tracepoint definitions which get included
> > > > incidentally.
> > > > 
> > > > For example, I'm seeing my paravirt tracepoints being instantiated in both
> > > > kernel/sched.o and kernel/irq/manage.o as side-effects of the scheduler
> > > > and
> > > > irq tracepoints being instantiated.
> > > > 
> > > > I'm experimenting with a different scheme, wherein a subsystem defines
> > > > CREATE_FOO_TRACE_POINTS in the .c file where it wants to instantiate the
> > > > tracepoints - rather than CREATE_TRACE_POINTS - and its trace/events/foo.h
> > > > does:
> > > > 
> > > >    #ifdef CREATE_FOO_TRACE_POINTS
> > > >    #undef CREATE_FOO_TRACE_POINTS	/* avoid infinite recursion */
> > > >    #include <trace/instantiate_trace.h>
> > > >    #else
> > > >    #include <trace/define_trace.h>
> > > >    #endif
> > > > 
> > > > where instantiate_trace.h is:
> > > > 
> > > >    #define CREATE_TRACE_POINTS
> > > >    #include <trace/define_trace.h>
> > > >    #undef CREATE_TRACE_POINTS
> 
> Have you tried:
> 
> #ifdef CREATE_PVOPS_TRACE_POINTS
> #define CREATE_TRACE_POINTS
> #include <trace/define_trace.h>
> #undef CREATE_TRACE_POINTS
> #endif
> 
> And in your C file do:
> 
> #define CREATE_PVOPS_TRACE_POINTS
> #include <trace/pvops.h>
> 
> ??
> 
> -- Steve

Jeremy brings up an interesting point. Given that we might eventually
include a few tracepoint header files in a given C file, but with the
intent of only "creating" the tracepoint callbacks for few of these, the
global "CREATE_TRACE_POINTS" flag makes little sense and seems like it
will easily lead to link-time errors.

Maybe we could consider requiring something like the following solution :

In the .c file :

#define CREATE_TRACE_POINTS
#include <trace/subsysa.h>
#undef CREATE_TRACE_POINTS

#include <trace/subsysb.h>

Where subsysa has its trace points callbacks created, but subsysb
doesn't. This seems half-way understandable, at least.

Mathieu

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

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Jeremy Fitzhardinge]

Steven Rostedt wrote:
> Have you tried:
>
> #ifdef CREATE_PVOPS_TRACE_POINTS
> #define CREATE_TRACE_POINTS
> #include <trace/define_trace.h>
> #undef CREATE_TRACE_POINTS
> #endif
>
> And in your C file do:
>
> #define CREATE_PVOPS_TRACE_POINTS
> #include <trace/pvops.h>
>   

Yes, that's precisely what I do.

    J

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Jeremy Fitzhardinge]

Mathieu Desnoyers wrote:
> Jeremy brings up an interesting point. Given that we might eventually
> include a few tracepoint header files in a given C file, but with the
> intent of only "creating" the tracepoint callbacks for few of these, the
> global "CREATE_TRACE_POINTS" flag makes little sense and seems like it
> will easily lead to link-time errors.
>   

Yes, that's what I'm seeing.

> Maybe we could consider requiring something like the following solution :
>
> In the .c file :
>
> #define CREATE_TRACE_POINTS
> #include <trace/subsysa.h>
> #undef CREATE_TRACE_POINTS
>
> #include <trace/subsysb.h>
>
> Where subsysa has its trace points callbacks created, but subsysb
> doesn't. This seems half-way understandable, at least.

But what if trace/subsysa.h includes some other headers to define some 
types it needs, which in turn ends up incidentally including 
trace/subsysc.h (either directly, or indirectly via any number of other 
headers?  Then it will end up instantiating tracepoints for subsysa and 
subsysc.  The only way to avoid it would be to impose an absolutely 
strict separation of type/constant definition headers from 
function/tracepoint ones.

    J

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Steven Rostedt]

On Wed, 15 Apr 2009, Jeremy Fitzhardinge wrote:

> Mathieu Desnoyers wrote:
> > Jeremy brings up an interesting point. Given that we might eventually
> > include a few tracepoint header files in a given C file, but with the
> > intent of only "creating" the tracepoint callbacks for few of these, the
> > global "CREATE_TRACE_POINTS" flag makes little sense and seems like it
> > will easily lead to link-time errors.
> >   
> 
> Yes, that's what I'm seeing.
> 
> > Maybe we could consider requiring something like the following solution :
> > 
> > In the .c file :
> > 
> > #define CREATE_TRACE_POINTS
> > #include <trace/subsysa.h>
> > #undef CREATE_TRACE_POINTS
> > 
> > #include <trace/subsysb.h>
> > 
> > Where subsysa has its trace points callbacks created, but subsysb
> > doesn't. This seems half-way understandable, at least.
> 
> But what if trace/subsysa.h includes some other headers to define some types
> it needs, which in turn ends up incidentally including trace/subsysc.h (either
> directly, or indirectly via any number of other headers?  Then it will end up
> instantiating tracepoints for subsysa and subsysc.  The only way to avoid it
> would be to impose an absolutely strict separation of type/constant definition
> headers from function/tracepoint ones.

I hate to do this because it adds some more work to the developer adding a 
new trace point header, but we could just remove the CREATE_TRACE_POINTS 
and do in the trace/events/*.h headers:

#ifdef CREATE_FOO_TRACE_POINTS
#include <trace/define_trace.h>
#endif

in all tracepoint headers. I originally had it this way with just the 
CREATE_TRACE_POINTS, but Christoph Hellwig and Mathieu both suggested 
putting that into define_trace.h.  It seems so much cleaner to keep it in 
define_trace.h, but if it is causing too many headaches, it may not be 
worth it :-/

-- Steve

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Christoph Hellwig]

On Wed, Apr 15, 2009 at 11:09:14PM -0400, Steven Rostedt wrote:
> I hate to do this because it adds some more work to the developer adding a 
> new trace point header, but we could just remove the CREATE_TRACE_POINTS 
> and do in the trace/events/*.h headers:
> 
> #ifdef CREATE_FOO_TRACE_POINTS
> #include <trace/define_trace.h>
> #endif
> 
> in all tracepoint headers. I originally had it this way with just the 
> CREATE_TRACE_POINTS, but Christoph Hellwig and Mathieu both suggested 
> putting that into define_trace.h.  It seems so much cleaner to keep it in 
> define_trace.h, but if it is causing too many headaches, it may not be 
> worth it :-/

I still don't like the magic include in every trace header.

What about

#define CREATE_FOO_TRACE_POINTS	"subsystem"

to only create them for a given subsystem?

Re: Problem with CREATE_TRACE_POINTS and recursion safety

[Steven Rostedt]

On Thu, 16 Apr 2009, Christoph Hellwig wrote:

> On Wed, Apr 15, 2009 at 11:09:14PM -0400, Steven Rostedt wrote:
> > I hate to do this because it adds some more work to the developer adding a 
> > new trace point header, but we could just remove the CREATE_TRACE_POINTS 
> > and do in the trace/events/*.h headers:
> > 
> > #ifdef CREATE_FOO_TRACE_POINTS
> > #include <trace/define_trace.h>
> > #endif
> > 
> > in all tracepoint headers. I originally had it this way with just the 
> > CREATE_TRACE_POINTS, but Christoph Hellwig and Mathieu both suggested 
> > putting that into define_trace.h.  It seems so much cleaner to keep it in 
> > define_trace.h, but if it is causing too many headaches, it may not be 
> > worth it :-/
> 
> I still don't like the magic include in every trace header.
> 
> What about
> 
> #define CREATE_FOO_TRACE_POINTS	"subsystem"
> 
> to only create them for a given subsystem?

I tried this. But it seems that there's no way to compare strings with the 
preprocessor. You can compare numbers, but having each subsystem define 
its own number would be a pain to manage.

-- Steve