tracing/events: convert NAPI's tracepoint via TRACE_EVENT

PATCH RESEND In review Xiao Guangrong 5 participants 10 messages Aug 31, 2009 – Sep 4, 2009

Subsystems (3)

Networking Network drivers Tracing

lore

Share & tools

lore

Xiao Guangrong

author

2009-08-31 06:16 UTC #0 lore raw

PATCH RESEND 1/1 tracing/events: convert NAPI's tracepoint via TRACE_EVENT

- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:

   sshd-2503  [000]    71.920846: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020291: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020418: napi_poll: ifname=eth0 state=NAPI_STATE_SCHED weigth=16 poll=pcnet32_poll

- 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 ---
 include/trace/events/napi.h  |   43 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 9 deletions(-)

include/trace/define_trace.h +0 -4

index 2a4b3bf..6158741 100644
@@ -31,10 +31,6 @@
 		assign, print, reg, unreg)			\
 	DEFINE_TRACE_FN(name, reg, unreg)
 
-#undef DECLARE_TRACE
-#define DECLARE_TRACE(name, proto, args)	\
-	DEFINE_TRACE(name)
-
 #undef TRACE_INCLUDE
 #undef __TRACE_INCLUDE

include/trace/events/napi.h +38 -5

49 lines not on this page · raw message

Signature · 1 line

1.6.1.2

Ingo Molnar

2009-08-31 08:49 UTC #1 lore raw

Quoted 2 lines · Xiao Guangrong

- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:

I think as long as it does not touch tracing infrastructure (which your patches dont do in their current form) this should be done/merged via the networking tree.

