Patchwork [v2] ftrace: change the export format of trace_kfree_skb()

login
register
mail settings
Submitter Xiao Guangrong
Date June 18, 2009, 3:26 a.m.
Message ID <4A39B3E4.6060004@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/28830/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Xiao Guangrong - June 18, 2009, 3:26 a.m.
Use %pf instead of %p to output the function's address and print the
protocol's name.

Before patch:

  <idle>-0     [000] 60144.542521: kfree_skb: skbaddr=de7b8240 protocol=2048 location=c1365429
  <idle>-0     [000] 60164.488153: kfree_skb: skbaddr=da66f900 protocol=2048 location=c1365429
  <idle>-0     [000] 60193.493933: kfree_skb: skbaddr=deaeb480 protocol=4 location=c134ec25
  <idle>-0     [000] 60253.118421: kfree_skb: skbaddr=de7c4900 protocol=4 location=c134ec25
          
After patch:

  <idle>-0     [000]   169.979205: kfree_skb: skbaddr=ceddc240 protocol=ETH_P_802_2 location=netif_receive_skb
  <idle>-0     [000]   172.587000: kfree_skb: skbaddr=ceddc300 protocol=ETH_P_802_2 location=netif_receive_skb
    ping-3391  [000]   192.109803: kfree_skb: skbaddr=ceddc900 protocol=ETH_P_IP location=icmp_rcv
    ping-3391  [000]   192.109902: kfree_skb: skbaddr=ceddc780 protocol=ETH_P_IP location=icmp_rcv

Changelog v1->v2:
Convert protocol from raw numeric to its name as Frederic's suggestion

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/trace/events/skb.h |   70 ++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 68 insertions(+), 2 deletions(-)
Neil Horman - June 18, 2009, 10:35 a.m.
On Thu, Jun 18, 2009 at 11:26:28AM +0800, Xiao Guangrong wrote:
> Use %pf instead of %p to output the function's address and print the
> protocol's name.
> 
> Before patch:
> 
>   <idle>-0     [000] 60144.542521: kfree_skb: skbaddr=de7b8240 protocol=2048 location=c1365429
>   <idle>-0     [000] 60164.488153: kfree_skb: skbaddr=da66f900 protocol=2048 location=c1365429
>   <idle>-0     [000] 60193.493933: kfree_skb: skbaddr=deaeb480 protocol=4 location=c134ec25
>   <idle>-0     [000] 60253.118421: kfree_skb: skbaddr=de7c4900 protocol=4 location=c134ec25
>           
> After patch:
> 
>   <idle>-0     [000]   169.979205: kfree_skb: skbaddr=ceddc240 protocol=ETH_P_802_2 location=netif_receive_skb
>   <idle>-0     [000]   172.587000: kfree_skb: skbaddr=ceddc300 protocol=ETH_P_802_2 location=netif_receive_skb
>     ping-3391  [000]   192.109803: kfree_skb: skbaddr=ceddc900 protocol=ETH_P_IP location=icmp_rcv
>     ping-3391  [000]   192.109902: kfree_skb: skbaddr=ceddc780 protocol=ETH_P_IP location=icmp_rcv
> 
> Changelog v1->v2:
> Convert protocol from raw numeric to its name as Frederic's suggestion
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  include/trace/events/skb.h |   70 ++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> index 1e8fabb..2496060 100644
> --- a/include/trace/events/skb.h
> +++ b/include/trace/events/skb.h
> @@ -7,6 +7,71 @@
>  #undef TRACE_SYSTEM
>  #define TRACE_SYSTEM skb
>  
> +#define protocol_name(protocol) { protocol, #protocol }
> +#define show_protocol_name(val)					\
> +	__print_symbolic(val,					\

Don't you need to include ftrace.h to pull in the __print_symbolic definition?
Or is that always guaranteed to be included from tracepoint.h?

Neil

> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt - June 18, 2009, 12:51 p.m.
On Thu, 18 Jun 2009, Neil Horman wrote:
> > 
> > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> > index 1e8fabb..2496060 100644
> > --- a/include/trace/events/skb.h
> > +++ b/include/trace/events/skb.h
> > @@ -7,6 +7,71 @@
> >  #undef TRACE_SYSTEM
> >  #define TRACE_SYSTEM skb
> >  
> > +#define protocol_name(protocol) { protocol, #protocol }
> > +#define show_protocol_name(val)					\
> > +	__print_symbolic(val,					\
> 
> Don't you need to include ftrace.h to pull in the __print_symbolic definition?
> Or is that always guaranteed to be included from tracepoint.h?

Its use is in one of the fields of TRACE_EVENT that are only used by 
define_trace.h. All other users will ignore it.

#define TRACE_EVENT(name, proto, args, struct, assign, print)	\
	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))

