Re: [PATCH v2 net-next 0/2] net: snmp: tracepoint support for snmp

From: David Ahern <dsahern@gmail.com>
To: Menglong Dong <menglong8.dong@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	David Miller <davem@davemloft.net>,
	mingo@redhat.com, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	dsahern@kernel.org, Menglong Dong <imagedong@tencent.com>,
	Yuchung Cheng <ycheng@google.com>,
	kuniyu@amazon.co.jp, LKML <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH v2 net-next 0/2] net: snmp: tracepoint support for snmp
Date: Thu, 18 Nov 2021 20:54:26 -0700	[thread overview]
Message-ID: <9ad07da4-8523-b861-6111-729b8d1d6d57@gmail.com> (raw)
In-Reply-To: <CADxym3ZfBVAecK-oFdMVV2gkOV6iUrq5XGkRZx3yXCuXDOS=2A@mail.gmail.com>

On 11/18/21 8:45 PM, Menglong Dong wrote:
> Hello~
> 
> On Thu, Nov 18, 2021 at 11:36 PM David Ahern <dsahern@gmail.com> wrote:
>>
> [...]
>>
>> there is already good infrastructure around kfree_skb - e.g., drop watch
>> monitor. Why not extend that in a way that other drop points can benefit
>> over time?
>>
> 
> Thanks for your advice.
> 
> In fact, I don't think that this is a perfect idea. This way may have benefit
> of reuse the existing kfree_skb event, but this will do plentiful modification
> to the current code. For example, in tcp_v4_rcv(), you need to introduce the
> new variate 'int free_reason' and record the drop reason in it, and pass
> it to 'kfree_skb_with_reason()' in 'discard_it:'. Many places need this kind
> modification. What's more, some statistics don't use 'kfree_skb()'.
> 
> However, with the tracepoint for snmp, we just need to pass 'skb' to
> 'UDP_INC_STATS()/TCP_INC_STATS()', the reason is already included.
> This way, the modification is more simple and easier to maintain.
> 

But it integrates into existing tooling which is a big win.

Ido gave the references for his work:
https://github.com/nhorman/dropwatch/pull/11
https://github.com/nhorman/dropwatch/commit/199440959a288dd97e3b7ae701d4e78968cddab7

And the Wireshark dissector is also upstream:
https://github.com/wireshark/wireshark/commit/a94a860c0644ec3b8a129fd243674a2e376ce1c8

i.e., the skb is already pushed to userspace for packet analysis. You
would just be augmenting more metadata along with it and not reinventing
all of this for just snmp counter based drops.