Skip to contentSkip to conversation

trace_evetns: fix napi's tracepoint

PATCH In review Xiao Guangrong 5 participants 14 messages Aug 25, 2009 – Aug 26, 2009

Subsystems (3)
Networking Network drivers Tracing
lore
Share & tools
lore

Xiao Guangrong

author
#0 lore raw

PATCH 1/2 trace_evetns: fix napi's tracepoint

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/trace/define_trace.h |    4 ----
 net/core/net-traces.c        |    4 +++-
 2 files changed, 3 insertions(+), 5 deletions(-)
include/trace/define_trace.h +0 -4
index f7a7ae1..76e93bf 100644
@@ -26,10 +26,6 @@
 #define TRACE_EVENT(name, proto, args, tstruct, assign, print)	\
 	DEFINE_TRACE(name)
 
-#undef DECLARE_TRACE
-#define DECLARE_TRACE(name, proto, args)	\
-	DEFINE_TRACE(name)
-
 #undef TRACE_INCLUDE
 #undef __TRACE_INCLUDE
net/core/net-traces.c +3 -1
index f1e982c..7eee0b7 100644
@@ -22,10 +22,12 @@
 
 #include <asm/unaligned.h>
 #include <asm/bitops.h>
+#include <linux/tracepoint.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/skb.h>
-#include <trace/events/napi.h>
+
+DEFINE_TRACE(napi_poll);
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(kfree_skb);
Signature · 1 line
1.6.1.2
author
#1 lore raw

PATCH 1/2 trace_events: fix napi's tracepoint

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/trace/define_trace.h |    4 ----
 net/core/net-traces.c        |    4 +++-
 2 files changed, 3 insertions(+), 5 deletions(-)
include/trace/define_trace.h +0 -4
index f7a7ae1..76e93bf 100644
@@ -26,10 +26,6 @@
 #define TRACE_EVENT(name, proto, args, tstruct, assign, print)	\
 	DEFINE_TRACE(name)
 
-#undef DECLARE_TRACE
-#define DECLARE_TRACE(name, proto, args)	\
-	DEFINE_TRACE(name)
-
 #undef TRACE_INCLUDE
 #undef __TRACE_INCLUDE
net/core/net-traces.c +3 -1
index f1e982c..42bda72 100644
@@ -19,13 +19,15 @@
 #include <linux/workqueue.h>
 #include <linux/netlink.h>
 #include <linux/net_dropmon.h>
+#include <linux/tracepoint.h>
 
 #include <asm/unaligned.h>
 #include <asm/bitops.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/skb.h>
-#include <trace/events/napi.h>
+
+DEFINE_TRACE(napi_poll);
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(kfree_skb);
Signature · 1 line
1.6.1.2
#2 lore raw

Re: [PATCH 1/2] trace_events: fix napi's tracepoint

Quoted 13 lines · Xiao Guangrong

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Quoted 5 lines
Signed-off-by Xiao Guangrong <[email protected]>
---
 include/trace/define_trace.h |    4 ----
 net/core/net-traces.c        |    4 +++-
 2 files changed, 3 insertions(+), 5 deletions(-)

This will collide with tracing bits in the networking tree. The skb-tracing plugin there should be turned into proper TRACE_EVENT() tracepoints.

Neil was away for some time but i think soon we should see some movement here.

Ingo

#3 lore raw

Re: [PATCH 1/2] trace_events: fix napi's tracepoint

Quoted 22 lines · Ingo Molnar
Quoted 13 lines · Xiao Guangrong

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Quoted 5 lines
Signed-off-by Xiao Guangrong <[email protected]>
---
 include/trace/define_trace.h |    4 ----
 net/core/net-traces.c        |    4 +++-
 2 files changed, 3 insertions(+), 5 deletions(-)
Quoted 9 lines

This will collide with tracing bits in the networking tree. The skb-tracing plugin there should be turned into proper TRACE_EVENT() tracepoints.

Neil was away for some time but i think soon we should see some movement here.

Ingo

Thank you Ingo, yes, I'm back from the beach now and will look at this shortly. I concur, we should just convert the napi_poll tracepoint to be defined via the TRACE_EVENT macro, same as the kfree_skb tracepoint. Xaio would you like to take care of that, or shall I?

Best
Neil

author
#4 lore raw

Re: [PATCH 1/2] trace_events: fix napi's tracepoint

Quoted 10 lines · Neil Horman
Quoted 9 lines · Ingo Molnar

* Xiao Guangrong <[email protected]> wrote:

This will collide with tracing bits in the networking tree. The skb-tracing plugin there should be turned into proper TRACE_EVENT() tracepoints.