Only name, proto, args is used. The define_trace.h will include 
trace/ftrace.h and that will use the struct, assign and print args.

So the answer is "no" ;-)

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Neil Horman - June 18, 2009, 2:08 p.m.
On Thu, Jun 18, 2009 at 08:51:12AM -0400, Steven Rostedt wrote:
> 
> On Thu, 18 Jun 2009, Neil Horman wrote:
> > > 
> > > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> > > index 1e8fabb..2496060 100644
> > > --- a/include/trace/events/skb.h
> > > +++ b/include/trace/events/skb.h
> > > @@ -7,6 +7,71 @@
> > >  #undef TRACE_SYSTEM
> > >  #define TRACE_SYSTEM skb
> > >  
> > > +#define protocol_name(protocol) { protocol, #protocol }
> > > +#define show_protocol_name(val)					\
> > > +	__print_symbolic(val,					\
> > 
> > Don't you need to include ftrace.h to pull in the __print_symbolic definition?
> > Or is that always guaranteed to be included from tracepoint.h?
> 
> Its use is in one of the fields of TRACE_EVENT that are only used by 
> define_trace.h. All other users will ignore it.
> 
> #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
> 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
> 
> Only name, proto, args is used. The define_trace.h will include 
> trace/ftrace.h and that will use the struct, assign and print args.
> 
> So the answer is "no" ;-)
> 
> -- Steve
> 
> 

Ok, thanks
Acked-by: Neil Horman <nhorman@tuxdriver.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt - June 19, 2009, 12:33 a.m.
On Thu, 18 Jun 2009, Xiao Guangrong wrote:

> Use %pf instead of %p to output the function's address and print the
> protocol's name.
> 
> Before patch:
> 
>   <idle>-0     [000] 60144.542521: kfree_skb: skbaddr=de7b8240 protocol=2048 location=c1365429
>   <idle>-0     [000] 60164.488153: kfree_skb: skbaddr=da66f900 protocol=2048 location=c1365429
>   <idle>-0     [000] 60193.493933: kfree_skb: skbaddr=deaeb480 protocol=4 location=c134ec25
>   <idle>-0     [000] 60253.118421: kfree_skb: skbaddr=de7c4900 protocol=4 location=c134ec25
>           
> After patch:
> 
>   <idle>-0     [000]   169.979205: kfree_skb: skbaddr=ceddc240 protocol=ETH_P_802_2 location=netif_receive_skb
>   <idle>-0     [000]   172.587000: kfree_skb: skbaddr=ceddc300 protocol=ETH_P_802_2 location=netif_receive_skb
>     ping-3391  [000]   192.109803: kfree_skb: skbaddr=ceddc900 protocol=ETH_P_IP location=icmp_rcv
>     ping-3391  [000]   192.109902: kfree_skb: skbaddr=ceddc780 protocol=ETH_P_IP location=icmp_rcv
> 
> Changelog v1->v2:
> Convert protocol from raw numeric to its name as Frederic's suggestion
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

I get this:

  CC      net/core/net-traces.o
In file included from include/trace/ftrace.h:262,
                 from include/trace/define_trace.h:57,
                 from include/trace/events/skb.h:107,
                 from net/core/net-traces.c:28:
include/trace/events/skb.h: In function 'ftrace_raw_output_kfree_skb':
include/trace/events/skb.h:78: error: 'ETH_P_IEEE802154' undeclared (first 
use in this function)
include/trace/events/skb.h:78: error: (Each undeclared identifier is 
reported only once
include/trace/events/skb.h:78: error: for each function it appears in.)

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong - June 19, 2009, 2:05 a.m.
Steven Rostedt wrote:

