Re: [PATCH v4 net-next 1/7] net: add rx_sk to trace_kfree_skb

[Yan Zhai <yan@cloudflare.com>]

On Fri, Jun 14, 2024 at 5:15 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> 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<yan@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'.
>

Jesper explained to me in a private message that "addr" makes more
sense when there was no BPF, since likely nothing would dereference
the pointer anymore at that time, so it's purely an address. But it is
no longer the case now. Also, in later patches of this change, I am
already breaking the "convention" by replacing kfree_skb with
sk_skb_reason_drop, so how about breaking it once more, and just
calling it "rx_sk". I want to keep the "rx_" to emphasize this is a
receiving socket. Let me send an amended version early next week and
see if more thoughts come.

thanks
Yan


> Thanks,
>
> Paolo
>