On Wed, 2024-06-12 at 09:59 +0200, Jesper Dangaard Brouer wrote: > > On 11/06/2024 22.11, Yan Zhai wrote: > > skb does not include enough information to find out receiving > > sockets/services and netns/containers on packet drops. In theory > > skb->dev tells about netns, but it can get cleared/reused, e.g. by TCP > > stack for OOO packet lookup. Similarly, skb->sk often identifies a local > > sender, and tells nothing about a receiver. > > > > Allow passing an extra receiving socket to the tracepoint to improve > > the visibility on receiving drops. > > > > Signed-off-by: Yan Zhai<y...@cloudflare.com> > > --- > > v3->v4: adjusted the TP_STRUCT field order to be consistent > > v2->v3: fixed drop_monitor function prototype > > --- > > include/trace/events/skb.h | 11 +++++++---- > > net/core/dev.c | 2 +- > > net/core/drop_monitor.c | 9 ++++++--- > > net/core/skbuff.c | 2 +- > > 4 files changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h > > index 07e0715628ec..3e9ea1cca6f2 100644 > > --- a/include/trace/events/skb.h > > +++ b/include/trace/events/skb.h > > @@ -24,13 +24,14 @@ DEFINE_DROP_REASON(FN, FN) > > TRACE_EVENT(kfree_skb, > > > > TP_PROTO(struct sk_buff *skb, void *location, > > - enum skb_drop_reason reason), > > + enum skb_drop_reason reason, struct sock *rx_sk), > > > > - TP_ARGS(skb, location, reason), > > + TP_ARGS(skb, location, reason, rx_sk), > > > > TP_STRUCT__entry( > > __field(void *, skbaddr) > > __field(void *, location) > > + __field(void *, rx_skaddr) > > Is there any reason for appending the "addr" part to "rx_sk" ? > It makes it harder to read this is the sk (socket). > > AFAICR the skbaddr naming is a legacy thing.
I'm double-minded about the above: I can see your point, but on the flip side the 'addr' suffix is consistently used in net-related tracepoints. > > > __field(unsigned short, protocol) > > __field(enum skb_drop_reason, reason) > > ), > > @@ -38,12 +39,14 @@ TRACE_EVENT(kfree_skb, > > TP_fast_assign( > > __entry->skbaddr = skb; > > __entry->location = location; > > + __entry->rx_skaddr = rx_sk; > > __entry->protocol = ntohs(skb->protocol); > > __entry->reason = reason; > > ), > > > > - TP_printk("skbaddr=%p protocol=%u location=%pS reason: %s", > > - __entry->skbaddr, __entry->protocol, __entry->location, > > + TP_printk("skbaddr=%p rx_skaddr=%p protocol=%u location=%pS reason: %s", > ^^^^^^^^^ > I find it hard to visually tell skbaddr and rx_skaddr apart. > And especially noticing the "skb" vs "sk" part of the string. I agree 'rx_skaddr' is sub-optimal. Either be consistent with all the other net tracepoints and use 'skaddr' (which will very likely will increase Jesper concerns, but I personally have no problem with such format) or prefer readability with something alike 'rx_sk' or (even better) 'sk'. Thanks, Paolo