>> Changelog v1->v2:
>> Convert protocol from raw numeric to its name as Frederic's suggestion
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> I get this:
> 
>   CC      net/core/net-traces.o
> In file included from include/trace/ftrace.h:262,
>                  from include/trace/define_trace.h:57,
>                  from include/trace/events/skb.h:107,
>                  from net/core/net-traces.c:28:
> include/trace/events/skb.h: In function 'ftrace_raw_output_kfree_skb':
> include/trace/events/skb.h:78: error: 'ETH_P_IEEE802154' undeclared (first 
> use in this function)
> include/trace/events/skb.h:78: error: (Each undeclared identifier is 
> reported only once
> include/trace/events/skb.h:78: error: for each function it appears in.)
> 

I use Ingo's tip tree and this build error not occur in my test, I also find
ETH_P_IEEE802154 is added by Sergey Lapin's patch, it's merged recently, can
be found here:
	http://marc.info/?l=git-commits-head&m=124515655608598&w=2

Could you check ETH_P_IEEE802154 is defined in include/linux/if_ether.h and
your code is the latest?

Thanks,
Xiao Guangrong

> -- Steve
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt - June 19, 2009, 2:11 a.m.
On Fri, 19 Jun 2009, Xiao Guangrong wrote:

> 
> 
> Steven Rostedt wrote:
> 
> >> Changelog v1->v2:
> >> Convert protocol from raw numeric to its name as Frederic's suggestion
> >>
> >> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> >> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> > 
> > I get this:
> > 
> >   CC      net/core/net-traces.o
> > In file included from include/trace/ftrace.h:262,
> >                  from include/trace/define_trace.h:57,
> >                  from include/trace/events/skb.h:107,
> >                  from net/core/net-traces.c:28:
> > include/trace/events/skb.h: In function 'ftrace_raw_output_kfree_skb':
> > include/trace/events/skb.h:78: error: 'ETH_P_IEEE802154' undeclared (first 
> > use in this function)
> > include/trace/events/skb.h:78: error: (Each undeclared identifier is 
> > reported only once
> > include/trace/events/skb.h:78: error: for each function it appears in.)
> > 
> 
> I use Ingo's tip tree and this build error not occur in my test, I also find
> ETH_P_IEEE802154 is added by Sergey Lapin's patch, it's merged recently, can
> be found here:
> 	http://marc.info/?l=git-commits-head&m=124515655608598&w=2
> 
> Could you check ETH_P_IEEE802154 is defined in include/linux/if_ether.h and
> your code is the latest?

