On Tue, Jan 21, 2025 at 9:15 AM Jason Xing [off-list ref] wrote:

On Mon, Jan 20, 2025 at 9:20 PM Breno Leitao [off-list ref] wrote:

quoted

On Mon, Jan 20, 2025 at 09:06:43PM +0800, Jason Xing wrote:

quoted

On Mon, Jan 20, 2025 at 9:02 PM Breno Leitao [off-list ref] wrote:

quoted

On Mon, Jan 20, 2025 at 08:08:52PM +0800, Jason Xing wrote:

quoted

On Mon, Jan 20, 2025 at 8:03 PM Breno Leitao [off-list ref] wrote:

quoted

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.

Surely it does :) The reason why I asked is that perhaps one year ago
I'm trying to add many tracepoints so that the user space monitor can
take advantage of these various stable hook points. I believe that
there are many other places which might be inlined because of gcc,
receiving a few similar reports in recent months, but there is no
guarantee to not touch some functions which obviously break some
monitor applications.

Before BPF gets widely used, changing function names will not hurt
applications. Things are a little bit different now, changing names or
inlined function will lead the monitor application adjusting codes
through versions, because they are not kabi functions.

Thanks,
Jason