Use %pf instead of %p to output the function's address, use 0x%04X instead
of %u to output the skb's protocol
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:
bash-2587 [001] 97685.781173: kfree_skb: skbaddr=deb9cc00 protocol=0x0000 location=netlink_unicast
bash-2587 [000] 97686.501121: kfree_skb: skbaddr=df9bb840 protocol=0x0000 location=netlink_unicast
<idle>-0 [000] 97696.200184: kfree_skb: skbaddr=df741240 protocol=0x0800 location=ip_rcv
<idle>-0 [000] 97696.200198: kfree_skb: skbaddr=dfb3de40 protocol=0x0800 location=ip_rcv
Signed-off-by: Xiao Guangrong <[email protected]>
---
Recover the file mode of include/trace/events/skb.h
---
include/trace/events/skb.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 1e8fabb..a58bef8 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -30,7 +30,7 @@ TRACE_EVENT(kfree_skb,
__entry->location = location;
),
- TP_printk("skbaddr=%p protocol=%u location=%p",
+ TP_printk("skbaddr=%p protocol=0x%04X location=%pf",
__entry->skbaddr, __entry->protocol, __entry->location)
);
--
1.6.1.2
On Tue, Jun 09, 2009 at 01:54:39PM +0800, Xiao Guangrong wrote:
> Use %pf instead of %p to output the function's address, use 0x%04X instead
> of %u to output the skb's protocol
>
> 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:
>
> bash-2587 [001] 97685.781173: kfree_skb: skbaddr=deb9cc00 protocol=0x0000 location=netlink_unicast
> bash-2587 [000] 97686.501121: kfree_skb: skbaddr=df9bb840 protocol=0x0000 location=netlink_unicast
> <idle>-0 [000] 97696.200184: kfree_skb: skbaddr=df741240 protocol=0x0800 location=ip_rcv
> <idle>-0 [000] 97696.200198: kfree_skb: skbaddr=dfb3de40 protocol=0x0800 location=ip_rcv
>
> Signed-off-by: Xiao Guangrong <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Would you also be interested in adding the conversion from raw numeric protocol
to its name defined in include/linux/if_ether.h
I guess __print_flags() can be used for that.
Thanks,
Frederic.
> ---
>
> Recover the file mode of include/trace/events/skb.h
>
> ---
> include/trace/events/skb.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> index 1e8fabb..a58bef8 100644
> --- a/include/trace/events/skb.h
> +++ b/include/trace/events/skb.h
> @@ -30,7 +30,7 @@ TRACE_EVENT(kfree_skb,
> __entry->location = location;
> ),
>
> - TP_printk("skbaddr=%p protocol=%u location=%p",
> + TP_printk("skbaddr=%p protocol=0x%04X location=%pf",
> __entry->skbaddr, __entry->protocol, __entry->location)
> );
>
> --
> 1.6.1.2
>
Frederic Weisbecker wrote:
>
> Acked-by: Frederic Weisbecker <[email protected]>
>
> Would you also be interested in adding the conversion from raw numeric protocol
> to its name defined in include/linux/if_ether.h
>
Hi Frederic:
OK, I'll do it.
> I guess __print_flags() can be used for that.
>
Thanks for you suggestion.
IMHO, __print_symbolic() is more suitable for this. :-)
> Thanks,
> Frederic.
>
>
>
On Wed, 10 Jun 2009, Xiao Guangrong wrote:
>
>
> Frederic Weisbecker wrote:
>
> >
> > Acked-by: Frederic Weisbecker <[email protected]>
> >
> > Would you also be interested in adding the conversion from raw numeric protocol
> > to its name defined in include/linux/if_ether.h
> >
> Hi Frederic:
>
> OK, I'll do it.
>
> > I guess __print_flags() can be used for that.
> >
>
> Thanks for you suggestion.
> IMHO, __print_symbolic() is more suitable for this. :-)
BTW, you might want to take a look at what is coming:
http://marc.info/?l=linux-kernel&m=124458472931447&w=2
-- Steve
On Wed, Jun 10, 2009 at 09:40:45AM +0800, Xiao Guangrong wrote:
>
>
> Frederic Weisbecker wrote:
>
> >
> > Acked-by: Frederic Weisbecker <[email protected]>
> >
> > Would you also be interested in adding the conversion from raw numeric protocol
> > to its name defined in include/linux/if_ether.h
> >
> Hi Frederic:
>
> OK, I'll do it.
Thanks!
>
> > I guess __print_flags() can be used for that.
> >
>
> Thanks for you suggestion.
> IMHO, __print_symbolic() is more suitable for this. :-)
Heh, indeed :)
> > Thanks,
> > Frederic.
> >
> >
>
> >
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 <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
---
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, \
+ 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 */
--
1.6.1.2
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 <[email protected]>
> Acked-by: Frederic Weisbecker <[email protected]>
> ---
> 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
>
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
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 <[email protected]>
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 <[email protected]>
> Acked-by: Frederic Weisbecker <[email protected]>
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
Steven Rostedt wrote:
>> Changelog v1->v2:
>> Convert protocol from raw numeric to its name as Frederic's suggestion
>>
>> Signed-off-by: Xiao Guangrong <[email protected]>
>> Acked-by: Frederic Weisbecker <[email protected]>
>
> 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
>
>
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 <[email protected]>
> >> Acked-by: Frederic Weisbecker <[email protected]>
> >
> > 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
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.
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
>