Message ID | 20211230093240.1125937-2-imagedong@tencent.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: skb: introduce kfree_skb_with_reason() and use it for tcp and udp | expand |
On Thu, 30 Dec 2021 17:32:38 +0800 menglong8.dong@gmail.com wrote: > From: Menglong Dong <imagedong@tencent.com> > > Introduce the interface kfree_skb_with_reason(), which is used to pass > the reason why the skb is dropped to 'kfree_skb' tracepoint. > > Add the 'reason' field to 'trace_kfree_skb', therefor user can get > more detail information about abnormal skb with 'drop_monitor' or > eBPF. > void skb_release_head_state(struct sk_buff *skb); > void kfree_skb(struct sk_buff *skb); Should this be turned into a static inline calling kfree_skb_with_reason() now? BTW you should drop the '_with'. > +void kfree_skb_with_reason(struct sk_buff *skb, > + enum skb_drop_reason reason); continuation line is unaligned, please try checkpatch > void kfree_skb_list(struct sk_buff *segs); > void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt); > void skb_tx_error(struct sk_buff *skb); > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h > index 9e92f22eb086..cab1c08a30cd 100644 > --- a/include/trace/events/skb.h > +++ b/include/trace/events/skb.h > @@ -9,29 +9,51 @@ > #include <linux/netdevice.h> > #include <linux/tracepoint.h> > > +#define TRACE_SKB_DROP_REASON \ > + EM(SKB_DROP_REASON_NOT_SPECIFIED, NOT_SPECIFIED) \ > + EMe(SKB_DROP_REASON_MAX, HAHA_MAX) HAHA_MAX ? > + > +#undef EM > +#undef EMe > + > +#define EM(a, b) TRACE_DEFINE_ENUM(a); > +#define EMe(a, b) TRACE_DEFINE_ENUM(a); > + > +TRACE_SKB_DROP_REASON > + > +#undef EM > +#undef EMe > +#define EM(a, b) { a, #b }, > +#define EMe(a, b) { a, #b } > + > + double new line > /* > * Tracepoint for free an sk_buff: > */ > TRACE_EVENT(kfree_skb, > > - TP_PROTO(struct sk_buff *skb, void *location), > + TP_PROTO(struct sk_buff *skb, void *location, > + enum skb_drop_reason reason), > > - TP_ARGS(skb, location), > + TP_ARGS(skb, location, reason), > > TP_STRUCT__entry( > - __field( void *, skbaddr ) > - __field( void *, location ) > - __field( unsigned short, protocol ) > + __field(void *, skbaddr) > + __field(void *, location) > + __field(unsigned short, protocol) > + __field(enum skb_drop_reason, reason) > ), > > TP_fast_assign( > __entry->skbaddr = skb; > __entry->location = location; > __entry->protocol = ntohs(skb->protocol); > + __entry->reason = reason; > ), > > - TP_printk("skbaddr=%p protocol=%u location=%p", > - __entry->skbaddr, __entry->protocol, __entry->location) > + TP_printk("skbaddr=%p protocol=%u location=%p reason: %s", > + __entry->skbaddr, __entry->protocol, __entry->location, > + __print_symbolic(__entry->reason, TRACE_SKB_DROP_REASON)) > ); > > TRACE_EVENT(consume_skb, > diff --git a/net/core/dev.c b/net/core/dev.c > index 644b9c8be3a8..9464dbf9e3d6 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4899,7 +4899,8 @@ static __latent_entropy void net_tx_action(struct softirq_action *h) > if (likely(get_kfree_skb_cb(skb)->reason == SKB_REASON_CONSUMED)) > trace_consume_skb(skb); > else > - trace_kfree_skb(skb, net_tx_action); > + trace_kfree_skb(skb, net_tx_action, > + SKB_DROP_REASON_NOT_SPECIFIED); > > if (skb->fclone != SKB_FCLONE_UNAVAILABLE) > __kfree_skb(skb); > diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c > index 3d0ab2eec916..7b288a121a41 100644 > --- a/net/core/drop_monitor.c > +++ b/net/core/drop_monitor.c > @@ -110,7 +110,8 @@ static u32 net_dm_queue_len = 1000; > > struct net_dm_alert_ops { > void (*kfree_skb_probe)(void *ignore, struct sk_buff *skb, > - void *location); > + void *location, > + enum skb_drop_reason reason); > void (*napi_poll_probe)(void *ignore, struct napi_struct *napi, > int work, int budget); > void (*work_item_func)(struct work_struct *work); > @@ -262,7 +263,9 @@ static void trace_drop_common(struct sk_buff *skb, void *location) > spin_unlock_irqrestore(&data->lock, flags); > } > > -static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location) > +static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, > + void *location, > + enum skb_drop_reason reason) > { > trace_drop_common(skb, location); > } > @@ -490,7 +493,8 @@ static const struct net_dm_alert_ops net_dm_alert_summary_ops = { > > static void net_dm_packet_trace_kfree_skb_hit(void *ignore, > struct sk_buff *skb, > - void *location) > + void *location, > + enum skb_drop_reason reason) > { > ktime_t tstamp = ktime_get_real(); > struct per_cpu_dm_data *data; > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 275f7b8416fe..570dc022a8a1 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -770,11 +770,31 @@ void kfree_skb(struct sk_buff *skb) > if (!skb_unref(skb)) > return; > > - trace_kfree_skb(skb, __builtin_return_address(0)); > + trace_kfree_skb(skb, __builtin_return_address(0), > + SKB_DROP_REASON_NOT_SPECIFIED); > __kfree_skb(skb); > } > EXPORT_SYMBOL(kfree_skb); > > +/** > + * kfree_skb_with_reason - free an sk_buff with reason > + * @skb: buffer to free > + * @reason: reason why this skb is dropped > + * > + * The same as kfree_skb() except that this function will pass > + * the drop reason to 'kfree_skb' tracepoint. > + */ > +void kfree_skb_with_reason(struct sk_buff *skb, > + enum skb_drop_reason reason) > +{ > + if (!skb_unref(skb)) > + return; > + > + trace_kfree_skb(skb, __builtin_return_address(0), reason); > + __kfree_skb(skb); > +} > +EXPORT_SYMBOL(kfree_skb_with_reason); > + > void kfree_skb_list(struct sk_buff *segs) > { > while (segs) {
On Fri, Dec 31, 2021 at 9:26 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 30 Dec 2021 17:32:38 +0800 menglong8.dong@gmail.com wrote: > > From: Menglong Dong <imagedong@tencent.com> > > > > Introduce the interface kfree_skb_with_reason(), which is used to pass > > the reason why the skb is dropped to 'kfree_skb' tracepoint. > > > > Add the 'reason' field to 'trace_kfree_skb', therefor user can get > > more detail information about abnormal skb with 'drop_monitor' or > > eBPF. > > > void skb_release_head_state(struct sk_buff *skb); > > void kfree_skb(struct sk_buff *skb); > > Should this be turned into a static inline calling > kfree_skb_with_reason() now? BTW you should drop the > '_with'. > I thought about it before, but I'm a little afraid that some users may trace kfree_skb() with kprobe, making it inline may not be friendly to them? > > +void kfree_skb_with_reason(struct sk_buff *skb, > > + enum skb_drop_reason reason); > > continuation line is unaligned, please try checkpatch > > > void kfree_skb_list(struct sk_buff *segs); > > void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt); > > void skb_tx_error(struct sk_buff *skb); > > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h > > index 9e92f22eb086..cab1c08a30cd 100644 > > --- a/include/trace/events/skb.h > > +++ b/include/trace/events/skb.h > > @@ -9,29 +9,51 @@ > > #include <linux/netdevice.h> > > #include <linux/tracepoint.h> > > > > +#define TRACE_SKB_DROP_REASON \ > > + EM(SKB_DROP_REASON_NOT_SPECIFIED, NOT_SPECIFIED) \ > > + EMe(SKB_DROP_REASON_MAX, HAHA_MAX) > > HAHA_MAX ? Enn......WOW_MAX? Just kidding, I'll make it 'MAX' (or remove this line, as it won't be used). > > > + > > +#undef EM > > +#undef EMe > > + > > +#define EM(a, b) TRACE_DEFINE_ENUM(a); > > +#define EMe(a, b) TRACE_DEFINE_ENUM(a); > > + > > +TRACE_SKB_DROP_REASON > > + > > +#undef EM > > +#undef EMe > > +#define EM(a, b) { a, #b }, > > +#define EMe(a, b) { a, #b } > > + > > + > > double new line Get it! Thanks~ Menglong Dong > > > /* > > * Tracepoint for free an sk_buff: > > */ > > TRACE_EVENT(kfree_skb, > > > > - TP_PROTO(struct sk_buff *skb, void *location), > > + TP_PROTO(struct sk_buff *skb, void *location, > > + enum skb_drop_reason reason), > > > > - TP_ARGS(skb, location), > > + TP_ARGS(skb, location, reason), > > > > TP_STRUCT__entry( > > - __field( void *, skbaddr ) > > - __field( void *, location ) > > - __field( unsigned short, protocol ) > > + __field(void *, skbaddr) > > + __field(void *, location) > > + __field(unsigned short, protocol) > > + __field(enum skb_drop_reason, reason) > > ), > > > > TP_fast_assign( > > __entry->skbaddr = skb; > > __entry->location = location; > > __entry->protocol = ntohs(skb->protocol); > > + __entry->reason = reason; > > ), > > > > - TP_printk("skbaddr=%p protocol=%u location=%p", > > - __entry->skbaddr, __entry->protocol, __entry->location) > > + TP_printk("skbaddr=%p protocol=%u location=%p reason: %s", > > + __entry->skbaddr, __entry->protocol, __entry->location, > > + __print_symbolic(__entry->reason, TRACE_SKB_DROP_REASON)) > > ); > > > > TRACE_EVENT(consume_skb, > > diff --git a/net/core/dev.c b/net/core/dev.c > > index 644b9c8be3a8..9464dbf9e3d6 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -4899,7 +4899,8 @@ static __latent_entropy void net_tx_action(struct softirq_action *h) > > if (likely(get_kfree_skb_cb(skb)->reason == SKB_REASON_CONSUMED)) > > trace_consume_skb(skb); > > else > > - trace_kfree_skb(skb, net_tx_action); > > + trace_kfree_skb(skb, net_tx_action, > > + SKB_DROP_REASON_NOT_SPECIFIED); > > > > if (skb->fclone != SKB_FCLONE_UNAVAILABLE) > > __kfree_skb(skb); > > diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c > > index 3d0ab2eec916..7b288a121a41 100644 > > --- a/net/core/drop_monitor.c > > +++ b/net/core/drop_monitor.c > > @@ -110,7 +110,8 @@ static u32 net_dm_queue_len = 1000; > > > > struct net_dm_alert_ops { > > void (*kfree_skb_probe)(void *ignore, struct sk_buff *skb, > > - void *location); > > + void *location, > > + enum skb_drop_reason reason); > > void (*napi_poll_probe)(void *ignore, struct napi_struct *napi, > > int work, int budget); > > void (*work_item_func)(struct work_struct *work); > > @@ -262,7 +263,9 @@ static void trace_drop_common(struct sk_buff *skb, void *location) > > spin_unlock_irqrestore(&data->lock, flags); > > } > > > > -static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location) > > +static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, > > + void *location, > > + enum skb_drop_reason reason) > > { > > trace_drop_common(skb, location); > > } > > @@ -490,7 +493,8 @@ static const struct net_dm_alert_ops net_dm_alert_summary_ops = { > > > > static void net_dm_packet_trace_kfree_skb_hit(void *ignore, > > struct sk_buff *skb, > > - void *location) > > + void *location, > > + enum skb_drop_reason reason) > > { > > ktime_t tstamp = ktime_get_real(); > > struct per_cpu_dm_data *data; > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 275f7b8416fe..570dc022a8a1 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -770,11 +770,31 @@ void kfree_skb(struct sk_buff *skb) > > if (!skb_unref(skb)) > > return; > > > > - trace_kfree_skb(skb, __builtin_return_address(0)); > > + trace_kfree_skb(skb, __builtin_return_address(0), > > + SKB_DROP_REASON_NOT_SPECIFIED); > > __kfree_skb(skb); > > } > > EXPORT_SYMBOL(kfree_skb); > > > > +/** > > + * kfree_skb_with_reason - free an sk_buff with reason > > + * @skb: buffer to free > > + * @reason: reason why this skb is dropped > > + * > > + * The same as kfree_skb() except that this function will pass > > + * the drop reason to 'kfree_skb' tracepoint. > > + */ > > +void kfree_skb_with_reason(struct sk_buff *skb, > > + enum skb_drop_reason reason) > > +{ > > + if (!skb_unref(skb)) > > + return; > > + > > + trace_kfree_skb(skb, __builtin_return_address(0), reason); > > + __kfree_skb(skb); > > +} > > +EXPORT_SYMBOL(kfree_skb_with_reason); > > + > > void kfree_skb_list(struct sk_buff *segs) > > { > > while (segs) { >
On Fri, 31 Dec 2021 14:35:31 +0800 Menglong Dong wrote: > > > void skb_release_head_state(struct sk_buff *skb); > > > void kfree_skb(struct sk_buff *skb); > > > > Should this be turned into a static inline calling > > kfree_skb_with_reason() now? BTW you should drop the > > '_with'. > > > > I thought about it before, but I'm a little afraid that some users may trace > kfree_skb() with kprobe, making it inline may not be friendly to them? Hm, there is a bpf sample which does that, but that's probably not commonly used given there is a tracepoint. If someone is using a kprobe they can switch to kprobing kfree_skb*reason().
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index aa9d42724e20..3620b3ff2154 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -305,6 +305,17 @@ struct sk_buff_head { struct sk_buff; +/* The reason of skb drop, which is used in kfree_skb_with_reason(). + * en...maybe they should be splited by group? + * + * Each item here should also be in 'TRACE_SKB_DROP_REASON', which is + * used to translate the reason to string. + */ +enum skb_drop_reason { + SKB_DROP_REASON_NOT_SPECIFIED, + SKB_DROP_REASON_MAX, +}; + /* To allow 64K frame to be packed as single skb without frag_list we * require 64K/PAGE_SIZE pages plus 1 additional page to allow for * buffers which do not start on a page boundary. @@ -1087,6 +1098,8 @@ static inline bool skb_unref(struct sk_buff *skb) void skb_release_head_state(struct sk_buff *skb); void kfree_skb(struct sk_buff *skb); +void kfree_skb_with_reason(struct sk_buff *skb, + enum skb_drop_reason reason); void kfree_skb_list(struct sk_buff *segs); void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt); void skb_tx_error(struct sk_buff *skb); diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h index 9e92f22eb086..cab1c08a30cd 100644 --- a/include/trace/events/skb.h +++ b/include/trace/events/skb.h @@ -9,29 +9,51 @@ #include <linux/netdevice.h> #include <linux/tracepoint.h> +#define TRACE_SKB_DROP_REASON \ + EM(SKB_DROP_REASON_NOT_SPECIFIED, NOT_SPECIFIED) \ + EMe(SKB_DROP_REASON_MAX, HAHA_MAX) + +#undef EM +#undef EMe + +#define EM(a, b) TRACE_DEFINE_ENUM(a); +#define EMe(a, b) TRACE_DEFINE_ENUM(a); + +TRACE_SKB_DROP_REASON + +#undef EM +#undef EMe +#define EM(a, b) { a, #b }, +#define EMe(a, b) { a, #b } + + /* * Tracepoint for free an sk_buff: */ TRACE_EVENT(kfree_skb, - TP_PROTO(struct sk_buff *skb, void *location), + TP_PROTO(struct sk_buff *skb, void *location, + enum skb_drop_reason reason), - TP_ARGS(skb, location), + TP_ARGS(skb, location, reason), TP_STRUCT__entry( - __field( void *, skbaddr ) - __field( void *, location ) - __field( unsigned short, protocol ) + __field(void *, skbaddr) + __field(void *, location) + __field(unsigned short, protocol) + __field(enum skb_drop_reason, reason) ), TP_fast_assign( __entry->skbaddr = skb; __entry->location = location; __entry->protocol = ntohs(skb->protocol); + __entry->reason = reason; ), - TP_printk("skbaddr=%p protocol=%u location=%p", - __entry->skbaddr, __entry->protocol, __entry->location) + TP_printk("skbaddr=%p protocol=%u location=%p reason: %s", + __entry->skbaddr, __entry->protocol, __entry->location, + __print_symbolic(__entry->reason, TRACE_SKB_DROP_REASON)) ); TRACE_EVENT(consume_skb, diff --git a/net/core/dev.c b/net/core/dev.c index 644b9c8be3a8..9464dbf9e3d6 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4899,7 +4899,8 @@ static __latent_entropy void net_tx_action(struct softirq_action *h) if (likely(get_kfree_skb_cb(skb)->reason == SKB_REASON_CONSUMED)) trace_consume_skb(skb); else - trace_kfree_skb(skb, net_tx_action); + trace_kfree_skb(skb, net_tx_action, + SKB_DROP_REASON_NOT_SPECIFIED); if (skb->fclone != SKB_FCLONE_UNAVAILABLE) __kfree_skb(skb); diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index 3d0ab2eec916..7b288a121a41 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -110,7 +110,8 @@ static u32 net_dm_queue_len = 1000; struct net_dm_alert_ops { void (*kfree_skb_probe)(void *ignore, struct sk_buff *skb, - void *location); + void *location, + enum skb_drop_reason reason); void (*napi_poll_probe)(void *ignore, struct napi_struct *napi, int work, int budget); void (*work_item_func)(struct work_struct *work); @@ -262,7 +263,9 @@ static void trace_drop_common(struct sk_buff *skb, void *location) spin_unlock_irqrestore(&data->lock, flags); } -static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location) +static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, + void *location, + enum skb_drop_reason reason) { trace_drop_common(skb, location); } @@ -490,7 +493,8 @@ static const struct net_dm_alert_ops net_dm_alert_summary_ops = { static void net_dm_packet_trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, - void *location) + void *location, + enum skb_drop_reason reason) { ktime_t tstamp = ktime_get_real(); struct per_cpu_dm_data *data; diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 275f7b8416fe..570dc022a8a1 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -770,11 +770,31 @@ void kfree_skb(struct sk_buff *skb) if (!skb_unref(skb)) return; - trace_kfree_skb(skb, __builtin_return_address(0)); + trace_kfree_skb(skb, __builtin_return_address(0), + SKB_DROP_REASON_NOT_SPECIFIED); __kfree_skb(skb); } EXPORT_SYMBOL(kfree_skb); +/** + * kfree_skb_with_reason - free an sk_buff with reason + * @skb: buffer to free + * @reason: reason why this skb is dropped + * + * The same as kfree_skb() except that this function will pass + * the drop reason to 'kfree_skb' tracepoint. + */ +void kfree_skb_with_reason(struct sk_buff *skb, + enum skb_drop_reason reason) +{ + if (!skb_unref(skb)) + return; + + trace_kfree_skb(skb, __builtin_return_address(0), reason); + __kfree_skb(skb); +} +EXPORT_SYMBOL(kfree_skb_with_reason); + void kfree_skb_list(struct sk_buff *segs) { while (segs) {