[PATCH v4] tracepoint: add new `tcp:tcp_ca_event` trace event

[PATCH v4] tracepoint: add new `tcp:tcp_ca_event` trace event

[Zheao Li]

Hello 

This the 4th version of the patch, the previous discusstion is here

https://lore.kernel.org/linux-trace-kernel/20230807183308.9015-1-me@manjusaka.me/

In this version of the code, here's some different:

1. The event name has been changed from `tcp_ca_event_set` to
`tcp_ca_event`

2. Output the current protocol family in TP_printk

3. Show the ca_event symbol instead of the original number

But the `./scripts/checkpatch.pl` has been failed to check this patch,
because we sill have some code error in ./scripts/checkpatch.pl(in
another world, the test would be failed when we use the 
scripts/checkpatch.pl to check the events/tcp.h

Feel free to ask me if you have have any issues and ideas.

Thanks

---

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

The developer can monitor the network quality more easier by tracing
TCP stack with this TP event.

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: Zheao Li <me@manjusaka.me>
---
 include/net/tcp.h          |  9 ++----
 include/trace/events/tcp.h | 60 ++++++++++++++++++++++++++++++++++++++
 net/ipv4/tcp_cong.c        | 10 +++++++
 3 files changed, 72 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 7b1ddffa3dfc..993eb00403ea 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -41,6 +41,18 @@
 	TP_STORE_V4MAPPED(__entry, saddr, daddr)
 #endif
 
+/* The TCP CA event traced by tcp_ca_event*/
+#define tcp_ca_event_names    \
+		EM(CA_EVENT_TX_START)     \
+		EM(CA_EVENT_CWND_RESTART) \
+		EM(CA_EVENT_COMPLETE_CWR) \
+		EM(CA_EVENT_LOSS)         \
+		EM(CA_EVENT_ECN_NO_CE)    \
+		EMe(CA_EVENT_ECN_IS_CE)
+
+#define show_tcp_ca_event_names(val) \
+	__print_symbolic(val, tcp_ca_event_names)
+
 /*
  * tcp event with arguments sk and skb
  *
@@ -419,6 +431,54 @@ 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)
+		__field(__u16, family)
+		__array(__u8, saddr, 4)
+		__array(__u8, daddr, 4)
+		__array(__u8, saddr_v6, 16)
+		__array(__u8, daddr_v6, 16)
+		__field(__u8, ca_event)
+	),
+
+	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);
+		__entry->family = sk->sk_family;
+
+		p32 = (__be32 *) __entry->saddr;
+		*p32 = inet->inet_saddr;
+
+		p32 = (__be32 *) __entry->daddr;
+		*p32 =  inet->inet_daddr;
+
+		TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr,
+			   sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);
+
+		__entry->ca_event = ca_event;
+	),
+
+	TP_printk("family=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%s",
+		  show_family_name(__entry->family),
+		  __entry->sport, __entry->dport,
+		  __entry->saddr, __entry->daddr,
+		  __entry->saddr_v6, __entry->daddr_v6,
+		  show_tcp_ca_event_names(__entry->ca_event))
+);
+
 #endif /* _TRACE_TCP_H */
 
 /* This part must be outside protection */
diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index 1b34050a7538..fb7ec6ebbbd0 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -34,6 +34,16 @@ struct tcp_congestion_ops *tcp_ca_find(const char *name)
 	return NULL;
 }
 
+void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event)
+{
+	const struct inet_connection_sock *icsk = inet_csk(sk);
+
+	trace_tcp_ca_event(sk, (u8)event);
+
+	if (icsk->icsk_ca_ops->cwnd_event)
+		icsk->icsk_ca_ops->cwnd_event(sk, event);
+}
+
 void tcp_set_ca_state(struct sock *sk, const u8 ca_state)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
-- 
2.34.1

Re: [PATCH v4] tracepoint: add new `tcp:tcp_ca_event` trace event

[Jakub Kicinski]

