Re: [PATCH v2 net-next 0/3] net: skb: introduce kfree_skb_with_reason() and use it for tcp and udp

[Menglong Dong]

On Tue, Jan 4, 2022 at 11:09 AM Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote:
>
> On Mon, Jan 03, 2022 at 07:01:30PM -0700, David Ahern wrote:
> > On 1/3/22 6:47 PM, Cong Wang wrote:
> > > On Thu, Dec 30, 2021 at 05:32:37PM +0800, menglong8.dong@xxxxxxxxx wrote:
> > >> From: Menglong Dong <imagedong@xxxxxxxxxxx>
> > >>
> > >> In this series patch, the interface kfree_skb_with_reason() is
> > >> introduced(), which is used to collect skb drop reason, and pass
> > >> it to 'kfree_skb' tracepoint. Therefor, 'drop_monitor' or eBPF is
> > >> able to monitor abnormal skb with detail reason.
> > >>
> > >
> > > We already something close, __dev_kfree_skb_any(). Can't we unify
> > > all of these?
> >
> > Specifically?
> >
> > The 'reason' passed around by those is either SKB_REASON_CONSUMED or
> > SKB_REASON_DROPPED and is used to call kfree_skb vs consume_skb. i.e.,
> > this is unrelated to this patch set and goal.
>
> What prevents you extending it?
>

I think extending kfree_skb() with kfree_skb_reason() is more reasonable,
considering the goal of kfree_skb() and __dev_kfree_skb_any().

> >
> > >
> > >
> > >> In fact, this series patches are out of the intelligence of David
> > >> and Steve, I'm just a truck man :/
> > >>
> > >
> > > I think there was another discussion before yours, which I got involved
> > > as well.
> > >
> > >> Previous discussion is here:
> > >>
> > >> https://lore.kernel.org/netdev/20211118105752.1d46e990@xxxxxxxxxxxxxxxxxx/
> > >> https://lore.kernel.org/netdev/67b36bd8-2477-88ac-83a0-35a1eeaf40c9@xxxxxxxxx/
> > >>
> > >> In the first patch, kfree_skb_with_reason() is introduced and
> > >> the 'reason' field is added to 'kfree_skb' tracepoint. In the
> > >> second patch, 'kfree_skb()' in replaced with 'kfree_skb_with_reason()'
> > >> in tcp_v4_rcv(). In the third patch, 'kfree_skb_with_reason()' is
> > >> used in __udp4_lib_rcv().
> > >>
> > >
> > > I don't follow all the discussions here, but IIRC it would be nice
> > > if we can provide the SNMP stat code (for instance, TCP_MIB_CSUMERRORS) to
> > > user-space, because those stats are already exposed to user-space, so
> > > you don't have to invent new ones.
> >
> > Those SNMP macros are not unique and can not be fed into a generic
> > kfree_skb_reason function.
>
> Sure, you also have the skb itself, particularly skb protocol, with
> these combined, it should be unique.

I thought about it before, but it's hard to use the reason in SNMP
directly. First,
the stats of SNMP are grouped, and the same skb protocol can use stats in
different groups, which makes it hard to be unique. Second, SNMP is used to
do statistics, not only drop statistics, which is a little different
from the goal here.
Third, it's not flexible enough to extend the new drop reason.

Thanks!
Menglong Dong

>
> Thanks.