[PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[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

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 include/trace/define_trace.h |    4 ---
 include/trace/events/napi.h  |   43 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 2a4b3bf..6158741 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -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
 
diff --git a/include/trace/events/napi.h b/include/trace/events/napi.h
index a8989c4..5c7a192 100644
--- a/include/trace/events/napi.h
+++ b/include/trace/events/napi.h
@@ -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>
-- 
1.6.1.2

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Ingo Molnar]

* Xiao Guangrong <xiaoguangrong@cn.fujitsu.com> wrote:

> - 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

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Steven Rostedt]

On Mon, 31 Aug 2009, Xiao Guangrong wrote:

> - 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

> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> ---
>  include/trace/define_trace.h |    4 ---
>  include/trace/events/napi.h  |   43 +++++++++++++++++++++++++++++++++++++----
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index 2a4b3bf..6158741 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -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
>  
> diff --git a/include/trace/events/napi.h b/include/trace/events/napi.h
> index a8989c4..5c7a192 100644
> --- a/include/trace/events/napi.h
> +++ b/include/trace/events/napi.h
> @@ -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>
> -- 
> 1.6.1.2
> 
> 

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Steven Rostedt]

On Mon, 31 Aug 2009, Ingo Molnar wrote:

> 
> * Xiao Guangrong <xiaoguangrong@cn.fujitsu.com> wrote:
> 
> > - 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.

> 
> [ 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

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Xiao Guangrong]

Steven Rostedt wrote:
> On Mon, 31 Aug 2009, Xiao Guangrong wrote:
> 
>> - 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.
> 

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

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Steven Rostedt]

On Tue, 2009-09-01 at 09:06 +0800, Xiao Guangrong wrote:
> 
> Steven Rostedt wrote:
> > On Mon, 31 Aug 2009, Xiao Guangrong wrote:
> > 
> >> - 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.
> > 
> 
> 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.

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 <rostedt@goodmis.org>

-- Steve

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Neil Horman]

On Tue, Sep 01, 2009 at 09:06:44AM +0800, Xiao Guangrong wrote:
> 
> 
> Steven Rostedt wrote:
> > On Mon, 31 Aug 2009, Xiao Guangrong wrote:
> > 
> >> - 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.
> > 
> 
> 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?
> 
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

> 
> Thanks,
> Xiao
> 
> 

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Mon, 31 Aug 2009 14:09:04 -0400 (EDT)

> 
> On Mon, 31 Aug 2009, Ingo Molnar wrote:
> 
>> 
>> * Xiao Guangrong <xiaoguangrong@cn.fujitsu.com> wrote:
>> 
>> > - 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.
> 
>> 
>> [ 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.

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Xiao Guangrong]

David Miller wrote:

> 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 <xiaoguangrong@cn.fujitsu.com>
AuthorDate: Tue, 25 Aug 2009 14:06:22 +0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 26 Aug 2009 00:32:09 -0400

tracing/events: fix the include file dependencies

> 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

Re: [PATCH resend] tracing/events: convert NAPI's tracepoint via TRACE_EVENT

[Ingo Molnar]

* Xiao Guangrong <xiaoguangrong@cn.fujitsu.com> wrote:

> David Miller wrote:
> 
> > 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)

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