Neil was away for some time but i think soon we should see some movement here.

Quoted 8 lines
Quoted 2 lines

Ingo

Thank you Ingo, yes, I'm back from the beach now and will look at this shortly. I concur, we should just convert the napi_poll tracepoint to be defined via the TRACE_EVENT macro, same as the kfree_skb tracepoint. Xaio would you like to take care of that, or shall I?

Hi Neil,

I'm glad to do it.

Actually, we should fix the include file dependencies first (the second patch in this patchset), else we can't include two TRACE_EVENT head files in net-traces.c

Steve will pull this patchset, so, I'll convert it after this patchset merged.

author
#5 lore raw

Re: [PATCH 1/2] trace_events: fix napi's tracepoint

In reply to Xiao Guangrong
Quoted 7 lines · Xiao Guangrong

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

Quoted 7 lines

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Signed-off-by Xiao Guangrong <[email protected]>

Hi Steven,

I'm sorry, please pull this patch too, because "[PATCH 7/8] tracing/events: fix the include file dependencies" is based on this patch, else will occur building error.

Thanks,
Xiao

author
#6 lore raw

Re: [PATCH 1/2] trace_events: fix napi's tracepoint

Quoted 15 lines · Xiao Guangrong
Quoted 13 lines · Xiao Guangrong

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Quoted 8 lines
Quoted 1 line
Signed-off-by Xiao Guangrong <[email protected]>

Hi Steven,

I'm sorry, please pull this patch too, because "[PATCH 7/8] tracing/events: fix the include file dependencies" is based on this patch, else will occur building error.

Sorry again, I say the wrong words, it not has building error, just not complete fix the bug which I mention it in the changelog of "[PATCH 7/8] tracing/events: fix the include file dependencies", that is we can't include more TRACE_EVENT head file in .c file all the same, like below:

Both define TRACE_EVENT in trace_a.h and trace_b.h, if we include those in .c file, like this:

#define CREATE_TRACE_POINTS
include <trace/events/trace_a.h> // re-define DECLARE_TRACE

include <trace/events/trace_b.h> // use the DECLARE_TRACE definition

				  // that re-define by trace_a.h

Thanks,
Xiao

maintainer
#7 lore raw

Re: [PATCH 1/2] trace_events: fix napi's tracepoint

Quoted 33 lines · Xiao Guangrong
Quoted 15 lines · Xiao Guangrong
Quoted 13 lines · Xiao Guangrong

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Quoted 8 lines
Quoted 1 line
Signed-off-by Xiao Guangrong <[email protected]>

Hi Steven,

I'm sorry, please pull this patch too, because "[PATCH 7/8] tracing/events: fix the include file dependencies" is based on this patch, else will occur building error.

Sorry again, I say the wrong words, it not has building error, just not complete fix the bug which I mention it in the changelog of "[PATCH 7/8] tracing/events: fix the include file dependencies", that is we can't include more TRACE_EVENT head file in .c file all the same, like below:

Quoted 8 lines

Both define TRACE_EVENT in trace_a.h and trace_b.h, if we include those in .c file, like this:

#define CREATE_TRACE_POINTS
include <trace/events/trace_a.h> // re-define DECLARE_TRACE

include <trace/events/trace_b.h> // use the DECLARE_TRACE definition

				  // that re-define by trace_a.h

Hi Xiao,

I pushed out the "fix the include file dependencies" patch since that was a bug fix in its own right. I held off the NAPI patch because it looked like it needed rework and only depended on the include dependencies fix.

Could you update this code to do as Neil and Ingo has mentioned.

Thanks,

-- Steve

author
#8 lore raw
In reply to Xiao Guangrong
Quoted 7 lines · Xiao Guangrong

Currently, the napi's tracepoint works will is depend on "DECLARE_TRACE" definiens in include/trace/define_trace.h, like below:

#include <trace/events/skb.h> // include define_trace.h
#include <trace/events/napi.h>

Quoted 7 lines

there have error, if we remove "#include <trace/events/skb.h>" or include napi.h in the front of include skb.h, It should depend on the definiens in include/linux/tracepoint.h and we can remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because "TRACE_EVENT" not use it

Signed-off-by Xiao Guangrong <[email protected]>

Sorry for forget to CC David Miller and [email protected], also mistype the title: trace_evetns -> trace_events, please ignore this mail and I'll resend it soon

author
#9 lore raw

PATCH 2/2 trace_events: fix the include file dependencies

In reply to Xiao Guangrong

The TRACE_EVENT depend on the include/linux/tracepoint.h first and include/trace/ftrace.h later, if we include the ftrace.h early, It'll occur building error, like blow:

