Re: [PATCH RFC net-next] trace: tcp: Add tracepoint for tcp_cwnd_reduction()

[Breno Leitao]

On Mon, Jan 20, 2025 at 09:06:43PM +0800, Jason Xing wrote:
> On Mon, Jan 20, 2025 at 9:02 PM Breno Leitao <leitao@xxxxxxxxxx> wrote:
> > On Mon, Jan 20, 2025 at 08:08:52PM +0800, Jason Xing wrote:
> > > On Mon, Jan 20, 2025 at 8:03 PM Breno Leitao <leitao@xxxxxxxxxx> wrote:
> > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > > > index 4811727b8a02258ec6fa1fd129beecf7cbb0f90e..fc88c511e81bc12ec57e8dc3e9185a920d1bd079 100644
> > > > --- a/net/ipv4/tcp_input.c
> > > > +++ b/net/ipv4/tcp_input.c
> > > > @@ -2710,6 +2710,8 @@ void tcp_cwnd_reduction(struct sock *sk, int newly_acked_sacked, int newly_lost,
> > > >         if (newly_acked_sacked <= 0 || WARN_ON_ONCE(!tp->prior_cwnd))
> > > >                 return;
> > > >
> > > > +       trace_tcp_cwnd_reduction(sk, newly_acked_sacked, newly_lost, flag);
> > > > +
> > >
> > > Are there any other reasons why introducing a new tracepoint here?
> > > AFAIK, it can be easily replaced by a bpf related program or script to
> > > monitor in the above position.
> >
> > In which position exactly?
> 
> I meant, in the position where you insert a one-line tracepoint, which
> should be easily replaced with a bpf program (kprobe
> tcp_cwnd_reduction with two checks like in the earlier if-statement).
> It doesn't mean that I object to this new tracepoint, just curious if
> you have other motivations.

This is exactly the current implementation we have at Meta, as it relies on
hooking into this specific function. This approach is unstable, as
compiler optimizations like inlining can break the functionality.

This patch enhances the API's stability by introducing a guaranteed hook
point, allowing the compiler to make changes without disrupting the
BPF program's functionality.