[ There might be some small collisions in define_trace.h (because
these tracepoints move from legacy to new-style TRACE_EVENT() form) but that's OK. ]

Ingo

Steven Rostedt

maintainer

2009-08-31 18:09 UTC #2 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 2 lines · Xiao Guangrong
- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:
I think as long as it does not touch tracing infrastructure (which your patches dont do in their current form) this should be done/merged via the networking tree.

I agree, all changes that are in include/trace/events/ and trace point usage can stay within the subsystem tree.

Quoted 4 lines

[ There might be some small collisions in define_trace.h (because
these tracepoints move from legacy to new-style TRACE_EVENT() form) but that's OK. ]

But changes to anything in include/trace or kernel/trace needs to go throught the tracing subsystem. This includes a changes to define_trace.h.

Thanks,

-- Steve

David Miller

maintainer

2009-09-02 01:26 UTC #3 lore raw

From: Steven Rostedt <[email protected]>
Date: Mon, 31 Aug 2009 14:09:04 -0400 (EDT)

Quoted 17 lines · Ingo Molnar

Quoted 10 lines · Xiao Guangrong
Quoted 3 lines
- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:
I think as long as it does not touch tracing infrastructure (which your patches dont do in their current form) this should be done/merged via the networking tree.
I agree, all changes that are in include/trace/events/ and trace point usage can stay within the subsystem tree.
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
[ There might be some small collisions in define_trace.h (because
these tracepoints move from legacy to new-style TRACE_EVENT() form) but that's OK. ]
But changes to anything in include/trace or kernel/trace needs to go throught the tracing subsystem. This includes a changes to define_trace.h.

This patch can't be split up, so I'm wondering how you suggest to handle this patch given that you have declared that define_trace.h changes aren't to go through the subsystem tree?

If we do the define_trace.h change only, we break the build (lack of macro defined for the trace).

If we do only the other parts of his patch, we get a duplicate definition.

And keep in mind that Neil and Xiao are probably going to want to do work on top of this to the networking bits. Thus if we put this patch here into the tracing tree, I'll have to develop a dependency on the tracing tree and I think that will go over like a fart in a spacesuit with the -next crowd and Stephen Rothwell in particular.

Please advise.

Xiao Guangrong

author

2009-09-02 03:12 UTC #4 lore raw

Quoted 10 lines · David Miller

This patch can't be split up, so I'm wondering how you suggest to handle this patch given that you have declared that define_trace.h changes aren't to go through the subsystem tree?
If we do the define_trace.h change only, we break the build (lack of macro defined for the trace).
If we do only the other parts of his patch, we get a duplicate definition.

This patch cooperate with my previous patch to solve the include file dependencies problem. So, we do better put those two patches together. (both in -tip tree or/add both in network tree)

The previous patch is below:

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

Quoted 6 lines

And keep in mind that Neil and Xiao are probably going to want to do work on top of this to the networking bits. Thus if we put this patch here into the tracing tree, I'll have to develop a dependency on the tracing tree and I think that will go over like a fart in a spacesuit with the -next crowd and Stephen Rothwell in particular.

Thank you, David

Xiao

Ingo Molnar

2009-09-04 07:06 UTC #5 lore raw

Quoted 9 lines · Xiao Guangrong

Quoted 7 lines · David Miller
This patch can't be split up, so I'm wondering how you suggest to handle this patch given that you have declared that define_trace.h changes aren't to go through the subsystem tree?
If we do the define_trace.h change only, we break the build (lack of macro defined for the trace).

Quoted 7 lines

Quoted 3 lines
If we do only the other parts of his patch, we get a duplicate definition.
This patch cooperate with my previous patch to solve the include file dependencies problem. So, we do better put those two patches together. (both in -tip tree or/add both in network tree)

I would not mind a duplicate commit here. Whoever merges later in the merge window will resolve the (easy) conflict.

David, you definitely dont want to pull the full tracing tree into the networking tree. It wouldnt cause technical problems in linux-next (Git sorts out such merges just fine and both trees are well tested) but it creates an unwanted merge window dependency: if the tracing tree's push to Linus is delayed for whatever reason the networking tree is delayed too. That's not really good.

It's also not good on a conceptual angle - it makes it very difficult for Linus to reject any of the trees, due to the mingling.

A second variant would be that we could create a mini-topic (.31-rc8 based) in -tip with just the skb bits and the header file changes and once we've tested it we could push it and you could pull that (small, -git based) tree into the networking tree.

A third variant would be that you could do such a mini-topic branch yourself in the networking tree and we could pull that into the tracing tree.

All of these variants work fine with Steve's and my workload, it's up to you and Neil - you are driving the changes so do whatever is most convenient to you.

Ingo

Steven Rostedt

maintainer

2009-08-31 18:03 UTC #6 lore raw

In reply to Xiao Guangrong

Quoted 9 lines · Xiao Guangrong

- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:
   sshd-2503  [000]    71.920846: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020291: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020418: napi_poll: ifname=eth0 state=NAPI_STATE_SCHED weigth=16 poll=pcnet32_poll
- Remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because TRACE_EVENT not use it

The above really needs to be in a separate patch, since it is a tracing infrastructure change.

-- Steve

Quoted 80 lines

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

---
 include/trace/define_trace.h |    4 ---
 include/trace/events/napi.h  |   43 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 9 deletions(-)

include/trace/define_trace.h +0 -4

index 2a4b3bf..6158741 100644
@@ -31,10 +31,6 @@
 		assign, print, reg, unreg)			\
 	DEFINE_TRACE_FN(name, reg, unreg)
 
-#undef DECLARE_TRACE
-#define DECLARE_TRACE(name, proto, args)	\
-	DEFINE_TRACE(name)
-
 #undef TRACE_INCLUDE
 #undef __TRACE_INCLUDE

include/trace/events/napi.h +38 -5

index a8989c4..5c7a192 100644
@@ -1,11 +1,44 @@
-#ifndef _TRACE_NAPI_H_
-#define _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,
+#define show_napi_state(state) state ? __print_flags(state, "|",	\
+	{ (1UL << NAPI_STATE_SCHED),	"NAPI_STATE_SCHED"	},	\
+	{ (1UL << NAPI_STATE_DISABLE),	"NAPI_STATE_DISABLE"	},	\
+	{ (1UL << NAPI_STATE_NPSVC),	"NAPI_STATE_NPSVC"	}) : "0"
+
+TRACE_EVENT(napi_poll,
+
 	TP_PROTO(struct napi_struct *napi),
-	TP_ARGS(napi));
 
-#endif
+	TP_ARGS(napi),
+
+	TP_STRUCT__entry(
+		__field(	unsigned long,	state		)
+		__field(	int,		weight		)
+		__field(	void *,		poll		)
+		__string(	ifname,		(napi->dev ?
+				napi->dev->name : "backlog")	)
+	),
+
+	TP_fast_assign(
+		__entry->state	= napi->state;
+		__entry->weight	= napi->weight;
+		__entry->poll	= napi->poll;
+		__assign_str(ifname, napi->dev ? napi->dev->name : "backlog")
+	),
+
+	TP_printk("ifname=%s state=%s weight=%d poll=%pf",
+		  __get_str(ifname), show_napi_state(__entry->state),
+		  __entry->weight, __entry->poll)
+);
+
+#endif /* _TRACE_NAPI_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

Signature · 1 line

1.6.1.2

Xiao Guangrong

author

2009-09-01 01:06 UTC #7 lore raw

Quoted 9 lines · Steven Rostedt

Quoted 7 lines · Xiao Guangrong

- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:

   sshd-2503  [000]    71.920846: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020291: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020418: napi_poll: ifname=eth0 state=NAPI_STATE_SCHED weigth=16 poll=pcnet32_poll

Quoted 6 lines

Quoted 2 lines
- Remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because TRACE_EVENT not use it
The above really needs to be in a separate patch, since it is a tracing infrastructure change.

Hi Steven,

We can't break this patch into two patches, because it's has redefined 'DECLARE_TRACE' in include/trace/define_trace.h, like below:

