On Tue, Aug 8, 2023 at 10:46 AM Manjusaka [off-list ref] wrote:

On 2023/8/8 16:26, Eric Dumazet wrote:

quoted

On Tue, Aug 8, 2023 at 7:59 AM Manjusaka [off-list ref] wrote:

quoted

In normal use case, the tcp_ca_event would be changed in high frequency.

It's a good indicator to represent the network quanlity.

quality ?

Honestly, it is more about TCP stack tracing than 'network quality'

quoted

So I propose to add a `tcp:tcp_ca_event` trace event
like `tcp:tcp_cong_state_set` to help the people to
trace the TCP connection status

Signed-off-by: Manjusaka <redacted>
---
 include/net/tcp.h          |  9 ++------
 include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++
 net/ipv4/tcp_cong.c        | 10 +++++++++
 3 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 0ca972ebd3dd..a68c5b61889c 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h

@@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk)
        return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN;
 }

-static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event)
-{
-       const struct inet_connection_sock *icsk = inet_csk(sk);
-
-       if (icsk->icsk_ca_ops->cwnd_event)
-               icsk->icsk_ca_ops->cwnd_event(sk, event);
-}
+/* from tcp_cong.c */
+void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event);

 /* From tcp_cong.c */
 void tcp_set_ca_state(struct sock *sk, const u8 ca_state);

diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index bf06db8d2046..b374eb636af9 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h

@@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set,
                  __entry->cong_state)
 );

+TRACE_EVENT(tcp_ca_event,
+
+       TP_PROTO(struct sock *sk, const u8 ca_event),
+
+       TP_ARGS(sk, ca_event),
+
+       TP_STRUCT__entry(
+               __field(const void *, skaddr)
+               __field(__u16, sport)
+               __field(__u16, dport)
+               __array(__u8, saddr, 4)
+               __array(__u8, daddr, 4)
+               __array(__u8, saddr_v6, 16)
+               __array(__u8, daddr_v6, 16)
+               __field(__u8, ca_event)
+       ),
+

Please add the family (look at commit 3dd344ea84e1 ("net: tracepoint:
exposing sk_family in all tcp:tracepoints"))

quoted

+       TP_fast_assign(
+               struct inet_sock *inet = inet_sk(sk);
+               __be32 *p32;
+
+               __entry->skaddr = sk;
+
+               __entry->sport = ntohs(inet->inet_sport);
+               __entry->dport = ntohs(inet->inet_dport);
+
+               p32 = (__be32 *) __entry->saddr;
+               *p32 = inet->inet_saddr;
+
+               p32 = (__be32 *) __entry->daddr;
+               *p32 =  inet->inet_daddr;

We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/

quoted

+
+               TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr,
+                          sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);

I will send a cleanup, because IP_STORE_ADDRS() should really take
care of all details.

quoted

+
+               __entry->ca_event = ca_event;
+       ),
+
+       TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%u",
+                 __entry->sport, __entry->dport,
+                 __entry->saddr, __entry->daddr,
+                 __entry->saddr_v6, __entry->daddr_v6,
+                 __entry->ca_event)

Please print the symbol instead of numeric ca_event.

Look at show_tcp_state_name() for instance.

Thanks for the kindness code review, I still get some issue here(Sorry for the first time to contribute):

1. > We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/

I'm not getting your means, would you mean that we should only save the IPv4 Address here?

2. > I will send a cleanup, because IP_STORE_ADDRS() should really take care of all details.

I think you will make the address assignment code in TP_fast_assign as a new function.

Should I submit the new change until you send the cleanup patch or I can make this in my patch(cleanup the address assignment)

Wait a bit, I am sending fixes today, so that no more copy/paste
duplicates the issues.