Hmm, I just work with the tracing/* branches. If this depends on another 
branch, then I'll let Ingo take care of it.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - June 24, 2009, 6:58 a.m.
FWIW there are parts I don't like about this change.

It's just the show_protocol_name() bit, it's designed to always be
out of date.

Nobody is going to look at trace/events/skb.h when they add a new
ETH_P_* value.

We could make some include/linux/eth_protos.def file, that has
lines like:

ETHERNET_PROTOCOL(ETH_P_FOO, N)

then headers that want to instantiate something like the usual
ETH_P_FOO defines or this symbolic printing table define
"ETHERNET_PROTOCOL" to a suitable macro and then include
linux/eth_protos.def to make the expansions.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong - June 26, 2009, 1:24 a.m.
David Miller wrote:
> FWIW there are parts I don't like about this change.
> 
> It's just the show_protocol_name() bit, it's designed to always be
> out of date.
> 
> Nobody is going to look at trace/events/skb.h when they add a new
> ETH_P_* value.
> 
> We could make some include/linux/eth_protos.def file, that has
> lines like:
> 
> ETHERNET_PROTOCOL(ETH_P_FOO, N)
> 
> then headers that want to instantiate something like the usual
> ETH_P_FOO defines or this symbolic printing table define
> "ETHERNET_PROTOCOL" to a suitable macro and then include
> linux/eth_protos.def to make the expansions.
> 

Thanks for your review and your nice solution.
I will fix it in next version. ;-)

Thanks,
Xiao

> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 1e8fabb..2496060 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -7,6 +7,71 @@ 
 #undef TRACE_SYSTEM
 #define TRACE_SYSTEM skb
 
+#define protocol_name(protocol) { protocol, #protocol }
+#define show_protocol_name(val)					\
+	__print_symbolic(val,					\
+			 protocol_name(ETH_P_LOOP),		\
+			 protocol_name(ETH_P_PUP),		\
+			 protocol_name(ETH_P_PUPAT),		\
+			 protocol_name(ETH_P_IP),		\
+			 protocol_name(ETH_P_X25),		\
+			 protocol_name(ETH_P_ARP),		\
+			 protocol_name(ETH_P_BPQ),		\
+			 protocol_name(ETH_P_IEEEPUP),		\
+			 protocol_name(ETH_P_IEEEPUPAT),	\
+			 protocol_name(ETH_P_DEC),		\
+			 protocol_name(ETH_P_DNA_DL),		\
+			 protocol_name(ETH_P_DNA_RC),		\
+			 protocol_name(ETH_P_DNA_RT),		\
+			 protocol_name(ETH_P_LAT),		\
+			 protocol_name(ETH_P_DIAG),		\
+			 protocol_name(ETH_P_CUST),		\
+			 protocol_name(ETH_P_SCA),		\
+			 protocol_name(ETH_P_TEB),		\
+			 protocol_name(ETH_P_RARP),		\
+			 protocol_name(ETH_P_ATALK),		\
+			 protocol_name(ETH_P_AARP),		\
+			 protocol_name(ETH_P_8021Q),		\
+			 protocol_name(ETH_P_IPX),		\
+			 protocol_name(ETH_P_IPV6),		\
+			 protocol_name(ETH_P_PAUSE),		\
+			 protocol_name(ETH_P_SLOW),		\
+			 protocol_name(ETH_P_WCCP),		\
+			 protocol_name(ETH_P_PPP_DISC),		\
+			 protocol_name(ETH_P_PPP_SES),		\
+			 protocol_name(ETH_P_MPLS_UC),		\
+			 protocol_name(ETH_P_MPLS_MC),		\
+			 protocol_name(ETH_P_ATMMPOA),		\
+			 protocol_name(ETH_P_ATMFATE),		\
+			 protocol_name(ETH_P_PAE),		\
+			 protocol_name(ETH_P_AOE),		\
+			 protocol_name(ETH_P_TIPC),		\
+			 protocol_name(ETH_P_FCOE),		\
+			 protocol_name(ETH_P_FIP),		\
+			 protocol_name(ETH_P_EDSA),		\
+			 protocol_name(ETH_P_802_3),		\
+			 protocol_name(ETH_P_AX25),		\
+			 protocol_name(ETH_P_ALL),		\
+			 protocol_name(ETH_P_802_2),		\
+			 protocol_name(ETH_P_SNAP),		\
+			 protocol_name(ETH_P_DDCMP),		\
+			 protocol_name(ETH_P_WAN_PPP),		\
+			 protocol_name(ETH_P_PPP_MP),		\
+			 protocol_name(ETH_P_LOCALTALK),	\
+			 protocol_name(ETH_P_CAN),		\
+			 protocol_name(ETH_P_PPPTALK),		\
+			 protocol_name(ETH_P_TR_802_2),		\
+			 protocol_name(ETH_P_MOBITEX),		\
+			 protocol_name(ETH_P_CONTROL),		\
+			 protocol_name(ETH_P_IRDA),		\
+			 protocol_name(ETH_P_ECONET),		\
+			 protocol_name(ETH_P_HDLC),		\
+			 protocol_name(ETH_P_ARCNET),		\
+			 protocol_name(ETH_P_DSA),		\
+			 protocol_name(ETH_P_TRAILER),		\
+			 protocol_name(ETH_P_PHONET),		\
+			 protocol_name(ETH_P_IEEE802154))
+
 /*
  * Tracepoint for free an sk_buff:
  */
@@ -30,8 +95,9 @@  TRACE_EVENT(kfree_skb,
 		__entry->location = location;
 	),
 
-	TP_printk("skbaddr=%p protocol=%u location=%p",
-		__entry->skbaddr, __entry->protocol, __entry->location)
+	TP_printk("skbaddr=%p protocol=%s location=%pf",
+		  __entry->skbaddr, show_protocol_name(__entry->protocol),
+		  __entry->location)
 );
 
 #endif /* _TRACE_SKB_H */