| Message ID | 4C44F286.1050907@jp.fujitsu.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mardi 20 juillet 2010 à 09:49 +0900, Koki Sanagi a écrit : > [RFC PATCH v3 4/5] skb: add tracepoints to freeing skb > This patch adds tracepoint to consume_skb, dev_kfree_skb_irq and > skb_free_datagram_locked. Combinating with tracepoint on dev_hard_start_xmit, > we can check how long it takes to free transmited packets. And using it, we can > calculate how many packets driver had at that time. It is useful when a drop of > transmited packet is a problem. > > <idle>-0 [001] 241409.218333: consume_skb: skbaddr=dd6b2fb8 > <idle>-0 [001] 241409.490555: dev_kfree_skb_irq: skbaddr=f5e29840 > > udp-recv-302 [001] 515031.206008: skb_free_datagram_locked: skbaddr=f5b1d900 > > > Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com> > --- > include/trace/events/skb.h | 42 ++++++++++++++++++++++++++++++++++++++++++ > net/core/datagram.c | 1 + > net/core/dev.c | 2 ++ > net/core/skbuff.c | 1 + > 4 files changed, 46 insertions(+), 0 deletions(-) > > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h > index 4b2be6d..84c9041 100644 > --- a/include/trace/events/skb.h > +++ b/include/trace/events/skb.h > @@ -35,6 +35,48 @@ TRACE_EVENT(kfree_skb, > __entry->skbaddr, __entry->protocol, __entry->location) > ); > > +DECLARE_EVENT_CLASS(free_skb, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb), > + > + TP_STRUCT__entry( > + __field( void *, skbaddr ) > + ), > + > + TP_fast_assign( > + __entry->skbaddr = skb; > + ), > + > + TP_printk("skbaddr=%p", __entry->skbaddr) > + > +); > + > +DEFINE_EVENT(free_skb, consume_skb, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb) > + > +); > + > +DEFINE_EVENT(free_skb, dev_kfree_skb_irq, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb) > + > +); > + > +DEFINE_EVENT(free_skb, skb_free_datagram_locked, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb) > + > +); > + > TRACE_EVENT(skb_copy_datagram_iovec, > > TP_PROTO(const struct sk_buff *skb, int len), > diff --git a/net/core/datagram.c b/net/core/datagram.c > index f5b6f43..1ea32a0 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -231,6 +231,7 @@ void skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb) > { > bool slow; > > + trace_skb_free_datagram_locked(skb); Here you unconditionally trace before the test on skb->users > if (likely(atomic_read(&skb->users) == 1)) > smp_rmb(); > else if (likely(!atomic_dec_and_test(&skb->users))) > diff --git a/net/core/dev.c b/net/core/dev.c > index 4acfec6..d979847 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -131,6 +131,7 @@ > #include <linux/random.h> > #include <trace/events/napi.h> > #include <trace/events/net.h> > +#include <trace/events/skb.h> > #include <linux/pci.h> > > #include "net-sysfs.h" > @@ -1581,6 +1582,7 @@ void dev_kfree_skb_irq(struct sk_buff *skb) > struct softnet_data *sd; > unsigned long flags; > > + trace_dev_kfree_skb_irq(skb); > local_irq_save(flags); > sd = &__get_cpu_var(softnet_data); > skb->next = sd->completion_queue; > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 34432b4..a7b4036 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -466,6 +466,7 @@ void consume_skb(struct sk_buff *skb) > smp_rmb(); > else if (likely(!atomic_dec_and_test(&skb->users))) > return; While here you trace _after_ the test on skb->users - and a "return;" , so you miss some consume_skb() calls > + trace_consume_skb(skb); > __kfree_skb(skb); > } > EXPORT_SYMBOL(consume_skb); > -- 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
(2010/07/20 13:54), Eric Dumazet wrote: > Le mardi 20 juillet 2010 à 09:49 +0900, Koki Sanagi a écrit : >> [RFC PATCH v3 4/5] skb: add tracepoints to freeing skb >> This patch adds tracepoint to consume_skb, dev_kfree_skb_irq and >> skb_free_datagram_locked. Combinating with tracepoint on dev_hard_start_xmit, >> we can check how long it takes to free transmited packets. And using it, we can >> calculate how many packets driver had at that time. It is useful when a drop of >> transmited packet is a problem. >> >> <idle>-0 [001] 241409.218333: consume_skb: skbaddr=dd6b2fb8 >> <idle>-0 [001] 241409.490555: dev_kfree_skb_irq: skbaddr=f5e29840 >> >> udp-recv-302 [001] 515031.206008: skb_free_datagram_locked: skbaddr=f5b1d900 >> >> >> Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com> >> --- >> include/trace/events/skb.h | 42 ++++++++++++++++++++++++++++++++++++++++++ >> net/core/datagram.c | 1 + >> net/core/dev.c | 2 ++ >> net/core/skbuff.c | 1 + >> 4 files changed, 46 insertions(+), 0 deletions(-) >> >> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h >> index 4b2be6d..84c9041 100644 >> --- a/include/trace/events/skb.h >> +++ b/include/trace/events/skb.h >> @@ -35,6 +35,48 @@ TRACE_EVENT(kfree_skb, >> __entry->skbaddr, __entry->protocol, __entry->location) >> ); >> >> +DECLARE_EVENT_CLASS(free_skb, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb), >> + >> + TP_STRUCT__entry( >> + __field( void *, skbaddr ) >> + ), >> + >> + TP_fast_assign( >> + __entry->skbaddr = skb; >> + ), >> + >> + TP_printk("skbaddr=%p", __entry->skbaddr) >> + >> +); >> + >> +DEFINE_EVENT(free_skb, consume_skb, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb) >> + >> +); >> + >> +DEFINE_EVENT(free_skb, dev_kfree_skb_irq, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb) >> + >> +); >> + >> +DEFINE_EVENT(free_skb, skb_free_datagram_locked, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb) >> + >> +); >> + >> TRACE_EVENT(skb_copy_datagram_iovec, >> >> TP_PROTO(const struct sk_buff *skb, int len), >> diff --git a/net/core/datagram.c b/net/core/datagram.c >> index f5b6f43..1ea32a0 100644 >> --- a/net/core/datagram.c >> +++ b/net/core/datagram.c >> @@ -231,6 +231,7 @@ void skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb) >> { >> bool slow; >> >> + trace_skb_free_datagram_locked(skb); > > Here you unconditionally trace before the test on skb->users > >> if (likely(atomic_read(&skb->users) == 1)) >> smp_rmb(); >> else if (likely(!atomic_dec_and_test(&skb->users))) >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 4acfec6..d979847 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -131,6 +131,7 @@ >> #include <linux/random.h> >> #include <trace/events/napi.h> >> #include <trace/events/net.h> >> +#include <trace/events/skb.h> >> #include <linux/pci.h> >> >> #include "net-sysfs.h" >> @@ -1581,6 +1582,7 @@ void dev_kfree_skb_irq(struct sk_buff *skb) >> struct softnet_data *sd; >> unsigned long flags; >> >> + trace_dev_kfree_skb_irq(skb); >> local_irq_save(flags); >> sd = &__get_cpu_var(softnet_data); >> skb->next = sd->completion_queue; >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index 34432b4..a7b4036 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -466,6 +466,7 @@ void consume_skb(struct sk_buff *skb) >> smp_rmb(); >> else if (likely(!atomic_dec_and_test(&skb->users))) >> return; > > While here you trace _after_ the test on skb->users - and a "return;" , > so you miss some consume_skb() calls > Yeah, I'll move trace_consume_skb() before the test. Thanks, Koki Sanagi. > >> + trace_consume_skb(skb); >> __kfree_skb(skb); >> } >> EXPORT_SYMBOL(consume_skb); >> > > > > -- 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
On Tue, Jul 20, 2010 at 09:49:10AM +0900, Koki Sanagi wrote: > [RFC PATCH v3 4/5] skb: add tracepoints to freeing skb > This patch adds tracepoint to consume_skb, dev_kfree_skb_irq and > skb_free_datagram_locked. Combinating with tracepoint on dev_hard_start_xmit, > we can check how long it takes to free transmited packets. And using it, we can > calculate how many packets driver had at that time. It is useful when a drop of > transmited packet is a problem. > > <idle>-0 [001] 241409.218333: consume_skb: skbaddr=dd6b2fb8 > <idle>-0 [001] 241409.490555: dev_kfree_skb_irq: skbaddr=f5e29840 > > udp-recv-302 [001] 515031.206008: skb_free_datagram_locked: skbaddr=f5b1d900 > > > Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com> > --- > include/trace/events/skb.h | 42 ++++++++++++++++++++++++++++++++++++++++++ > net/core/datagram.c | 1 + > net/core/dev.c | 2 ++ > net/core/skbuff.c | 1 + > 4 files changed, 46 insertions(+), 0 deletions(-) > > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h > index 4b2be6d..84c9041 100644 > --- a/include/trace/events/skb.h > +++ b/include/trace/events/skb.h > @@ -35,6 +35,48 @@ TRACE_EVENT(kfree_skb, > __entry->skbaddr, __entry->protocol, __entry->location) > ); > > +DECLARE_EVENT_CLASS(free_skb, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb), > + > + TP_STRUCT__entry( > + __field( void *, skbaddr ) > + ), > + > + TP_fast_assign( > + __entry->skbaddr = skb; > + ), > + > + TP_printk("skbaddr=%p", __entry->skbaddr) > + > +); > + > +DEFINE_EVENT(free_skb, consume_skb, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb) > + > +); > + > +DEFINE_EVENT(free_skb, dev_kfree_skb_irq, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb) > + > +); > + > +DEFINE_EVENT(free_skb, skb_free_datagram_locked, > + > + TP_PROTO(struct sk_buff *skb), > + > + TP_ARGS(skb) > + > +); > + Why create these last two tracepoints at all? dev_kfree_skb_irq will eventually pass through kfree_skb anyway, getting picked up by the tracepoint there, the while the latter won't (since it uses __kfree_skb instead), I think that could be fixed up by add a call to trace_kfree_skb there directly, saving you two tracepoints. 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
(2010/07/20 20:50), Neil Horman wrote: > On Tue, Jul 20, 2010 at 09:49:10AM +0900, Koki Sanagi wrote: >> [RFC PATCH v3 4/5] skb: add tracepoints to freeing skb >> This patch adds tracepoint to consume_skb, dev_kfree_skb_irq and >> skb_free_datagram_locked. Combinating with tracepoint on dev_hard_start_xmit, >> we can check how long it takes to free transmited packets. And using it, we can >> calculate how many packets driver had at that time. It is useful when a drop of >> transmited packet is a problem. >> >> <idle>-0 [001] 241409.218333: consume_skb: skbaddr=dd6b2fb8 >> <idle>-0 [001] 241409.490555: dev_kfree_skb_irq: skbaddr=f5e29840 >> >> udp-recv-302 [001] 515031.206008: skb_free_datagram_locked: skbaddr=f5b1d900 >> >> >> Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com> >> --- >> include/trace/events/skb.h | 42 ++++++++++++++++++++++++++++++++++++++++++ >> net/core/datagram.c | 1 + >> net/core/dev.c | 2 ++ >> net/core/skbuff.c | 1 + >> 4 files changed, 46 insertions(+), 0 deletions(-) >> >> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h >> index 4b2be6d..84c9041 100644 >> --- a/include/trace/events/skb.h >> +++ b/include/trace/events/skb.h >> @@ -35,6 +35,48 @@ TRACE_EVENT(kfree_skb, >> __entry->skbaddr, __entry->protocol, __entry->location) >> ); >> >> +DECLARE_EVENT_CLASS(free_skb, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb), >> + >> + TP_STRUCT__entry( >> + __field( void *, skbaddr ) >> + ), >> + >> + TP_fast_assign( >> + __entry->skbaddr = skb; >> + ), >> + >> + TP_printk("skbaddr=%p", __entry->skbaddr) >> + >> +); >> + >> +DEFINE_EVENT(free_skb, consume_skb, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb) >> + >> +); >> + >> +DEFINE_EVENT(free_skb, dev_kfree_skb_irq, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb) >> + >> +); >> + >> +DEFINE_EVENT(free_skb, skb_free_datagram_locked, >> + >> + TP_PROTO(struct sk_buff *skb), >> + >> + TP_ARGS(skb) >> + >> +); >> + > > Why create these last two tracepoints at all? dev_kfree_skb_irq will eventually > pass through kfree_skb anyway, getting picked up by the tracepoint there, the > while the latter won't (since it uses __kfree_skb instead), I think that could > be fixed up by add a call to trace_kfree_skb there directly, saving you two > tracepoints. > > Neil > I think dev_kfree_skb_irq isn't chased by trace_kfree_skb or trace_consume_skb completely. Because net_tx_action frees skb by __kfree_skb. So it is better to add trace_kfree_skb before it. skb_free_datagram_locked is same. Thanks, Koki Sanagi. >> > > -- 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
On Wed, Jul 21, 2010 at 04:02:57PM +0900, Koki Sanagi wrote: > (2010/07/20 20:50), Neil Horman wrote: > > On Tue, Jul 20, 2010 at 09:49:10AM +0900, Koki Sanagi wrote: > >> [RFC PATCH v3 4/5] skb: add tracepoints to freeing skb > >> This patch adds tracepoint to consume_skb, dev_kfree_skb_irq and > >> skb_free_datagram_locked. Combinating with tracepoint on dev_hard_start_xmit, > >> we can check how long it takes to free transmited packets. And using it, we can > >> calculate how many packets driver had at that time. It is useful when a drop of > >> transmited packet is a problem. > >> > >> <idle>-0 [001] 241409.218333: consume_skb: skbaddr=dd6b2fb8 > >> <idle>-0 [001] 241409.490555: dev_kfree_skb_irq: skbaddr=f5e29840 > >> > >> udp-recv-302 [001] 515031.206008: skb_free_datagram_locked: skbaddr=f5b1d900 > >> > >> > >> Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com> > >> --- > >> include/trace/events/skb.h | 42 ++++++++++++++++++++++++++++++++++++++++++ > >> net/core/datagram.c | 1 + > >> net/core/dev.c | 2 ++ > >> net/core/skbuff.c | 1 + > >> 4 files changed, 46 insertions(+), 0 deletions(-) > >> > >> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h > >> index 4b2be6d..84c9041 100644 > >> --- a/include/trace/events/skb.h > >> +++ b/include/trace/events/skb.h > >> @@ -35,6 +35,48 @@ TRACE_EVENT(kfree_skb, > >> __entry->skbaddr, __entry->protocol, __entry->location) > >> ); > >> > >> +DECLARE_EVENT_CLASS(free_skb, > >> + > >> + TP_PROTO(struct sk_buff *skb), > >> + > >> + TP_ARGS(skb), > >> + > >> + TP_STRUCT__entry( > >> + __field( void *, skbaddr ) > >> + ), > >> + > >> + TP_fast_assign( > >> + __entry->skbaddr = skb; > >> + ), > >> + > >> + TP_printk("skbaddr=%p", __entry->skbaddr) > >> + > >> +); > >> + > >> +DEFINE_EVENT(free_skb, consume_skb, > >> + > >> + TP_PROTO(struct sk_buff *skb), > >> + > >> + TP_ARGS(skb) > >> + > >> +); > >> + > >> +DEFINE_EVENT(free_skb, dev_kfree_skb_irq, > >> + > >> + TP_PROTO(struct sk_buff *skb), > >> + > >> + TP_ARGS(skb) > >> + > >> +); > >> + > >> +DEFINE_EVENT(free_skb, skb_free_datagram_locked, > >> + > >> + TP_PROTO(struct sk_buff *skb), > >> + > >> + TP_ARGS(skb) > >> + > >> +); > >> + > > > > Why create these last two tracepoints at all? dev_kfree_skb_irq will eventually > > pass through kfree_skb anyway, getting picked up by the tracepoint there, the > > while the latter won't (since it uses __kfree_skb instead), I think that could > > be fixed up by add a call to trace_kfree_skb there directly, saving you two > > tracepoints. > > > > Neil > > > I think dev_kfree_skb_irq isn't chased by trace_kfree_skb or trace_consume_skb > completely. Because net_tx_action frees skb by __kfree_skb. So it is better to > add trace_kfree_skb before it. skb_free_datagram_locked is same. > It isn't, you're right, but that was the point I made above. Those missed areas could be easily handled by adding calls to trace_kfree_skb which already exists, to the missed areas. Then you don't need to create those new tracepoints. The way your doing this, if someone wants to trace all skb frees in debugfs, they would have to enable three tracepoints, not just one. Not that thats the point of your patch, but its something to consider, and it simplifies your code. Neil > Thanks, > Koki Sanagi. > >> > > > > > > > -- 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
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h index 4b2be6d..84c9041 100644 --- a/include/trace/events/skb.h +++ b/include/trace/events/skb.h @@ -35,6 +35,48 @@ TRACE_EVENT(kfree_skb, __entry->skbaddr, __entry->protocol, __entry->location) ); +DECLARE_EVENT_CLASS(free_skb, + + TP_PROTO(struct sk_buff *skb), + + TP_ARGS(skb), + + TP_STRUCT__entry( + __field( void *, skbaddr ) + ), + + TP_fast_assign( + __entry->skbaddr = skb; + ), + + TP_printk("skbaddr=%p", __entry->skbaddr) + +); + +DEFINE_EVENT(free_skb, consume_skb, + + TP_PROTO(struct sk_buff *skb), + + TP_ARGS(skb) + +); + +DEFINE_EVENT(free_skb, dev_kfree_skb_irq, + + TP_PROTO(struct sk_buff *skb), + + TP_ARGS(skb) + +); + +DEFINE_EVENT(free_skb, skb_free_datagram_locked, + + TP_PROTO(struct sk_buff *skb), + + TP_ARGS(skb) + +); + TRACE_EVENT(skb_copy_datagram_iovec, TP_PROTO(const struct sk_buff *skb, int len), diff --git a/net/core/datagram.c b/net/core/datagram.c index f5b6f43..1ea32a0 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -231,6 +231,7 @@ void skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb) { bool slow; + trace_skb_free_datagram_locked(skb); if (likely(atomic_read(&skb->users) == 1)) smp_rmb(); else if (likely(!atomic_dec_and_test(&skb->users))) diff --git a/net/core/dev.c b/net/core/dev.c index 4acfec6..d979847 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -131,6 +131,7 @@ #include <linux/random.h> #include <trace/events/napi.h> #include <trace/events/net.h> +#include <trace/events/skb.h> #include <linux/pci.h> #include "net-sysfs.h" @@ -1581,6 +1582,7 @@ void dev_kfree_skb_irq(struct sk_buff *skb) struct softnet_data *sd; unsigned long flags; + trace_dev_kfree_skb_irq(skb); local_irq_save(flags); sd = &__get_cpu_var(softnet_data); skb->next = sd->completion_queue; diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 34432b4..a7b4036 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -466,6 +466,7 @@ void consume_skb(struct sk_buff *skb) smp_rmb(); else if (likely(!atomic_dec_and_test(&skb->users))) return; + trace_consume_skb(skb); __kfree_skb(skb); } EXPORT_SYMBOL(consume_skb);
[RFC PATCH v3 4/5] skb: add tracepoints to freeing skb This patch adds tracepoint to consume_skb, dev_kfree_skb_irq and skb_free_datagram_locked. Combinating with tracepoint on dev_hard_start_xmit, we can check how long it takes to free transmited packets. And using it, we can calculate how many packets driver had at that time. It is useful when a drop of transmited packet is a problem. <idle>-0 [001] 241409.218333: consume_skb: skbaddr=dd6b2fb8 <idle>-0 [001] 241409.490555: dev_kfree_skb_irq: skbaddr=f5e29840 udp-recv-302 [001] 515031.206008: skb_free_datagram_locked: skbaddr=f5b1d900 Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com> --- include/trace/events/skb.h | 42 ++++++++++++++++++++++++++++++++++++++++++ net/core/datagram.c | 1 + net/core/dev.c | 2 ++ net/core/skbuff.c | 1 + 4 files changed, 46 insertions(+), 0 deletions(-) -- 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