Both define TRACE_EVENT in trace_a.h and trace_b.h, if we include those in .c file, like this:

#define CREATE_TRACE_POINTS
include <trace/events/trace_a.h>
include <trace/events/trace_b.h>

There are can't work, because the TRACE_EVENT has re-defined by the previous .h file

Reported-by Wei Yongjun <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/tracepoint.h   |    3 +--
 include/trace/define_trace.h |    1 +
 2 files changed, 2 insertions(+), 2 deletions(-)
include/linux/tracepoint.h +1 -2
index 5984ed0..8170985 100644
@@ -180,6 +180,7 @@ static inline void tracepoint_synchronize_unregister(void)
 }
 
 #define PARAMS(args...) args
+#endif
 
 #ifndef TRACE_EVENT
 /*

@@ -287,5 +288,3 @@ static inline void tracepoint_synchronize_unregister(void)
 #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
 #endif
-
-#endif
include/trace/define_trace.h +1 -0
index 76e93bf..202cecd 100644
@@ -52,6 +52,7 @@
 #include <trace/ftrace.h>
 #endif
 
+#undef TRACE_EVENT
 #undef TRACE_HEADER_MULTI_READ
 
 /* Only undef what we defined in this file */
Signature · 1 line
1.6.1.2
Acked-by on v1 2/2
#10 lore raw

Re: [PATCH 2/2] trace_events: fix the include file dependencies

Quoted 26 lines · Xiao Guangrong

The TRACE_EVENT depend on the include/linux/tracepoint.h first and include/trace/ftrace.h later, if we include the ftrace.h early, It'll occur building error, like blow:

Both define TRACE_EVENT in trace_a.h and trace_b.h, if we include those in .c file, like this:

#define CREATE_TRACE_POINTS
include <trace/events/trace_a.h>
include <trace/events/trace_b.h>

There are can't work, because the TRACE_EVENT has re-defined by the previous .h file

Reported-by Wei Yongjun <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/tracepoint.h   |    3 +--
 include/trace/define_trace.h |    1 +
 2 files changed, 2 insertions(+), 2 deletions(-)
include/linux/tracepoint.h +0 -0
index 5984ed0..8170985 100644

include/linux/tracepoint.h static inline void tracepoint_synchronize_unregister(void) · line 183

Quoted 4 lines
include/linux/tracepoint.h +1 -0
@@ -180,6 +180,7 @@ static inline void tracepoint_synchronize_unregister(void)
 }
 
 #define PARAMS(args...) args
+#endif

Please add a comment to explain what your endifs are closing, that helps for reviews. Especially while walking on such complicated header files, it's a crutch.

I guess this one closes _LINUX_TRACEPOINT_H, right?

Quoted 14 lines
include/linux/tracepoint.h +0 -2
 #ifndef TRACE_EVENT
 /*

@@ -287,5 +288,3 @@ static inline void tracepoint_synchronize_unregister(void)
 #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
 #endif
-
-#endif
include/trace/define_trace.h +0 -0
index 76e93bf..202cecd 100644

include/trace/define_trace.h line 58

Quoted 7 lines
include/trace/define_trace.h +1 -0
@@ -52,6 +52,7 @@
 #include <trace/ftrace.h>
 #endif
 
+#undef TRACE_EVENT
 #undef TRACE_HEADER_MULTI_READ
 
 /* Only undef what we defined in this file */

Well, hopefully we are not missing something subtle, but yeah that seems to solve the problem, for both CONFIG_EVENT_TRACING and !CONFIG_EVENT_TRACING cases.
And that seems to also fit well whenever CREATE_TRACE_POINTS is defined or not.

Other than the missing comment:

Acked-by Frederic Weisbecker <[email protected]>

Thanks.

maintainer
#11 lore raw

Re: [PATCH 2/2] trace_events: fix the include file dependencies

Quoted 37 lines · Frederic Weisbecker
Quoted 26 lines · Xiao Guangrong

The TRACE_EVENT depend on the include/linux/tracepoint.h first and include/trace/ftrace.h later, if we include the ftrace.h early, It'll occur building error, like blow:

Both define TRACE_EVENT in trace_a.h and trace_b.h, if we include those in .c file, like this:

#define CREATE_TRACE_POINTS
include <trace/events/trace_a.h>
include <trace/events/trace_b.h>

There are can't work, because the TRACE_EVENT has re-defined by the previous .h file

Reported-by Wei Yongjun <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/tracepoint.h   |    3 +--
 include/trace/define_trace.h |    1 +
 2 files changed, 2 insertions(+), 2 deletions(-)
include/linux/tracepoint.h +0 -0
index 5984ed0..8170985 100644

