[PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
be included with CREATE_TRACE_EVENTS defined

I've been workingon adding a few tracepoints to the network stack, and in so
doing it was convienient for me to add a second include file to
net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
run through define_trace.h (which is included from skb.h, included in
net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
to start a new cycle with the next header.

The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
This places us back in the proper state to define a new set of tracepoints in a
subsequent header file.  Not sure if theres a better way to handle this, but
this worked well for me, allowing me to include multiple headers with
TRACE_EVENT macros in a c file with CREATE_TRACE_POINTS defined.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


linux/tracepoint.h   |   18 ++++++++++++++++++
trace/define_trace.h |    1 +
2 files changed, 19 insertions(+)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 2aac8a8..562d8ba 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -173,6 +173,24 @@ static inline void tracepoint_synchronize_unregister(void)
  *  trace event headers under one "CREATE_TRACE_POINTS" the first include
  *  will override the TRACE_EVENT and break the second include.
  */
+#ifndef DECLARE_TRACE
+#define DECLARE_TRACE(name, proto, args)                                \
+	extern struct tracepoint __tracepoint_##name;                   \
+	static inline void trace_##name(proto)                          \
+	{                                                               \
+		if (unlikely(__tracepoint_##name.state))                \
+			__DO_TRACE(&__tracepoint_##name,                \
+				TP_PROTO(proto), TP_ARGS(args));        \
+	}                                                               \
+	static inline int register_trace_##name(void (*probe)(proto))   \
+	{                                                               \
+		return tracepoint_probe_register(#name, (void *)probe); \
+	}                                                               \
+	static inline int unregister_trace_##name(void (*probe)(proto)) \
+	{                                                               \
+		return tracepoint_probe_unregister(#name, (void *)probe);\
+	}
+#endif
 
 #ifndef TRACE_EVENT
 /*
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 2a4b3bf..a3e0095 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -64,6 +64,7 @@
 #undef TRACE_EVENT
 #undef TRACE_EVENT_FN
 #undef TRACE_HEADER_MULTI_READ
+#undef DECLARE_TRACE
 
 /* Only undef what we defined in this file */
 #ifdef UNDEF_TRACE_INCLUDE_FILE

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Wed, 2009-12-02 at 14:19 -0500, Neil Horman wrote:
> Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> be included with CREATE_TRACE_EVENTS defined
> 
> I've been workingon adding a few tracepoints to the network stack, and in so
> doing it was convienient for me to add a second include file to
> net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
> CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
> define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> run through define_trace.h (which is included from skb.h, included in
> net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> to start a new cycle with the next header.
> 
> The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> This places us back in the proper state to define a new set of tracepoints in a
> subsequent header file.  Not sure if theres a better way to handle this, but
> this worked well for me, allowing me to include multiple headers with
> TRACE_EVENT macros in a c file with CREATE_TRACE_POINTS defined.
> 

Nice, just some comments below.

> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
> linux/tracepoint.h   |   18 ++++++++++++++++++
> trace/define_trace.h |    1 +
> 2 files changed, 19 insertions(+)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 2aac8a8..562d8ba 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -173,6 +173,24 @@ static inline void tracepoint_synchronize_unregister(void)
>   *  trace event headers under one "CREATE_TRACE_POINTS" the first include
>   *  will override the TRACE_EVENT and break the second include.
>   */
> +#ifndef DECLARE_TRACE
> +#define DECLARE_TRACE(name, proto, args)                                \
> +	extern struct tracepoint __tracepoint_##name;                   \
> +	static inline void trace_##name(proto)                          \
> +	{                                                               \
> +		if (unlikely(__tracepoint_##name.state))                \
> +			__DO_TRACE(&__tracepoint_##name,                \
> +				TP_PROTO(proto), TP_ARGS(args));        \
> +	}                                                               \
> +	static inline int register_trace_##name(void (*probe)(proto))   \
> +	{                                                               \
> +		return tracepoint_probe_register(#name, (void *)probe); \
> +	}                                                               \
> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> +	{                                                               \
> +		return tracepoint_probe_unregister(#name, (void *)probe);\
> +	}
> +#endif
>  

You duplicate the DECLARE_TRACE from above and place it into an
unprotected area. Why not move it instead of duplicating it. This is
just a bug waiting to happen if we ever need to modify DECLARE_TRACE.

If you change it to:

#if !defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)

#define DECLARE_TRACE(...) ....

#endif

Then you can remove the first definition of it. The first time
processing the file, when it hits the #ifndef DECLARE_TRACE in the
_LINUX_TRACEPOINT_H conditional, it still does not define DECLARE_TRACE,
and then when it exits that conditional, it does.

-- Steve


>  #ifndef TRACE_EVENT
>  /*
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index 2a4b3bf..a3e0095 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -64,6 +64,7 @@
>  #undef TRACE_EVENT
>  #undef TRACE_EVENT_FN
>  #undef TRACE_HEADER_MULTI_READ
> +#undef DECLARE_TRACE
>  
>  /* Only undef what we defined in this file */
>  #ifdef UNDEF_TRACE_INCLUDE_FILE

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Thu, Dec 03, 2009 at 09:15:26AM -0500, Steven Rostedt wrote:
> On Wed, 2009-12-02 at 14:19 -0500, Neil Horman wrote:
> > Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> > be included with CREATE_TRACE_EVENTS defined
> > 
> > I've been workingon adding a few tracepoints to the network stack, and in so
> > doing it was convienient for me to add a second include file to
> > net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
> > CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> > duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
> > define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> > run through define_trace.h (which is included from skb.h, included in
> > net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> > to start a new cycle with the next header.
> > 
> > The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> > end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> > This places us back in the proper state to define a new set of tracepoints in a
> > subsequent header file.  Not sure if theres a better way to handle this, but
> > this worked well for me, allowing me to include multiple headers with
> > TRACE_EVENT macros in a c file with CREATE_TRACE_POINTS defined.
> > 
> 
> Nice, just some comments below.
><snip> 
> 
> You duplicate the DECLARE_TRACE from above and place it into an
> unprotected area. Why not move it instead of duplicating it. This is
> just a bug waiting to happen if we ever need to modify DECLARE_TRACE.
> 
> If you change it to:
> 
> #if !defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
> 
> #define DECLARE_TRACE(...) ....
> 
> #endif
> 
> Then you can remove the first definition of it. The first time
> processing the file, when it hits the #ifndef DECLARE_TRACE in the
> _LINUX_TRACEPOINT_H conditional, it still does not define DECLARE_TRACE,
> and then when it exits that conditional, it does.


Yeah, that makes sense to me.  Although I think you mean if
defined(CONFIG_TRACEPOINTS) rather than if !defined(CONFIG_TRACEPOINTS).  The
case I'm moving to outside the unprotected area is the one that we use if
tracepoints are enabled.  I just tested this new patch, and it works well for
me.  Thanks!

Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
 trace/define_trace.h |    1 +
 2 files changed, 24 insertions(+), 22 deletions(-)


Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
be included with CREATE_TRACE_EVENTS defined

I've been workingon adding a few tracepoints to the network stack, and in so
doing it was convienient for me to add a second include file to
net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
run through define_trace.h (which is included from skb.h, included in
net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
to start a new cycle with the next header.

The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
This places us back in the proper state to define a new set of tracepoints in a
subsequent header file.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 2aac8a8..311abbf 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -58,28 +58,6 @@ struct tracepoint {
 		rcu_read_unlock_sched_notrace();			\
 	} while (0)
 
-/*
- * Make sure the alignment of the structure in the __tracepoints section will
- * not add unwanted padding between the beginning of the section and the
- * structure. Force alignment to the same alignment as the section start.
- */
-#define DECLARE_TRACE(name, proto, args)				\
-	extern struct tracepoint __tracepoint_##name;			\
-	static inline void trace_##name(proto)				\
-	{								\
-		if (unlikely(__tracepoint_##name.state))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(proto), TP_ARGS(args));	\
-	}								\
-	static inline int register_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_register(#name, (void *)probe);	\
-	}								\
-	static inline int unregister_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_unregister(#name, (void *)probe);\
-	}
-
 
 #define DEFINE_TRACE_FN(name, reg, unreg)				\
 	static const char __tpstrtab_##name[]				\
@@ -173,6 +151,29 @@ static inline void tracepoint_synchronize_unregister(void)
  *  trace event headers under one "CREATE_TRACE_POINTS" the first include
  *  will override the TRACE_EVENT and break the second include.
  */
+#if defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
+/*
+ * Make sure the alignment of the structure in the __tracepoints section will
+ * not add unwanted padding between the beginning of the section and the
+ * structure. Force alignment to the same alignment as the section start.
+ */
+#define DECLARE_TRACE(name, proto, args)                                \
+	extern struct tracepoint __tracepoint_##name;                   \
+	static inline void trace_##name(proto)                          \
+	{                                                               \
+		if (unlikely(__tracepoint_##name.state))                \
+			__DO_TRACE(&__tracepoint_##name,                \
+				TP_PROTO(proto), TP_ARGS(args));        \
+	}                                                               \
+	static inline int register_trace_##name(void (*probe)(proto))   \
+	{                                                               \
+		return tracepoint_probe_register(#name, (void *)probe); \
+	}                                                               \
+	static inline int unregister_trace_##name(void (*probe)(proto)) \
+	{                                                               \
+		return tracepoint_probe_unregister(#name, (void *)probe);\
+	}
+#endif
 
 #ifndef TRACE_EVENT
 /*
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 2a4b3bf..a3e0095 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -64,6 +64,7 @@
 #undef TRACE_EVENT
 #undef TRACE_EVENT_FN
 #undef TRACE_HEADER_MULTI_READ
+#undef DECLARE_TRACE
 
 /* Only undef what we defined in this file */
 #ifdef UNDEF_TRACE_INCLUDE_FILE

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Thu, 2009-12-03 at 14:35 -0500, Neil Horman wrote:
> On Thu, Dec 03, 2009 at 09:15:26AM -0500, Steven Rostedt wrote:
> > On Wed, 2009-12-02 at 14:19 -0500, Neil Horman wrote:
> > > Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> > > be included with CREATE_TRACE_EVENTS defined
> > > 
> > > I've been workingon adding a few tracepoints to the network stack, and in so
> > > doing it was convienient for me to add a second include file to
> > > net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
> > > CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> > > duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
> > > define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> > > run through define_trace.h (which is included from skb.h, included in
> > > net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> > > to start a new cycle with the next header.
> > > 
> > > The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> > > end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> > > This places us back in the proper state to define a new set of tracepoints in a
> > > subsequent header file.  Not sure if theres a better way to handle this, but
> > > this worked well for me, allowing me to include multiple headers with
> > > TRACE_EVENT macros in a c file with CREATE_TRACE_POINTS defined.
> > > 
> > 
> > Nice, just some comments below.
> ><snip> 
> > 
> > You duplicate the DECLARE_TRACE from above and place it into an
> > unprotected area. Why not move it instead of duplicating it. This is
> > just a bug waiting to happen if we ever need to modify DECLARE_TRACE.
> > 
> > If you change it to:
> > 
> > #if !defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
> > 
> > #define DECLARE_TRACE(...) ....
> > 
> > #endif
> > 
> > Then you can remove the first definition of it. The first time
> > processing the file, when it hits the #ifndef DECLARE_TRACE in the
> > _LINUX_TRACEPOINT_H conditional, it still does not define DECLARE_TRACE,
> > and then when it exits that conditional, it does.
> 
> 
> Yeah, that makes sense to me.  Although I think you mean if
> defined(CONFIG_TRACEPOINTS) rather than if !defined(CONFIG_TRACEPOINTS).  The

Yep, that's what I meant ;-)

> case I'm moving to outside the unprotected area is the one that we use if
> tracepoints are enabled.  I just tested this new patch, and it works well for
> me.  Thanks!
> 
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

OK, I'll give it some testing tonight.

-- Steve

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Thu, 2009-12-03 at 14:35 -0500, Neil Horman wrote:

> Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> be included with CREATE_TRACE_EVENTS defined
> 
> I've been workingon adding a few tracepoints to the network stack, and in so
> doing it was convienient for me to add a second include file to
> net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
> CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
> define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> run through define_trace.h (which is included from skb.h, included in
> net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> to start a new cycle with the next header.
> 
> The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> This places us back in the proper state to define a new set of tracepoints in a
> subsequent header file.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 

By itself, this patch breaks the build (for me). I take it that you
converted the napi.h file to use TRACE_EVENT. Currently it uses a
DECLARE_TRACE, which will break with this patch. We need a patch that
removes that DECLARE_TRACE before we can apply this patch.

To keep bisectablity, we either need to make one patch that converts the
napi to TRACE_EVENT and this patch, or we need to first remove the
napi.h DECLARE_TRACE, add this patch, and then one that adds
TRACE_EVENT.

-- Steve

> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 2aac8a8..311abbf 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -58,28 +58,6 @@ struct tracepoint {
>  		rcu_read_unlock_sched_notrace();			\
>  	} while (0)
>  
> -/*
> - * Make sure the alignment of the structure in the __tracepoints section will
> - * not add unwanted padding between the beginning of the section and the
> - * structure. Force alignment to the same alignment as the section start.
> - */
> -#define DECLARE_TRACE(name, proto, args)				\
> -	extern struct tracepoint __tracepoint_##name;			\
> -	static inline void trace_##name(proto)				\
> -	{								\
> -		if (unlikely(__tracepoint_##name.state))		\
> -			__DO_TRACE(&__tracepoint_##name,		\
> -				TP_PROTO(proto), TP_ARGS(args));	\
> -	}								\
> -	static inline int register_trace_##name(void (*probe)(proto))	\
> -	{								\
> -		return tracepoint_probe_register(#name, (void *)probe);	\
> -	}								\
> -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> -	{								\
> -		return tracepoint_probe_unregister(#name, (void *)probe);\
> -	}
> -
>  
>  #define DEFINE_TRACE_FN(name, reg, unreg)				\
>  	static const char __tpstrtab_##name[]				\
> @@ -173,6 +151,29 @@ static inline void tracepoint_synchronize_unregister(void)
>   *  trace event headers under one "CREATE_TRACE_POINTS" the first include
>   *  will override the TRACE_EVENT and break the second include.
>   */
> +#if defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
> +/*
> + * Make sure the alignment of the structure in the __tracepoints section will
> + * not add unwanted padding between the beginning of the section and the
> + * structure. Force alignment to the same alignment as the section start.
> + */
> +#define DECLARE_TRACE(name, proto, args)                                \
> +	extern struct tracepoint __tracepoint_##name;                   \
> +	static inline void trace_##name(proto)                          \
> +	{                                                               \
> +		if (unlikely(__tracepoint_##name.state))                \
> +			__DO_TRACE(&__tracepoint_##name,                \
> +				TP_PROTO(proto), TP_ARGS(args));        \
> +	}                                                               \
> +	static inline int register_trace_##name(void (*probe)(proto))   \
> +	{                                                               \
> +		return tracepoint_probe_register(#name, (void *)probe); \
> +	}                                                               \
> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> +	{                                                               \
> +		return tracepoint_probe_unregister(#name, (void *)probe);\
> +	}
> +#endif
>  
>  #ifndef TRACE_EVENT
>  /*
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index 2a4b3bf..a3e0095 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -64,6 +64,7 @@
>  #undef TRACE_EVENT
>  #undef TRACE_EVENT_FN
>  #undef TRACE_HEADER_MULTI_READ
> +#undef DECLARE_TRACE
>  
>  /* Only undef what we defined in this file */
>  #ifdef UNDEF_TRACE_INCLUDE_FILE

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Mon, Dec 07, 2009 at 03:04:42PM -0500, Steven Rostedt wrote:
> On Thu, 2009-12-03 at 14:35 -0500, Neil Horman wrote:
> 
> > Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> > be included with CREATE_TRACE_EVENTS defined
> > 
> > I've been workingon adding a few tracepoints to the network stack, and in so
> > doing it was convienient for me to add a second include file to
> > net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
> > CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> > duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
> > define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> > run through define_trace.h (which is included from skb.h, included in
> > net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> > to start a new cycle with the next header.
> > 
> > The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> > end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> > This places us back in the proper state to define a new set of tracepoints in a
> > subsequent header file.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> 
> By itself, this patch breaks the build (for me). I take it that you
> converted the napi.h file to use TRACE_EVENT. Currently it uses a
> DECLARE_TRACE, which will break with this patch. We need a patch that
> removes that DECLARE_TRACE before we can apply this patch.
> 
Yes, I did that conversion, sorry, wasn't thinking about how that might affect
the build.  Given that converting to a TRACE_EVENT shouldn't be noticible to any
of the users of that tracepoint (it can still be hooked in an identical
fashion), I'd vote for just doing a monolitic patch.  please find that below.

Regards
Neil



Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
be included with CREATE_TRACE_EVENTS defined

I've been workingon adding a few tracepoints to the network stack, and in so
doing it was convienient for me to add a second include file to
net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
run through define_trace.h (which is included from skb.h, included in
net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
to start a new cycle with the next header.

The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
This places us back in the proper state to define a new set of tracepoints in a
subsequent header file.


Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT.  This
is done so that the TRACE_EVENT fix doesn't break the build, and because its
rather pointless I think given the current tracing infrastructure to not have
napi_poll be an independent event.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
trace/define_trace.h |    1 +
trace/events/napi.h  |   25 ++++++++++++++++++++++---
3 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 2aac8a8..311abbf 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -58,28 +58,6 @@ struct tracepoint {
 		rcu_read_unlock_sched_notrace();			\
 	} while (0)
 
-/*
- * Make sure the alignment of the structure in the __tracepoints section will
- * not add unwanted padding between the beginning of the section and the
- * structure. Force alignment to the same alignment as the section start.
- */
-#define DECLARE_TRACE(name, proto, args)				\
-	extern struct tracepoint __tracepoint_##name;			\
-	static inline void trace_##name(proto)				\
-	{								\
-		if (unlikely(__tracepoint_##name.state))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(proto), TP_ARGS(args));	\
-	}								\
-	static inline int register_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_register(#name, (void *)probe);	\
-	}								\
-	static inline int unregister_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_unregister(#name, (void *)probe);\
-	}
-
 
 #define DEFINE_TRACE_FN(name, reg, unreg)				\
 	static const char __tpstrtab_##name[]				\
@@ -173,6 +151,29 @@ static inline void tracepoint_synchronize_unregister(void)
  *  trace event headers under one "CREATE_TRACE_POINTS" the first include
  *  will override the TRACE_EVENT and break the second include.
  */
+#if defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
+/*
+ * Make sure the alignment of the structure in the __tracepoints section will
+ * not add unwanted padding between the beginning of the section and the
+ * structure. Force alignment to the same alignment as the section start.
+ */
+#define DECLARE_TRACE(name, proto, args)                                \
+	extern struct tracepoint __tracepoint_##name;                   \
+	static inline void trace_##name(proto)                          \
+	{                                                               \
+		if (unlikely(__tracepoint_##name.state))                \
+			__DO_TRACE(&__tracepoint_##name,                \
+				TP_PROTO(proto), TP_ARGS(args));        \
+	}                                                               \
+	static inline int register_trace_##name(void (*probe)(proto))   \
+	{                                                               \
+		return tracepoint_probe_register(#name, (void *)probe); \
+	}                                                               \
+	static inline int unregister_trace_##name(void (*probe)(proto)) \
+	{                                                               \
+		return tracepoint_probe_unregister(#name, (void *)probe);\
+	}
+#endif
 
 #ifndef TRACE_EVENT
 /*
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 2a4b3bf..a3e0095 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -64,6 +64,7 @@
 #undef TRACE_EVENT
 #undef TRACE_EVENT_FN
 #undef TRACE_HEADER_MULTI_READ
+#undef DECLARE_TRACE
 
 /* Only undef what we defined in this file */
 #ifdef UNDEF_TRACE_INCLUDE_FILE
diff --git a/include/trace/events/napi.h b/include/trace/events/napi.h
index a8989c4..25a1709 100644
--- a/include/trace/events/napi.h
+++ b/include/trace/events/napi.h
@@ -1,11 +1,30 @@
-#ifndef _TRACE_NAPI_H_
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM napi
+
+#if !defined(_TRACE_NAPI_H_) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_NAPI_H_
 
 #include <linux/netdevice.h>
 #include <linux/tracepoint.h>
 
-DECLARE_TRACE(napi_poll,
+TRACE_EVENT(napi_poll,
+
 	TP_PROTO(struct napi_struct *napi),
-	TP_ARGS(napi));
+
+	TP_ARGS(napi),
+
+	TP_STRUCT__entry(
+		__field(        struct napi_struct *,	napi)
+        ),
+
+        TP_fast_assign(
+                __entry->napi = napi;
+        ),
+
+        TP_printk("napi poll on napi struct %p for device %s",
+		__entry->napi, __entry->napi->dev->name)
+);
 
 #endif
+
+#include <trace/define_trace.h>

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Mon, 2009-12-07 at 15:18 -0500, Neil Horman wrote:
> On Mon, Dec 07, 2009 at 03:04:42PM -0500, Steven Rostedt wrote:
> > On Thu, 2009-12-03 at 14:35 -0500, Neil Horman wrote:
> > 
> > > Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> > > be included with CREATE_TRACE_EVENTS defined
> > > 
> > > I've been workingon adding a few tracepoints to the network stack, and in so
> > > doing it was convienient for me to add a second include file to
> > > net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
> > > CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> > > duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
> > > define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> > > run through define_trace.h (which is included from skb.h, included in
> > > net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> > > to start a new cycle with the next header.
> > > 
> > > The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> > > end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> > > This places us back in the proper state to define a new set of tracepoints in a
> > > subsequent header file.
> > > 
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > 
> > 
> > By itself, this patch breaks the build (for me). I take it that you
> > converted the napi.h file to use TRACE_EVENT. Currently it uses a
> > DECLARE_TRACE, which will break with this patch. We need a patch that
> > removes that DECLARE_TRACE before we can apply this patch.
> > 
> Yes, I did that conversion, sorry, wasn't thinking about how that might affect
> the build.  Given that converting to a TRACE_EVENT shouldn't be noticible to any
> of the users of that tracepoint (it can still be hooked in an identical
> fashion), I'd vote for just doing a monolitic patch.  please find that below.
> 
> Regards
> Neil
> 
> 
> 
> Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> be included with CREATE_TRACE_EVENTS defined
> 
> I've been workingon adding a few tracepoints to the network stack, and in so
> doing it was convienient for me to add a second include file to
> net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
> CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
> define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> run through define_trace.h (which is included from skb.h, included in
> net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> to start a new cycle with the next header.
> 
> The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> This places us back in the proper state to define a new set of tracepoints in a
> subsequent header file.
> 
> 
> Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT.  This
> is done so that the TRACE_EVENT fix doesn't break the build, and because its
> rather pointless I think given the current tracing infrastructure to not have
> napi_poll be an independent event.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Thanks, I'll give it a test. But because this now touches networking
code, I need an Acked-by from David Miller before I can pull it in.

-- Steve



> 
> 
> linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
> trace/define_trace.h |    1 +
> trace/events/napi.h  |   25 ++++++++++++++++++++++---
> 3 files changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 2aac8a8..311abbf 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -58,28 +58,6 @@ struct tracepoint {
>  		rcu_read_unlock_sched_notrace();			\
>  	} while (0)
>  
> -/*
> - * Make sure the alignment of the structure in the __tracepoints section will
> - * not add unwanted padding between the beginning of the section and the
> - * structure. Force alignment to the same alignment as the section start.
> - */
> -#define DECLARE_TRACE(name, proto, args)				\
> -	extern struct tracepoint __tracepoint_##name;			\
> -	static inline void trace_##name(proto)				\
> -	{								\
> -		if (unlikely(__tracepoint_##name.state))		\
> -			__DO_TRACE(&__tracepoint_##name,		\
> -				TP_PROTO(proto), TP_ARGS(args));	\
> -	}								\
> -	static inline int register_trace_##name(void (*probe)(proto))	\
> -	{								\
> -		return tracepoint_probe_register(#name, (void *)probe);	\
> -	}								\
> -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> -	{								\
> -		return tracepoint_probe_unregister(#name, (void *)probe);\
> -	}
> -
>  
>  #define DEFINE_TRACE_FN(name, reg, unreg)				\
>  	static const char __tpstrtab_##name[]				\
> @@ -173,6 +151,29 @@ static inline void tracepoint_synchronize_unregister(void)
>   *  trace event headers under one "CREATE_TRACE_POINTS" the first include
>   *  will override the TRACE_EVENT and break the second include.
>   */
> +#if defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
> +/*
> + * Make sure the alignment of the structure in the __tracepoints section will
> + * not add unwanted padding between the beginning of the section and the
> + * structure. Force alignment to the same alignment as the section start.
> + */
> +#define DECLARE_TRACE(name, proto, args)                                \
> +	extern struct tracepoint __tracepoint_##name;                   \
> +	static inline void trace_##name(proto)                          \
> +	{                                                               \
> +		if (unlikely(__tracepoint_##name.state))                \
> +			__DO_TRACE(&__tracepoint_##name,                \
> +				TP_PROTO(proto), TP_ARGS(args));        \
> +	}                                                               \
> +	static inline int register_trace_##name(void (*probe)(proto))   \
> +	{                                                               \
> +		return tracepoint_probe_register(#name, (void *)probe); \
> +	}                                                               \
> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> +	{                                                               \
> +		return tracepoint_probe_unregister(#name, (void *)probe);\
> +	}
> +#endif
>  
>  #ifndef TRACE_EVENT
>  /*
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index 2a4b3bf..a3e0095 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -64,6 +64,7 @@
>  #undef TRACE_EVENT
>  #undef TRACE_EVENT_FN
>  #undef TRACE_HEADER_MULTI_READ
> +#undef DECLARE_TRACE
>  
>  /* Only undef what we defined in this file */
>  #ifdef UNDEF_TRACE_INCLUDE_FILE
> diff --git a/include/trace/events/napi.h b/include/trace/events/napi.h
> index a8989c4..25a1709 100644
> --- a/include/trace/events/napi.h
> +++ b/include/trace/events/napi.h
> @@ -1,11 +1,30 @@
> -#ifndef _TRACE_NAPI_H_
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM napi
> +
> +#if !defined(_TRACE_NAPI_H_) || defined(TRACE_HEADER_MULTI_READ)
>  #define _TRACE_NAPI_H_
>  
>  #include <linux/netdevice.h>
>  #include <linux/tracepoint.h>
>  
> -DECLARE_TRACE(napi_poll,
> +TRACE_EVENT(napi_poll,
> +
>  	TP_PROTO(struct napi_struct *napi),
> -	TP_ARGS(napi));
> +
> +	TP_ARGS(napi),
> +
> +	TP_STRUCT__entry(
> +		__field(        struct napi_struct *,	napi)
> +        ),
> +
> +        TP_fast_assign(
> +                __entry->napi = napi;
> +        ),
> +
> +        TP_printk("napi poll on napi struct %p for device %s",
> +		__entry->napi, __entry->napi->dev->name)
> +);
>  
>  #endif
> +
> +#include <trace/define_trace.h>

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Mon, 2009-12-07 at 15:47 -0500, Steven Rostedt wrote:

> > 
> > Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT.  This
> > is done so that the TRACE_EVENT fix doesn't break the build, and because its
> > rather pointless I think given the current tracing infrastructure to not have
> > napi_poll be an independent event.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Thanks, I'll give it a test. But because this now touches networking
> code, I need an Acked-by from David Miller before I can pull it in.
> 
> -- Steve
> 
> 
> 
> > 
> > 
> > linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
> > trace/define_trace.h |    1 +
> > trace/events/napi.h  |   25 ++++++++++++++++++++++---

Hmm, This really isn't networking code. But still, a patch that changes
the way a networking trace point works, I would rather have an ack.

Thanks,

-- Steve

> > 3 files changed, 46 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 2aac8a8..311abbf 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -58,28 +58,6 @@ struct tracepoint {
> >  		rcu_read_unlock_sched_notrace();			\
> >  	} while (0)
> >  

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Mon, Dec 07, 2009 at 03:49:26PM -0500, Steven Rostedt wrote:
> On Mon, 2009-12-07 at 15:47 -0500, Steven Rostedt wrote:
> 
> > > 
> > > Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT.  This
> > > is done so that the TRACE_EVENT fix doesn't break the build, and because its
> > > rather pointless I think given the current tracing infrastructure to not have
> > > napi_poll be an independent event.
> > > 
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > Thanks, I'll give it a test. But because this now touches networking
> > code, I need an Acked-by from David Miller before I can pull it in.
> > 
> > -- Steve
> > 
> > 
> > 
> > > 
> > > 
> > > linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
> > > trace/define_trace.h |    1 +
> > > trace/events/napi.h  |   25 ++++++++++++++++++++++---
> 
> Hmm, This really isn't networking code. But still, a patch that changes
> the way a networking trace point works, I would rather have an ack.
> 
> Thanks,
> 
Copy that, I agree an Ack from dave is called for here, but Just FYi, theres
only one user of this tracepoint currently in the tree (the drop monitor) and
this change has no bearing on it, as you can still hook the tracepoint with the
same register_trace_napi_poll call

Neil

> -- Steve
> 
> > > 3 files changed, 46 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > index 2aac8a8..311abbf 100644
> > > --- a/include/linux/tracepoint.h
> > > +++ b/include/linux/tracepoint.h
> > > @@ -58,28 +58,6 @@ struct tracepoint {
> > >  		rcu_read_unlock_sched_notrace();			\
> > >  	} while (0)
> > >  
> 
> 
> 

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Mon, Dec 07, 2009 at 03:53:41PM -0500, Neil Horman wrote:
> On Mon, Dec 07, 2009 at 03:49:26PM -0500, Steven Rostedt wrote:
> > On Mon, 2009-12-07 at 15:47 -0500, Steven Rostedt wrote:
> > 
> > > > 
> > > > Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT.  This
> > > > is done so that the TRACE_EVENT fix doesn't break the build, and because its
> > > > rather pointless I think given the current tracing infrastructure to not have
> > > > napi_poll be an independent event.
> > > > 
> > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > 
> > > Thanks, I'll give it a test. But because this now touches networking
> > > code, I need an Acked-by from David Miller before I can pull it in.
> > > 
> > > -- Steve
> > > 
> > > 
> > > 
> > > > 
> > > > 
> > > > linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
> > > > trace/define_trace.h |    1 +
> > > > trace/events/napi.h  |   25 ++++++++++++++++++++++---
> > 
> > Hmm, This really isn't networking code. But still, a patch that changes
> > the way a networking trace point works, I would rather have an ack.
> > 
> > Thanks,
> > 
> Copy that, I agree an Ack from dave is called for here, but Just FYi, theres
> only one user of this tracepoint currently in the tree (the drop monitor) and
> this change has no bearing on it, as you can still hook the tracepoint with the
> same register_trace_napi_poll call
> 
> Neil
> 
Ping, Dave, any thoughts here?
Regards
Neil

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Mon, Dec 14, 2009 at 02:50:58PM -0500, Neil Horman wrote:
> On Mon, Dec 07, 2009 at 03:53:41PM -0500, Neil Horman wrote:
> > On Mon, Dec 07, 2009 at 03:49:26PM -0500, Steven Rostedt wrote:
> > > On Mon, 2009-12-07 at 15:47 -0500, Steven Rostedt wrote:
> > > 
> > > > > 
> > > > > Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT.  This
> > > > > is done so that the TRACE_EVENT fix doesn't break the build, and because its
> > > > > rather pointless I think given the current tracing infrastructure to not have
> > > > > napi_poll be an independent event.
> > > > > 
> > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > 
> > > > Thanks, I'll give it a test. But because this now touches networking
> > > > code, I need an Acked-by from David Miller before I can pull it in.
> > > > 
> > > > -- Steve
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
> > > > > trace/define_trace.h |    1 +
> > > > > trace/events/napi.h  |   25 ++++++++++++++++++++++---
> > > 
> > > Hmm, This really isn't networking code. But still, a patch that changes
> > > the way a networking trace point works, I would rather have an ack.
> > > 
> > > Thanks,
> > > 
> > Copy that, I agree an Ack from dave is called for here, but Just FYi, theres
> > only one user of this tracepoint currently in the tree (the drop monitor) and
> > this change has no bearing on it, as you can still hook the tracepoint with the
> > same register_trace_napi_poll call
> > 
> > Neil
> > 
> Ping, Dave, any thoughts here?
> Regards
> Neil


So because I have no patience, I asked Dave M. about this on irc last night, and
he asked that I append his ACK to it, so here's the patch again, with his
blessing by proxy :)


Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
be included with CREATE_TRACE_EVENTS defined

I've been workingon adding a few tracepoints to the network stack, and in so
doing it was convienient for me to add a second include file to
net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
run through define_trace.h (which is included from skb.h, included in
net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
to start a new cycle with the next header.

The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
This places us back in the proper state to define a new set of tracepoints in a
subsequent header file.


Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT.  This
is done so that the TRACE_EVENT fix doesn't break the build, and because its
rather pointless I think given the current tracing infrastructure to not have
napi_poll be an independent event.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: David S. Miller  <davem@davemloft.net>

linux/tracepoint.h   |   45 +++++++++++++++++++++++----------------------
trace/define_trace.h |    1 +
trace/events/napi.h  |   25 ++++++++++++++++++++++---
3 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 2aac8a8..311abbf 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -58,28 +58,6 @@ struct tracepoint {
 		rcu_read_unlock_sched_notrace();			\
 	} while (0)
 
-/*
- * Make sure the alignment of the structure in the __tracepoints section will
- * not add unwanted padding between the beginning of the section and the
- * structure. Force alignment to the same alignment as the section start.
- */
-#define DECLARE_TRACE(name, proto, args)				\
-	extern struct tracepoint __tracepoint_##name;			\
-	static inline void trace_##name(proto)				\
-	{								\
-		if (unlikely(__tracepoint_##name.state))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(proto), TP_ARGS(args));	\
-	}								\
-	static inline int register_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_register(#name, (void *)probe);	\
-	}								\
-	static inline int unregister_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_unregister(#name, (void *)probe);\
-	}
-
 
 #define DEFINE_TRACE_FN(name, reg, unreg)				\
 	static const char __tpstrtab_##name[]				\
@@ -173,6 +151,29 @@ static inline void tracepoint_synchronize_unregister(void)
  *  trace event headers under one "CREATE_TRACE_POINTS" the first include
  *  will override the TRACE_EVENT and break the second include.
  */
+#if defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
+/*
+ * Make sure the alignment of the structure in the __tracepoints section will
+ * not add unwanted padding between the beginning of the section and the
+ * structure. Force alignment to the same alignment as the section start.
+ */
+#define DECLARE_TRACE(name, proto, args)                                \
+	extern struct tracepoint __tracepoint_##name;                   \
+	static inline void trace_##name(proto)                          \
+	{                                                               \
+		if (unlikely(__tracepoint_##name.state))                \
+			__DO_TRACE(&__tracepoint_##name,                \
+				TP_PROTO(proto), TP_ARGS(args));        \
+	}                                                               \
+	static inline int register_trace_##name(void (*probe)(proto))   \
+	{                                                               \
+		return tracepoint_probe_register(#name, (void *)probe); \
+	}                                                               \
+	static inline int unregister_trace_##name(void (*probe)(proto)) \
+	{                                                               \
+		return tracepoint_probe_unregister(#name, (void *)probe);\
+	}
+#endif
 
 #ifndef TRACE_EVENT
 /*
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 2a4b3bf..a3e0095 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -64,6 +64,7 @@
 #undef TRACE_EVENT
 #undef TRACE_EVENT_FN
 #undef TRACE_HEADER_MULTI_READ
+#undef DECLARE_TRACE
 
 /* Only undef what we defined in this file */
 #ifdef UNDEF_TRACE_INCLUDE_FILE
diff --git a/include/trace/events/napi.h b/include/trace/events/napi.h
index a8989c4..25a1709 100644
--- a/include/trace/events/napi.h
+++ b/include/trace/events/napi.h
@@ -1,11 +1,30 @@
-#ifndef _TRACE_NAPI_H_
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM napi
+
+#if !defined(_TRACE_NAPI_H_) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_NAPI_H_
 
 #include <linux/netdevice.h>
 #include <linux/tracepoint.h>
 
-DECLARE_TRACE(napi_poll,
+TRACE_EVENT(napi_poll,
+
 	TP_PROTO(struct napi_struct *napi),
-	TP_ARGS(napi));
+
+	TP_ARGS(napi),
+
+	TP_STRUCT__entry(
+		__field(        struct napi_struct *,	napi)
+        ),
+
+        TP_fast_assign(
+                __entry->napi = napi;
+        ),
+
+        TP_printk("napi poll on napi struct %p for device %s",
+		__entry->napi, __entry->napi->dev->name)
+);
 
 #endif
+
+#include <trace/define_trace.h>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Tue, 2009-12-15 at 11:08 -0500, Neil Horman wrote:

Hi Neil,

I was playing with this, and I got really nasty errors in the trace
parsing tools. Then I noticed why:

> -DECLARE_TRACE(napi_poll,
> +TRACE_EVENT(napi_poll,
> +
>  	TP_PROTO(struct napi_struct *napi),
> -	TP_ARGS(napi));
> +
> +	TP_ARGS(napi),
> +
> +	TP_STRUCT__entry(
> +		__field(        struct napi_struct *,	napi)
> +        ),
> +
> +        TP_fast_assign(
> +                __entry->napi = napi;
> +        ),
> +
> +        TP_printk("napi poll on napi struct %p for device %s",
> +		__entry->napi, __entry->napi->dev->name)

You can't trust this! That "__entry" happens to reside on the ring
buffer. If for some reason the device goes away, this blows up when you
read the trace.

If you need to save the name of the device, then store it in the ring
buffer. You can do it with a dynamic array:

	TP_STRUCT__entry(
		__field(	struct napi_struct *, napi)
		__string(	dev_name, napi->dev->name)
	),

	TP_fast_assign(
		__entry->napi = napi;
		__assign_str(dev_name, napi->dev->name);
	),

	TP_printk("napi poll on napi struct %p for device %s",
		__entry->napi, __get_string(dev_name))

-- Steve

> +);
>  
>  #endif

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Tue, Dec 15, 2009 at 11:13:49PM -0500, Steven Rostedt wrote:
> On Tue, 2009-12-15 at 11:08 -0500, Neil Horman wrote:
> 
> Hi Neil,
> 
> I was playing with this, and I got really nasty errors in the trace
> parsing tools. Then I noticed why:
> 
> > -DECLARE_TRACE(napi_poll,
> > +TRACE_EVENT(napi_poll,
> > +
> >  	TP_PROTO(struct napi_struct *napi),
> > -	TP_ARGS(napi));
> > +
> > +	TP_ARGS(napi),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(        struct napi_struct *,	napi)
> > +        ),
> > +
> > +        TP_fast_assign(
> > +                __entry->napi = napi;
> > +        ),
> > +
> > +        TP_printk("napi poll on napi struct %p for device %s",
> > +		__entry->napi, __entry->napi->dev->name)
> 
> You can't trust this! That "__entry" happens to reside on the ring
> buffer. If for some reason the device goes away, this blows up when you
> read the trace.
> 
> If you need to save the name of the device, then store it in the ring
> buffer. You can do it with a dynamic array:
> 
> 	TP_STRUCT__entry(
> 		__field(	struct napi_struct *, napi)
> 		__string(	dev_name, napi->dev->name)
> 	),
> 
> 	TP_fast_assign(
> 		__entry->napi = napi;
> 		__assign_str(dev_name, napi->dev->name);
> 	),
> 
> 	TP_printk("napi poll on napi struct %p for device %s",
> 		__entry->napi, __get_string(dev_name))
> 
> -- Steve
> 
> > +);
> >  
> >  #endif
> 
> 
> 
Ok, thanks, I'll resubmit shortly.
Neil

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

Steven-
	Here you go, new version of the patch, with the napi trace event fixed
up so that we don't errneously access a net_device we don't hold a ref on.  I've
tested it here and its working well

Regards
Neil


Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
be included with CREATE_TRACE_EVENTS defined

I've been workingon adding a few tracepoints to the network stack, and in so
doing it was convienient for me to add a second include file to
net/core/net-traces.c with TRACE_EVENTS defined in them.  net-traces.c defines
CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
duplicate definitions of __tpstrtab_<name>.  I tracked down the bug to find that
define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
run through define_trace.h (which is included from skb.h, included in
net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
to start a new cycle with the next header.

The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
This places us back in the proper state to define a new set of tracepoints in a
subsequent header file.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index f59604e..f350688 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -58,28 +58,6 @@ struct tracepoint {
 		rcu_read_unlock_sched_notrace();			\
 	} while (0)
 
-/*
- * Make sure the alignment of the structure in the __tracepoints section will
- * not add unwanted padding between the beginning of the section and the
- * structure. Force alignment to the same alignment as the section start.
- */
-#define DECLARE_TRACE(name, proto, args)				\
-	extern struct tracepoint __tracepoint_##name;			\
-	static inline void trace_##name(proto)				\
-	{								\
-		if (unlikely(__tracepoint_##name.state))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(proto), TP_ARGS(args));	\
-	}								\
-	static inline int register_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_register(#name, (void *)probe);	\
-	}								\
-	static inline int unregister_trace_##name(void (*probe)(proto))	\
-	{								\
-		return tracepoint_probe_unregister(#name, (void *)probe);\
-	}
-
 
 #define DEFINE_TRACE_FN(name, reg, unreg)				\
 	static const char __tpstrtab_##name[]				\
@@ -173,6 +151,29 @@ static inline void tracepoint_synchronize_unregister(void)
  *  trace event headers under one "CREATE_TRACE_POINTS" the first include
  *  will override the TRACE_EVENT and break the second include.
  */
+#if defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
+/*
+ * Make sure the alignment of the structure in the __tracepoints section will
+ * not add unwanted padding between the beginning of the section and the
+ * structure. Force alignment to the same alignment as the section start.
+ */
+#define DECLARE_TRACE(name, proto, args)                                \
+	extern struct tracepoint __tracepoint_##name;                   \
+	static inline void trace_##name(proto)                          \
+	{                                                               \
+		if (unlikely(__tracepoint_##name.state))                \
+			__DO_TRACE(&__tracepoint_##name,                \
+				TP_PROTO(proto), TP_ARGS(args));        \
+	}                                                               \
+	static inline int register_trace_##name(void (*probe)(proto))   \
+	{                                                               \
+		return tracepoint_probe_register(#name, (void *)probe); \
+	}                                                               \
+	static inline int unregister_trace_##name(void (*probe)(proto)) \
+	{                                                               \
+		return tracepoint_probe_unregister(#name, (void *)probe);\
+	}
+#endif
 
 #ifndef TRACE_EVENT
 /*
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 5acfb1e..3e8cf03 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -75,6 +75,7 @@
 #undef DEFINE_EVENT
 #undef DEFINE_EVENT_PRINT
 #undef TRACE_HEADER_MULTI_READ
+#undef DECLARE_TRACE
 
 /* Only undef what we defined in this file */
 #ifdef UNDEF_TRACE_INCLUDE_FILE
diff --git a/include/trace/events/napi.h b/include/trace/events/napi.h
index a8989c4..6afe7a8 100644
--- a/include/trace/events/napi.h
+++ b/include/trace/events/napi.h
@@ -1,11 +1,33 @@
-#ifndef _TRACE_NAPI_H_
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM napi
+
+#if !defined(_TRACE_NAPI_H_) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_NAPI_H_
 
 #include <linux/netdevice.h>
 #include <linux/tracepoint.h>
+#include <linux/ftrace.h>
+
+TRACE_EVENT(napi_poll,
 
-DECLARE_TRACE(napi_poll,
 	TP_PROTO(struct napi_struct *napi),
-	TP_ARGS(napi));
+
+	TP_ARGS(napi),
+
+	TP_STRUCT__entry(
+		__field(        struct napi_struct *,	napi)
+		__string(	dev_name, napi->dev->name)
+        ),
+
+        TP_fast_assign(
+                __entry->napi = napi;
+		__assign_str(dev_name, napi->dev->name);
+        ),
+
+        TP_printk("napi poll on napi struct %p for device %s",
+		__entry->napi, __get_str(dev_name))
+);
 
 #endif
+
+#include <trace/define_trace.h>

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Wed, 2009-12-16 at 11:04 -0500, Neil Horman wrote:


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

Ug, I'm still getting panics because of this. I found out that it's
because napi->dev can be NULL.

Here's what I did to solve it:


>  #include <linux/netdevice.h>
>  #include <linux/tracepoint.h>
> +#include <linux/ftrace.h>
> +

#define NO_DEV		"(no device)"

> +TRACE_EVENT(napi_poll,
>  
> -DECLARE_TRACE(napi_poll,
>  	TP_PROTO(struct napi_struct *napi),
> -	TP_ARGS(napi));
> +
> +	TP_ARGS(napi),
> +
> +	TP_STRUCT__entry(
> +		__field(        struct napi_struct *,	napi)
> +		__string(	dev_name, napi->dev->name)

		__string(	dev_name, napi->dev ? napi->dev->name, NO_DEV )

> +        ),
> +
> +        TP_fast_assign(
> +                __entry->napi = napi;
> +		__assign_str(dev_name, napi->dev->name);

		__assign_str(dev_name, napi->dev ? napi->dev->name, NO_DEV);

> +        ),
> +
> +        TP_printk("napi poll on napi struct %p for device %s",
> +		__entry->napi, __get_str(dev_name))
> +);

#undef NO_DEV

-- Steve


>  
>  #endif
> +
> +#include <trace/define_trace.h>

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Tue, Dec 22, 2009 at 01:03:59PM -0500, Steven Rostedt wrote:
> On Wed, 2009-12-16 at 11:04 -0500, Neil Horman wrote:
> 
> 
> > +#if !defined(_TRACE_NAPI_H_) || defined(TRACE_HEADER_MULTI_READ)
> >  #define _TRACE_NAPI_H_
> >  
> 
> Ug, I'm still getting panics because of this. I found out that it's
> because napi->dev can be NULL.
> 
> Here's what I did to solve it:
Gahhh, you're right, We had to do the same thing in net-next's commit
f2798eb4e01b095f273f4bf40f511c9d69c0e1da.  If you queue to the backlog, the
backlog napi instance has no net device and so its null.  This is exactly the
right fix.

Thanks!
Neil

> 

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Steven Rostedt]

On Wed, 2009-12-23 at 07:02 -0500, Neil Horman wrote:
> On Tue, Dec 22, 2009 at 01:03:59PM -0500, Steven Rostedt wrote:
> > On Wed, 2009-12-16 at 11:04 -0500, Neil Horman wrote:
> > 
> > 
> > > +#if !defined(_TRACE_NAPI_H_) || defined(TRACE_HEADER_MULTI_READ)
> > >  #define _TRACE_NAPI_H_
> > >  
> > 
> > Ug, I'm still getting panics because of this. I found out that it's
> > because napi->dev can be NULL.
> > 
> > Here's what I did to solve it:
> Gahhh, you're right, We had to do the same thing in net-next's commit
> f2798eb4e01b095f273f4bf40f511c9d69c0e1da.  If you queue to the backlog, the
> backlog napi instance has no net device and so its null.  This is exactly the
> right fix.

OK, I'll update the patch and apply it.

But this doesn't need to go into 33 correct. We can let it brew for 34?

-- Steve

Re: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS

[Neil Horman]

On Wed, Dec 23, 2009 at 08:58:33AM -0500, Steven Rostedt wrote:
> On Wed, 2009-12-23 at 07:02 -0500, Neil Horman wrote:
> > On Tue, Dec 22, 2009 at 01:03:59PM -0500, Steven Rostedt wrote:
> > > On Wed, 2009-12-16 at 11:04 -0500, Neil Horman wrote:
> > > 
> > > 
> > > > +#if !defined(_TRACE_NAPI_H_) || defined(TRACE_HEADER_MULTI_READ)
> > > >  #define _TRACE_NAPI_H_
> > > >  
> > > 
> > > Ug, I'm still getting panics because of this. I found out that it's
> > > because napi->dev can be NULL.
> > > 
> > > Here's what I did to solve it:
> > Gahhh, you're right, We had to do the same thing in net-next's commit
> > f2798eb4e01b095f273f4bf40f511c9d69c0e1da.  If you queue to the backlog, the
> > backlog napi instance has no net device and so its null.  This is exactly the
> > right fix.
> 
> OK, I'll update the patch and apply it.
> 
> But this doesn't need to go into 33 correct. We can let it brew for 34?
> 
Yeah, the thing I was depending on it for got NAK'ed anyway, so its just a
straight bugfix + a cleanup of the napi_poll tracepoint.  It can definately wait
until 34.

Thanks!
Neil

> -- Steve
> 
> 
>