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

From: Menglong Dong <menglong8.dong@gmail.com>
To: David Ahern <dsahern@gmail.com>, Steven Rostedt <rostedt@goodmis.org>
Cc: Jakub Kicinski <kuba@kernel.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: Sun, 21 Nov 2021 18:47:21 +0800	[thread overview]
Message-ID: <CADxym3bTScvYzpUzvz62zpUvqksbfW-f=JpCUHbEJCagjY6wuQ@mail.gmail.com> (raw)
In-Reply-To: <9ad07da4-8523-b861-6111-729b8d1d6d57@gmail.com>

On Fri, Nov 19, 2021 at 11:54 AM David Ahern <dsahern@gmail.com> wrote:
>
[...]
>
> 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
>

I have been thinking about this all day, and I think your words make sense.
Indeed, this can make use of the frame of the 'drop monitor' module of kernel
and the userspace tools of wireshark, dropwatch, etc. And this idea is more
suitable for the aim of 'get the reason for packet drop'. However, the
shortcoming
of this idea is that it can't reuse the drop reason for the 'snmp'
frame.

With creating a tracepoint for 'snmp', it can make use of the 'snmp' frame and
the modifications can be easier. However, it's not friendly to the
users, such as
dropwatch, wireshark, etc. And it seems it is a little redundant with what
the tracepoint for 'kfree_sbk()' do. However, I think it's not
difficult to develop
a userspace tool. In fact, I have already write a tool based on BCC, which is
able to make use of 'snmp' tracepoint, such as:

$ sudo ./nettrace.py --tracer snmp -p udp --addr 192.168.122.8
begin tracing......
785487.366412: [snmp][udplite_noports]: UDP: 192.168.122.8:35310 ->
192.168.122.1:7979

And it can monitor packet drop of udp with ip 192.168.122.8 (filter by port,
statistics type are supported too).

And maybe we can integrate tracepoint of  'snmp' into 'drop monitor' with
NET_DM_ATTR_SNMP, just link NET_DM_ATTR_SW_DROPS and
NET_DM_ATTR_HW_DROPS?

@Steven What do you think? I think I'm ok with both ideas, as my main target
is to get the reason for the packet drop. As for the idea of
'kfree_skb_with_reason', I'm just a little worry about if we can accept the
modification it brings in.

Thanks!
Menglong Dong

> 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.