1: if we remove 'DECLARE_TRACE' first, then NAPI's tracepoint can't define tracepoint
(it's a NAPI tracepoint's bug,

    see my previous patch: http://marc.info/?l=linux-kernel&m=125118032931716&w=2)

2: if we convert NAPI's tracepoint first, it can't include more TRACE_EVENT .h files in
one .c file, the compiler will complain with variables redefined:

include/trace/events/napi.h:15: error: redefinition of ‘__tpstrtab_napi_poll’ include/trace/events/napi.h:15: error: previous definition of ‘__tpstrtab_napi_poll’ was here include/trace/events/napi.h:15: error: redefinition of ‘__tracepoint_napi_poll’ include/trace/events/napi.h:15: error: previous definition of ‘__tracepoint_napi_poll’ was here In file included from include/trace/ftrace.h:644,

So, I think we do better fix this bug early, then other people can go on his work on ftrace.

Hi Neil,

How do you think about this patch, can you give me some comments?

Thanks,
Xiao

Steven Rostedt

maintainer

2009-09-01 01:41 UTC #8 lore raw

Quoted 30 lines · Xiao Guangrong

Quoted 9 lines · Steven Rostedt
Quoted 7 lines · Xiao Guangrong
- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:
   sshd-2503  [000]    71.920846: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020291: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020418: napi_poll: ifname=eth0 state=NAPI_STATE_SCHED weigth=16 poll=pcnet32_poll
Quoted 6 lines
Quoted 2 lines
- Remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because TRACE_EVENT not use it
The above really needs to be in a separate patch, since it is a tracing infrastructure change.
Hi Steven,
We can't break this patch into two patches, because it's has redefined 'DECLARE_TRACE' in include/trace/define_trace.h, like below:
1: if we remove 'DECLARE_TRACE' first, then NAPI's tracepoint can't define tracepoint
(it's a NAPI tracepoint's bug,
    see my previous patch: http://marc.info/?l=linux-kernel&m=125118032931716&w=2)
2: if we convert NAPI's tracepoint first, it can't include more TRACE_EVENT .h files in
one .c file, the compiler will complain with variables redefined:

Quoted 7 lines

include/trace/events/napi.h:15: error: redefinition of ‘__tpstrtab_napi_poll’ include/trace/events/napi.h:15: error: previous definition of ‘__tpstrtab_napi_poll’ was here include/trace/events/napi.h:15: error: redefinition of ‘__tracepoint_napi_poll’ include/trace/events/napi.h:15: error: previous definition of ‘__tracepoint_napi_poll’ was here In file included from include/trace/ftrace.h:644,
So, I think we do better fix this bug early, then other people can go on his work on ftrace.

Ah, I missed the dependency there. That needs to be commented in the change log. That is, to comment that a dependency exists.

Luckily, none of the DECLARE_TRACE users used the CREATE_TRACEPOINT way of creating the trace points. Of course using DECLARE_TRACE directly is now deprecated, but this change would have broken it if it was used, because the define_trace.h was also written for that in mind. But since we never converted any of the old users to do so, this wont break anything.

OK, you have have Acked-by: Steven Rostedt <[email protected]>

-- Steve

Neil Horman

2009-09-01 11:04 UTC #9 lore raw

In reply to Xiao Guangrong

Quoted 37 lines · Xiao Guangrong

Quoted 9 lines · Steven Rostedt
Quoted 7 lines · Xiao Guangrong
- Convert NAPI's tracepoint via TRACE_EVENT macro, the output information like below:
   sshd-2503  [000]    71.920846: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020291: napi_poll: ifname=eth0 state=0 weigth=16 poll=pcnet32_poll
   sshd-2503  [000]    72.020418: napi_poll: ifname=eth0 state=NAPI_STATE_SCHED weigth=16 poll=pcnet32_poll
Quoted 6 lines
Quoted 2 lines
- Remove the "DECLARE_TRACE" definiens in include/trace/define_trace.h, because TRACE_EVENT not use it
The above really needs to be in a separate patch, since it is a tracing infrastructure change.
Hi Steven,
We can't break this patch into two patches, because it's has redefined 'DECLARE_TRACE' in include/trace/define_trace.h, like below:
1: if we remove 'DECLARE_TRACE' first, then NAPI's tracepoint can't define tracepoint
(it's a NAPI tracepoint's bug,
    see my previous patch: http://marc.info/?l=linux-kernel&m=125118032931716&w=2)
2: if we convert NAPI's tracepoint first, it can't include more TRACE_EVENT .h files in
one .c file, the compiler will complain with variables redefined:
include/trace/events/napi.h:15: error: redefinition of ‘__tpstrtab_napi_poll’ include/trace/events/napi.h:15: error: previous definition of ‘__tpstrtab_napi_poll’ was here include/trace/events/napi.h:15: error: redefinition of ‘__tracepoint_napi_poll’ include/trace/events/napi.h:15: error: previous definition of ‘__tracepoint_napi_poll’ was here In file included from include/trace/ftrace.h:644,

Quoted 6 lines

So, I think we do better fix this bug early, then other people can go on his work on ftrace.
Hi Neil,
How do you think about this patch, can you give me some comments?

For the things that I use the napi_poll traepoint for, this is all fine by me, so ack as far as that goes, but I'm probably not qualified to comment on the tracepoint infrastructure changes you've made (the recent skb ftracer argument is like a good indicator of that :) )

Neil

Quoted 5 lines

Thanks,
Xiao