Re: [PATCH net-next v2 3/3] tcp: add location into reset trace process

From: Jason Xing <kerneljasonxing@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>,
	edumazet@google.com, mhiramat@kernel.org,
	 mathieu.desnoyers@efficios.com, rostedt@goodmis.org,
	davem@davemloft.net,  netdev@vger.kernel.org,
	linux-trace-kernel@vger.kernel.org,
	 Jason Xing <kernelxing@tencent.com>
Subject: Re: [PATCH net-next v2 3/3] tcp: add location into reset trace process
Date: Fri, 29 Mar 2024 10:53:06 +0800	[thread overview]
Message-ID: <CAL+tcoCF=tndnFQdBK_7r5KssyaHs+nDf3qmDbPgy5wJFvTe+g@mail.gmail.com> (raw)
In-Reply-To: <20240328181530.0d9229cb@kernel.org>

On Fri, Mar 29, 2024 at 9:15 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 26 Mar 2024 12:08:01 +0100 Paolo Abeni wrote:
> > > -   TP_PROTO(const struct sock *sk, const struct sk_buff *skb),
> > > +   TP_PROTO(
> > > +           const struct sock *sk,
> > > +           const struct sk_buff *skb,
> > > +           void *location),
> >
> > Very minor nit: the above lines should be aligned with the open
> > bracket.
>
> Yes, and a very odd way of breaking it up. Empty line after ( but
> ) not on a separate line.

After I blamed the history, maybe I should follow the format like
TRACE_EVENT(netfs_read)?

>
> > No need to repost just for this, but let's wait for Eric's feedback.
>
> Erring on the side of caution I'd read this:
> https://lore.kernel.org/all/CANn89iKK-qPhQ91Sq8rR_=KDWajnY2=Et2bUjDsgoQK4wxFOHw@mail.gmail.com/
> as lukewarm towards tp changes. Please repost if you think otherwise
> (with the formatting fixed)

Yes, I will repost it. I'm not introducing a controversial new tracepoint.

This patch is not only about whether we should use 'old-way' tracing
but about the tracepoint of this tcp reset that is not complete. Some
admins could use bpf to capture RST behaviours through hooking this
tracepoint which is not right currently apparently.

Besides, I simply tested the performance between using tracing and bpf
to monitor the fast path (like __tcp_transmit_skb()) on my loopback. I
saw at least 12% degradation with BPF used. So the advantage of trace
is obvious even though nowadays it is considered as an old school
method.

Thanks,
Jason

> --
> pw-bot: cr