include/linux/tracepoint.h static inline void tracepoint_synchronize_unregister(void) · line 183

Quoted 4 lines
include/linux/tracepoint.h +1 -0
@@ -180,6 +180,7 @@ static inline void tracepoint_synchronize_unregister(void)
 }
 
 #define PARAMS(args...) args
+#endif

Please add a comment to explain what your endifs are closing, that helps for reviews. Especially while walking on such complicated header files, it's a crutch.

Quoted 1 line

I guess this one closes _LINUX_TRACEPOINT_H, right?

This needs a lot more than a simple /* _LINUX_TRACEPOINT_H */ comment. It needs a nice explanation to why this is happening. This is a global header (located in include/linux), when it does something out of the norm, it needs a nice comment to explain why.

I'll pull this in and add a separate commit that adds the comment.

Thanks!

-- Steve

#12 lore raw

Re: [PATCH 2/2] trace_events: fix the include file dependencies

In reply to Xiao Guangrong
Quoted 11 lines · Xiao Guangrong

The TRACE_EVENT depend on the include/linux/tracepoint.h first and include/trace/ftrace.h later, if we include the ftrace.h early, It'll occur building error, like blow:

Both define TRACE_EVENT in trace_a.h and trace_b.h, if we include those in .c file, like this:

#define CREATE_TRACE_POINTS
include <trace/events/trace_a.h>
include <trace/events/trace_b.h>

Quoted 5 lines

There are can't work, because the TRACE_EVENT has re-defined by the previous .h file

Reported-by Wei Yongjun <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>

I'll queue it for .32 and handle the missing comment, thanks!

Quoted 39 lines
---
 include/linux/tracepoint.h   |    3 +--
 include/trace/define_trace.h |    1 +
 2 files changed, 2 insertions(+), 2 deletions(-)
include/linux/tracepoint.h +1 -2
index 5984ed0..8170985 100644
@@ -180,6 +180,7 @@ static inline void tracepoint_synchronize_unregister(void)
 }
 
 #define PARAMS(args...) args
+#endif
 
 #ifndef TRACE_EVENT
 /*

@@ -287,5 +288,3 @@ static inline void tracepoint_synchronize_unregister(void)
 #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
 #endif
-
-#endif
include/trace/define_trace.h +1 -0
index 76e93bf..202cecd 100644
@@ -52,6 +52,7 @@
 #include <trace/ftrace.h>
 #endif
 
+#undef TRACE_EVENT
 #undef TRACE_HEADER_MULTI_READ
 
 /* Only undef what we defined in this file */
Signature · 1 line
1.6.1.2
author
#13 lore raw

[tip:tracing/core] tracing/events: fix the include file dependencies

In reply to Xiao Guangrong
Commit-ID:  5ac35daa9343936038a3c9c4f4d6d3fe6a2a7bd8
Gitweb:     http://git.kernel.org/tip/5ac35daa9343936038a3c9c4f4d6d3fe6a2a7bd8
Author:     Xiao Guangrong <[email protected]>
AuthorDate: Tue, 25 Aug 2009 14:06:22 +0800
Committer:  Steven Rostedt <[email protected]>
CommitDate: Wed, 26 Aug 2009 00:32:09 -0400

tracing/events: fix the include file dependencies

The TRACE_EVENT depends on the include/linux/tracepoint.h first and include/trace/ftrace.h later, if we include the ftrace.h early, a building error will occur.

Both define TRACE_EVENT in trace_a.h and trace_b.h, if we include those in .c file, like this:

#define CREATE_TRACE_POINTS
include <trace/events/trace_a.h>
include <trace/events/trace_b.h>

The above will not work, because the TRACE_EVENT was re-defined by the previous .h file.

Reported-by Wei Yongjun <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/tracepoint.h   |    3 +--
 include/trace/define_trace.h |    1 +
 2 files changed, 2 insertions(+), 2 deletions(-)
include/linux/tracepoint.h +1 -2
index 5984ed0..8170985 100644
@@ -180,6 +180,7 @@ static inline void tracepoint_synchronize_unregister(void)
 }
 
 #define PARAMS(args...) args
+#endif
 
 #ifndef TRACE_EVENT
 /*

@@ -287,5 +288,3 @@ static inline void tracepoint_synchronize_unregister(void)
 #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
 #endif
-
-#endif
include/trace/define_trace.h +1 -0
index f7a7ae1..cd150b9 100644
@@ -56,6 +56,7 @@
 #include <trace/ftrace.h>
 #endif
 
+#undef TRACE_EVENT
 #undef TRACE_HEADER_MULTI_READ
 
 /* Only undef what we defined in this file */