[PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[Zoltan Kiss]

The sample missed the moving of the header files into the events subdirectory.
I've also extended it based on the existing headers, and mentioned the tiny
but important role of CREATE_TRACE_POINTS.

Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
---
 Documentation/trace/tracepoints.txt |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/trace/tracepoints.txt b/Documentation/trace/tracepoints.txt
index da49437..e8e3c4b 100644
--- a/Documentation/trace/tracepoints.txt
+++ b/Documentation/trace/tracepoints.txt
@@ -40,7 +40,13 @@ Two elements are required for tracepoints :
 
 In order to use tracepoints, you should include linux/tracepoint.h.
 
-In include/trace/subsys.h :
+In include/trace/events/subsys.h :
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM subsys
+
+#if !defined(_TRACE_SUBSYS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_SUBSYS_H
 
 #include <linux/tracepoint.h>
 
@@ -48,10 +54,16 @@ DECLARE_TRACE(subsys_eventname,
 	TP_PROTO(int firstarg, struct task_struct *p),
 	TP_ARGS(firstarg, p));
 
+#endif /* _TRACE_SUBSYS_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
+
 In subsys/file.c (where the tracing statement must be added) :
 
-#include <trace/subsys.h>
+#include <trace/events/subsys.h>
 
+#define CREATE_TRACE_POINTS
 DEFINE_TRACE(subsys_eventname);
 
 void somefct(void)
@@ -72,6 +84,9 @@ Where :
 - TP_ARGS(firstarg, p) are the parameters names, same as found in the
   prototype.
 
+- if you use the header in multiple source files, #define CREATE_TRACE_POINTS
+  should appear only in one source file
+
 Connecting a function (probe) to a tracepoint is done by providing a
 probe (function to call) for the specific tracepoint through
 register_trace_subsys_eventname().  Removing a probe is done through
-- 
1.7.9.5

Re: [PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[Mathieu Desnoyers]

* Zoltan Kiss (zoltan.kiss@citrix.com) wrote:
> The sample missed the moving of the header files into the events subdirectory.
> I've also extended it based on the existing headers, and mentioned the tiny
> but important role of CREATE_TRACE_POINTS.

Given that we expect tracepoints to be used though the TRACE_EVENT
wrapper, it makes sense indeed. A small nit below:

> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> ---
>  Documentation/trace/tracepoints.txt |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/trace/tracepoints.txt b/Documentation/trace/tracepoints.txt
> index da49437..e8e3c4b 100644
> --- a/Documentation/trace/tracepoints.txt
> +++ b/Documentation/trace/tracepoints.txt
> @@ -40,7 +40,13 @@ Two elements are required for tracepoints :
>  
>  In order to use tracepoints, you should include linux/tracepoint.h.
>  
> -In include/trace/subsys.h :
> +In include/trace/events/subsys.h :
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM subsys
> +
> +#if !defined(_TRACE_SUBSYS_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_SUBSYS_H
>  
>  #include <linux/tracepoint.h>
>  
> @@ -48,10 +54,16 @@ DECLARE_TRACE(subsys_eventname,
>  	TP_PROTO(int firstarg, struct task_struct *p),
>  	TP_ARGS(firstarg, p));
>  
> +#endif /* _TRACE_SUBSYS_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> +
>  In subsys/file.c (where the tracing statement must be added) :
>  
> -#include <trace/subsys.h>
> +#include <trace/events/subsys.h>
>  
> +#define CREATE_TRACE_POINTS
>  DEFINE_TRACE(subsys_eventname);
>  
>  void somefct(void)
> @@ -72,6 +84,9 @@ Where :
>  - TP_ARGS(firstarg, p) are the parameters names, same as found in the
>    prototype.
>  
> +- if you use the header in multiple source files, #define CREATE_TRACE_POINTS
> +  should appear only in one source file

Missing dot at the end of the sentence above.

Other than that,

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks!

Mathieu

> +
>  Connecting a function (probe) to a tracepoint is done by providing a
>  probe (function to call) for the specific tracepoint through
>  register_trace_subsys_eventname().  Removing a probe is done through
> -- 
> 1.7.9.5
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[Rob Landley]

On 08/22/2013 04:49:31 PM, Zoltan Kiss wrote:
> The sample missed the moving of the header files into the events  
> subdirectory.
> I've also extended it based on the existing headers, and mentioned  
> the tiny
> but important role of CREATE_TRACE_POINTS.
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> ---
>  Documentation/trace/tracepoints.txt |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/trace/tracepoints.txt  
> b/Documentation/trace/tracepoints.txt
> index da49437..e8e3c4b 100644
> --- a/Documentation/trace/tracepoints.txt
> +++ b/Documentation/trace/tracepoints.txt
> @@ -40,7 +40,13 @@ Two elements are required for tracepoints :
> 
>  In order to use tracepoints, you should include linux/tracepoint.h.
> 
> -In include/trace/subsys.h :
> +In include/trace/events/subsys.h :
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM subsys

That addition I can sort of see, I guess?

> +#if !defined(_TRACE_SUBSYS_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_SUBSYS_H

But this makes no sense to me: why is it needed? (I.E. why must it be  
block copied into each _user_ of tracepoints?)

>  #include <linux/tracepoint.h>
> 
> @@ -48,10 +54,16 @@ DECLARE_TRACE(subsys_eventname,
>  	TP_PROTO(int firstarg, struct task_struct *p),
>  	TP_ARGS(firstarg, p));
> 
> +#endif /* _TRACE_SUBSYS_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> +

Why? (Both why do you need to #include a header outside a multiple  
inclusion guard, and why is the additional header needed at all in  
_every_ subsystem trace header?)

>  In subsys/file.c (where the tracing statement must be added) :
> 
> -#include <trace/subsys.h>
> +#include <trace/events/subsys.h>
> 
> +#define CREATE_TRACE_POINTS
>  DEFINE_TRACE(subsys_eventname);
> 
>  void somefct(void)
> @@ -72,6 +84,9 @@ Where :
>  - TP_ARGS(firstarg, p) are the parameters names, same as found in the
>    prototype.
> 
> +- if you use the header in multiple source files, #define  
> CREATE_TRACE_POINTS
> +  should appear only in one source file
> +
>  Connecting a function (probe) to a tracepoint is done by providing a
>  probe (function to call) for the specific tracepoint through
>  register_trace_subsys_eventname().  Removing a probe is done through

I guess the documentation isn't at fault if the tracepoint subsystem is  
suddenly becoming a lot more repetitive and complicated for no readily  
apparent reason, but did anybody ask _why_ this thing changed? Can we  
document why the extra redundancy is needed?

Rob

Re: [PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[Jiri Kosina]

On Sat, 24 Aug 2013, Mathieu Desnoyers wrote:

> * Zoltan Kiss (zoltan.kiss@citrix.com) wrote:
> > The sample missed the moving of the header files into the events subdirectory.
> > I've also extended it based on the existing headers, and mentioned the tiny
> > but important role of CREATE_TRACE_POINTS.
> 
> Given that we expect tracepoints to be used though the TRACE_EVENT
> wrapper, it makes sense indeed. A small nit below:
> 
> > 
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> > ---
> >  Documentation/trace/tracepoints.txt |   19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/trace/tracepoints.txt b/Documentation/trace/tracepoints.txt
> > index da49437..e8e3c4b 100644
> > --- a/Documentation/trace/tracepoints.txt
> > +++ b/Documentation/trace/tracepoints.txt
> > @@ -40,7 +40,13 @@ Two elements are required for tracepoints :
> >  
> >  In order to use tracepoints, you should include linux/tracepoint.h.
> >  
> > -In include/trace/subsys.h :
> > +In include/trace/events/subsys.h :
> > +
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM subsys
> > +
> > +#if !defined(_TRACE_SUBSYS_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _TRACE_SUBSYS_H
> >  
> >  #include <linux/tracepoint.h>
> >  
> > @@ -48,10 +54,16 @@ DECLARE_TRACE(subsys_eventname,
> >  	TP_PROTO(int firstarg, struct task_struct *p),
> >  	TP_ARGS(firstarg, p));
> >  
> > +#endif /* _TRACE_SUBSYS_H */
> > +
> > +/* This part must be outside protection */
> > +#include <trace/define_trace.h>
> > +
> >  In subsys/file.c (where the tracing statement must be added) :
> >  
> > -#include <trace/subsys.h>
> > +#include <trace/events/subsys.h>
> >  
> > +#define CREATE_TRACE_POINTS
> >  DEFINE_TRACE(subsys_eventname);
> >  
> >  void somefct(void)
> > @@ -72,6 +84,9 @@ Where :
> >  - TP_ARGS(firstarg, p) are the parameters names, same as found in the
> >    prototype.
> >  
> > +- if you use the header in multiple source files, #define CREATE_TRACE_POINTS
> > +  should appear only in one source file
> 
> Missing dot at the end of the sentence above.
> 
> Other than that,
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

I have added the dot and applied :)

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[Zoltan Kiss]

Hi,

I'm not very familiar with the tracing framework, but I will try to 
comment on your questions.

On 25/08/13 09:59, Rob Landley wrote:
> On 08/22/2013 04:49:31 PM, Zoltan Kiss wrote:
>> +#if !defined(_TRACE_SUBSYS_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_SUBSYS_H
>
> But this makes no sense to me: why is it needed? (I.E. why must it be
> block copied into each _user_ of tracepoints?)
This is to prevent header reinclusion, the second condition makes it 
possible to include it again from trace/define_trace.h

>>  #include <linux/tracepoint.h>
>>
>> @@ -48,10 +54,16 @@ DECLARE_TRACE(subsys_eventname,
>>      TP_PROTO(int firstarg, struct task_struct *p),
>>      TP_ARGS(firstarg, p));
>>
>> +#endif /* _TRACE_SUBSYS_H */
>> +
>> +/* This part must be outside protection */
>> +#include <trace/define_trace.h>
>> +
>
> Why? (Both why do you need to #include a header outside a multiple
> inclusion guard, and why is the additional header needed at all in
> _every_ subsystem trace header?)
I see only one inclusion guard here, the one above. define_trace.h 
should take effect at only one place, where CREATE_TRACE_POINTS is 
defined, to create the tracepoints exactly once. However I don't see as 
well why it should be outside protection. Maybe because the intentional 
header reinclusion in it?

Regards,

Zoli

Re: [PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[Steven Rostedt]

On Mon, 2 Sep 2013 18:02:47 +0100
Zoltan Kiss <zoltan.kiss@citrix.com> wrote:


> > Why? (Both why do you need to #include a header outside a multiple
> > inclusion guard, and why is the additional header needed at all in
> > _every_ subsystem trace header?)
> I see only one inclusion guard here, the one above. define_trace.h 
> should take effect at only one place, where CREATE_TRACE_POINTS is 
> defined, to create the tracepoints exactly once. However I don't see as 
> well why it should be outside protection. Maybe because the intentional 
> header reinclusion in it?

Because if it was inside the protection you can miss the
CREATE_TRACE_POINTS define. These headers can be included by other
headers (although they probably shouldn't be). But that's the point of
the protection isn't it? In case multiple headers include the file?

Anyway, lets say the header was include earlier, thus you can have this:


#include <trace/foo.h> // include by another header

#define CREATE_TRACE_POINTS

#include <trace/foo.h>


Now if the include of define_trace.h was inside the protection, it will
not get included again, and no trace points would be created.

-- Steve

Re: [PATCH] Documentation/trace: Correcting and extending tracepoint documentation

[Steven Rostedt]

Cleaning out my inbox I find things that are fun to read.

On Mon, 2 Sep 2013 18:02:47 +0100
Zoltan Kiss <zoltan.kiss@citrix.com> wrote:

> Hi,
> 
> I'm not very familiar with the tracing framework, but I will try to 
> comment on your questions.
> 
> On 25/08/13 09:59, Rob Landley wrote:
> > On 08/22/2013 04:49:31 PM, Zoltan Kiss wrote:
> >> +#if !defined(_TRACE_SUBSYS_H) || defined(TRACE_HEADER_MULTI_READ)
> >> +#define _TRACE_SUBSYS_H
> >
> > But this makes no sense to me: why is it needed? (I.E. why must it be
> > block copied into each _user_ of tracepoints?)
> This is to prevent header reinclusion, the second condition makes it 
> possible to include it again from trace/define_trace.h

Right. The TRACE_HEADER_MULTI_READ must be there to allow the multiple
inclusion of the tracepoint header that is used by not only
define_trace.h, but also from include/trace/ftrace.h.

If you look in that file you'll find this:

#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)

where TRACE_INCLUDE(TRACE_INCLUDE_FILE) is defined as your header file
and it will include your data again.

It uses this basic idea:

#define DOGS \
   C(JACK_RUSSELL,	1), \
   C(ITALIAN_GREYHOUND, 2), \
   C(GERMAN_SHEPARD,	3)

#undef C
#define C(a, b)	#a

char *dog_names[] = { DOGS };

#undef C
#define C(a, b) a = b

enum dog_numbers { DOGS };

And so on.

That is, we abuse the C(a,b) macro to redefine what it stands for, and
then use DOGS, which has the C(a,b) redefined and can create strings of
dogs, enums of dogs, and guarantee that they always map correctly.

The ftrace.h file is basically that, and it abuses the various things
within TRACE_EVENT() to create all the code necessary to add a
tracepoint. All one needs to do is follow the example in
samples/trace_events/ and there events will magically appear in
the /sys/kernel/debug/tracing/events directory and they can trace there
data without having to worry at all about how the tracing
infrastructure actually works. That is, they don't need to write code
to make the tracing work. They just need to create a TRACE_EVENT() and
follow the directions.

Considering there's now over a thousand tracepoints in the kernel means
that this method worked rather well.



> 
> >>  #include <linux/tracepoint.h>
> >>
> >> @@ -48,10 +54,16 @@ DECLARE_TRACE(subsys_eventname,
> >>      TP_PROTO(int firstarg, struct task_struct *p),
> >>      TP_ARGS(firstarg, p));
> >>
> >> +#endif /* _TRACE_SUBSYS_H */
> >> +
> >> +/* This part must be outside protection */
> >> +#include <trace/define_trace.h>
> >> +
> >
> > Why? (Both why do you need to #include a header outside a multiple
> > inclusion guard, and why is the additional header needed at all in
> > _every_ subsystem trace header?)
> I see only one inclusion guard here, the one above. define_trace.h 
> should take effect at only one place, where CREATE_TRACE_POINTS is 
> defined, to create the tracepoints exactly once. However I don't see as 
> well why it should be outside protection. Maybe because the intentional 
> header reinclusion in it?

The define_trace.h is protected by:

#ifdef CREATE_TRACE_TRACEPOINTS

which it quickly undefines, to prevent recursion. But define_trace also
defines the TRACE_HEADER_MULTI_READ to let the code enter again.

The reason define_trace.h is outside of protection is because the trace
header may be included in header files which will define the
TRACE_SUBSYS_H. Since the CREATE_TRACE_POINTS is not defined yet, the
define_trace.h wont do anything. But since the TRACE_SUBSYS_H has
already been defined, and the define_trace.h is what would define the
TRACE_HEADER_MULTI_READ, then the define_trace.h would not be
accessible when the CREATE_TRACE_POINTS is defined and the header is
included again.

Make sense?

Also, it looks like that tracepoint.txt file does need some update.

Want to resend this patch?

I should probably write up a tracepoint-design.txt document that
explains the workings of the tracepoints in include/trace.

(I'll just add that to my long list of TODOs :-/ )

-- Steve