On Fri, 25 Aug 2023 13:32:47 +0000 Zheao Li wrote:
> This the 4th version of the patch, the previous discusstion is here
> 
> https://lore.kernel.org/linux-trace-kernel/20230807183308.9015-1-me@manjusaka.me/
> 
> In this version of the code, here's some different:
> 
> 1. The event name has been changed from `tcp_ca_event_set` to
> `tcp_ca_event`
> 
> 2. Output the current protocol family in TP_printk
> 
> 3. Show the ca_event symbol instead of the original number
> 
> But the `./scripts/checkpatch.pl` has been failed to check this patch,
> because we sill have some code error in ./scripts/checkpatch.pl(in
> another world, the test would be failed when we use the 
> scripts/checkpatch.pl to check the events/tcp.h
> 
> Feel free to ask me if you have have any issues and ideas.

## Form letter - net-next-closed

The merge window for v6.6 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after Sept 11th.

RFC patches sent for review only are obviously welcome at any time.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
-- 
pw-bot: defer

Re: [PATCH v4] tracepoint: add new `tcp:tcp_ca_event` trace event

[Manjusaka]

On 2023/8/25 21:32, Zheao Li wrote:
> Hello 
> 
> This the 4th version of the patch, the previous discusstion is here
> 
> https://lore.kernel.org/linux-trace-kernel/20230807183308.9015-1-me@manjusaka.me/
> 
> In this version of the code, here's some different:
> 
> 1. The event name has been changed from `tcp_ca_event_set` to
> `tcp_ca_event`
> 
> 2. Output the current protocol family in TP_printk
> 
> 3. Show the ca_event symbol instead of the original number
> 
> But the `./scripts/checkpatch.pl` has been failed to check this patch,
> because we sill have some code error in ./scripts/checkpatch.pl(in
> another world, the test would be failed when we use the 
> scripts/checkpatch.pl to check the events/tcp.h
> 
> Feel free to ask me if you have have any issues and ideas.
> 
> Thanks
> 
> ---
> 
> In normal use case, the tcp_ca_event would be changed in high frequency.
> 
> The developer can monitor the network quality more easier by tracing
> TCP stack with this TP event.
> 
> 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: Zheao Li <me@manjusaka.me>
> ---
>  include/net/tcp.h          |  9 ++----
>  include/trace/events/tcp.h | 60 ++++++++++++++++++++++++++++++++++++++
>  net/ipv4/tcp_cong.c        | 10 +++++++
>  3 files changed, 72 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 7b1ddffa3dfc..993eb00403ea 100644
> --- a/include/trace/events/tcp.h
> +++ b/include/trace/events/tcp.h
> @@ -41,6 +41,18 @@
>  	TP_STORE_V4MAPPED(__entry, saddr, daddr)
>  #endif
>  
> +/* The TCP CA event traced by tcp_ca_event*/
> +#define tcp_ca_event_names    \
> +		EM(CA_EVENT_TX_START)     \
> +		EM(CA_EVENT_CWND_RESTART) \
> +		EM(CA_EVENT_COMPLETE_CWR) \
> +		EM(CA_EVENT_LOSS)         \
> +		EM(CA_EVENT_ECN_NO_CE)    \
> +		EMe(CA_EVENT_ECN_IS_CE)
> +
> +#define show_tcp_ca_event_names(val) \
> +	__print_symbolic(val, tcp_ca_event_names)
> +
>  /*
>   * tcp event with arguments sk and skb
>   *
> @@ -419,6 +431,54 @@ 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)
> +		__field(__u16, family)
> +		__array(__u8, saddr, 4)
> +		__array(__u8, daddr, 4)
> +		__array(__u8, saddr_v6, 16)
> +		__array(__u8, daddr_v6, 16)
> +		__field(__u8, ca_event)
> +	),
> +
> +	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);
> +		__entry->family = sk->sk_family;
> +
> +		p32 = (__be32 *) __entry->saddr;
> +		*p32 = inet->inet_saddr;
> +
> +		p32 = (__be32 *) __entry->daddr;
> +		*p32 =  inet->inet_daddr;
> +
> +		TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr,
> +			   sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);
> +
> +		__entry->ca_event = ca_event;
> +	),
> +
> +	TP_printk("family=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%s",
> +		  show_family_name(__entry->family),
> +		  __entry->sport, __entry->dport,
> +		  __entry->saddr, __entry->daddr,
> +		  __entry->saddr_v6, __entry->daddr_v6,
> +		  show_tcp_ca_event_names(__entry->ca_event))
> +);
> +
>  #endif /* _TRACE_TCP_H */
>  
>  /* This part must be outside protection */
> diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> index 1b34050a7538..fb7ec6ebbbd0 100644
> --- a/net/ipv4/tcp_cong.c
> +++ b/net/ipv4/tcp_cong.c
> @@ -34,6 +34,16 @@ struct tcp_congestion_ops *tcp_ca_find(const char *name)
>  	return NULL;
>  }
>  
> +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event)
> +{
> +	const struct inet_connection_sock *icsk = inet_csk(sk);
> +
> +	trace_tcp_ca_event(sk, (u8)event);
> +
> +	if (icsk->icsk_ca_ops->cwnd_event)
> +		icsk->icsk_ca_ops->cwnd_event(sk, event);
> +}
> +
>  void tcp_set_ca_state(struct sock *sk, const u8 ca_state)
>  {
>  	struct inet_connection_sock *icsk = inet_csk(sk);


Ping to review(

Re: [PATCH v4] tracepoint: add new `tcp:tcp_ca_event` trace event

[Steven Rostedt]

On Sun, 17 Sep 2023 22:06:45 +0800
Manjusaka <me@manjusaka.me> wrote:

> Ping to review(

The ask was to resend the patches after the merge window.

-- Steve