| Message ID | 20211230093240.1125937-1-imagedong@tencent.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: skb: introduce kfree_skb_with_reason() and use it for tcp and udp | expand |
On Thu, Dec 30, 2021 at 05:32:37PM +0800, menglong8.dong@gmail.com wrote: > From: Menglong Dong <imagedong@tencent.com> > > 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? > 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@gandalf.local.home/ > https://lore.kernel.org/netdev/67b36bd8-2477-88ac-83a0-35a1eeaf40c9@gmail.com/ > > 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. Thanks.
On 1/3/22 6:47 PM, Cong Wang wrote: > On Thu, Dec 30, 2021 at 05:32:37PM +0800, menglong8.dong@gmail.com wrote: >> From: Menglong Dong <imagedong@tencent.com> >> >> 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. > > >> 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@gandalf.local.home/ >> https://lore.kernel.org/netdev/67b36bd8-2477-88ac-83a0-35a1eeaf40c9@gmail.com/ >> >> 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.
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@gmail.com wrote: > >> From: Menglong Dong <imagedong@tencent.com> > >> > >> 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? > > > > > > >> 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@gandalf.local.home/ > >> https://lore.kernel.org/netdev/67b36bd8-2477-88ac-83a0-35a1eeaf40c9@gmail.com/ > >> > >> 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. Thanks.
On Tue, Jan 4, 2022 at 11:09 AM Cong Wang <xiyou.wangcong@gmail.com> 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@gmail.com wrote: > > >> From: Menglong Dong <imagedong@tencent.com> > > >> > > >> 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@gandalf.local.home/ > > >> https://lore.kernel.org/netdev/67b36bd8-2477-88ac-83a0-35a1eeaf40c9@gmail.com/ > > >> > > >> 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.
From: Menglong Dong <imagedong@tencent.com> 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. In fact, this series patches are out of the intelligence of David and Steve, I'm just a truck man :/ Previous discussion is here: https://lore.kernel.org/netdev/20211118105752.1d46e990@gandalf.local.home/ https://lore.kernel.org/netdev/67b36bd8-2477-88ac-83a0-35a1eeaf40c9@gmail.com/ 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(). Changes since v1: - rename some drop reason, as David suggested - add the third patch Menglong Dong (3): net: skb: introduce kfree_skb_with_reason() net: skb: use kfree_skb_with_reason() in tcp_v4_rcv() net: skb: use kfree_skb_with_reason() in __udp4_lib_rcv() include/linux/skbuff.h | 18 +++++++++++++++++ include/trace/events/skb.h | 41 +++++++++++++++++++++++++++++++------- net/core/dev.c | 3 ++- net/core/drop_monitor.c | 10 +++++++--- net/core/skbuff.c | 22 +++++++++++++++++++- net/ipv4/tcp_ipv4.c | 14 ++++++++++--- net/ipv4/udp.c | 10 ++++++++-- 7 files changed, 101 insertions(+), 17 deletions(-)