Re: [RFC net-next 1/6] net: add kfree_skb_for_sk function

[Eric Dumazet]

On Thu, May 30, 2024 at 11:46 PM Yan Zhai <yan@xxxxxxxxxxxxxx> wrote:
>
> Implement a new kfree_skb_for_sk to replace kfree_skb_reason on a few
> local receive path. The function accepts an extra receiving socket
> argument, which will be set in skb->cb for kfree_skb/consume_skb
> tracepoint consumption. With this extra bit of information, it will be
> easier to attribute dropped packets to netns/containers and
> sockets/services for performance and error monitoring purpose.

This is a lot of code churn...

I have to ask : Why not simply adding an sk parameter to an existing
trace point ?

If this not possible, I would rather add new tracepoints, adding new classes,
because it will ease your debugging :

When looking for TCP drops, simply use a tcp_event_sk_skb_reason instance,
and voila, no distractions caused by RAW/ICMP/ICMPv6/af_packet drops.

DECLARE_EVENT_CLASS(tcp_event_sk_skb_reason,

     TP_PROTO(const struct sock *sk, const struct sk_buff *skb, enum
skb_drop_reason reason),
..
);

Also, the name ( kfree_skb_for_sk) and order of parameters is confusing.

I always prefer this kind of ordering/names :

void sk_skb_reason_drop( [struct net *net ] // not relevant here, but
to expand the rationale
              struct sock *sk, struct sk_buff *skb, enum skb_drop_reason reason)

Looking at the name, we immediately see the parameter order.

The consume one (no @reason there) would be called

void sk_skb_consume(struct sock *sk, struct sk_buff *skb);