[PATCH] tracepoint: add new `tcp:tcp_ca_event_set` trace event

[PATCH] tracepoint: add new `tcp:tcp_ca_event_set` trace event

[Manjusaka]

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

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

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

Signed-off-by: Manjusaka <me@manjusaka.me>
---
 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..38415c5f1d52 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_set,
+
+	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)
+	),
+
+	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;
+
+		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("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)
+);
+
 #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..08e02850d3de 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_set(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] tracepoint: add new `tcp:tcp_ca_event_set` trace event

[Neal Cardwell]

On Mon, Aug 7, 2023 at 2:33 PM Manjusaka <me@manjusaka.me> wrote:
>
> In normal use case, the tcp_ca_event would be changed in high frequency.
>
> It's a good indicator to represent the network quanlity.
>
> So I propose to add a `tcp:tcp_ca_event_set` trace event
> like `tcp:tcp_cong_state_set` to help the people to
> trace the TCP connection status
>
> Signed-off-by: Manjusaka <me@manjusaka.me>
> ---
>  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..38415c5f1d52 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_set,

Regarding the proposed name, "tcp_ca_event_set"... including "set" in
the name is confusing, since the tcp_ca_event() function is not really
setting anything. :-)

The trace_tcp_cong_state_set() call you are using as a model has "set"
in the name because the function it is tracing, tcp_set_ca_state(),
has "set" in the name. :-)

Would it work to use something like:
  TRACE_EVENT(tcp_ca_event,

thanks,
neal

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

[Manjusaka]

On 2023/8/8 04:00, Neal Cardwell wrote:
> On Mon, Aug 7, 2023 at 2:33 PM Manjusaka <me@manjusaka.me> wrote:
>> In normal use case, the tcp_ca_event would be changed in high frequency.
>>
>> It's a good indicator to represent the network quanlity.
>>
>> So I propose to add a `tcp:tcp_ca_event_set` trace event
>> like `tcp:tcp_cong_state_set` to help the people to
>> trace the TCP connection status
>>
>> Signed-off-by: Manjusaka <me@manjusaka.me>
>> ---
>>  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..38415c5f1d52 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_set,
> Regarding the proposed name, "tcp_ca_event_set"... including "set" in
> the name is confusing, since the tcp_ca_event() function is not really
> setting anything. :-)
>
> The trace_tcp_cong_state_set() call you are using as a model has "set"
> in the name because the function it is tracing, tcp_set_ca_state(),
> has "set" in the name. :-)
>
> Would it work to use something like:
>   TRACE_EVENT(tcp_ca_event,
>
> thanks,
> neal
Thanks for the review! LGTM, I will send a new patch ASAP

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

[Manjusaka]

On 2023/8/8 04:00, Neal Cardwell wrote:
> On Mon, Aug 7, 2023 at 2:33 PM Manjusaka <me@manjusaka.me> wrote:
>>
>> In normal use case, the tcp_ca_event would be changed in high frequency.
>>
>> It's a good indicator to represent the network quanlity.
>>
>> So I propose to add a `tcp:tcp_ca_event_set` trace event
>> like `tcp:tcp_cong_state_set` to help the people to
>> trace the TCP connection status
>>
>> Signed-off-by: Manjusaka <me@manjusaka.me>
>> ---
>>  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..38415c5f1d52 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_set,
> 
> Regarding the proposed name, "tcp_ca_event_set"... including "set" in
> the name is confusing, since the tcp_ca_event() function is not really
> setting anything. :-)
> 
> The trace_tcp_cong_state_set() call you are using as a model has "set"
> in the name because the function it is tracing, tcp_set_ca_state(),
> has "set" in the name. :-)
> 
> Would it work to use something like:
>   TRACE_EVENT(tcp_ca_event,
> 
> thanks,
> neal

Emmmm, sorry about the confusing var name and thanks for the review. I will update my patch ASAP.

best regards
Manjusaka

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

[Manjusaka]

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

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

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 <me@manjusaka.me>
---
 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)
+	),
+
+	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;
+
+		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("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)
+);
+
 #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 v2] tracepoint: add new `tcp:tcp_ca_event` trace event

[Eric Dumazet]

On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <me@manjusaka.me> wrote:
>
> 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'

>
> 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 <me@manjusaka.me>
> ---
>  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"))



> +       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 :/

> +
> +               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.


> +
> +               __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.

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

[Manjusaka]

On 2023/8/8 16:26, Eric Dumazet wrote:
> On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <me@manjusaka.me> wrote:
>>
>> 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'
> 
>>
>> 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 <me@manjusaka.me>
>> ---
>>  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"))
> 
> 
> 
>> +       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 :/
> 
>> +
>> +               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.
> 
> 
>> +
>> +               __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)

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

[Eric Dumazet]

On Tue, Aug 8, 2023 at 10:46 AM Manjusaka <me@manjusaka.me> wrote:
>
>
>
> On 2023/8/8 16:26, Eric Dumazet wrote:
> > On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <me@manjusaka.me> wrote:
> >>
> >> 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'
> >
> >>
> >> 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 <me@manjusaka.me>
> >> ---
> >>  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"))
> >
> >
> >
> >> +       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 :/
> >
> >> +
> >> +               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.
> >
> >
> >> +
> >> +               __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.

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

[Jakub Kicinski]

On Tue,  8 Aug 2023 05:58:18 +0000 Manjusaka wrote:
> Signed-off-by: Manjusaka <me@manjusaka.me>

Is that your name? For Developer's Certificate of Origin
https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin
we need something that resembles a real name that'd stand up in court.

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

[Manjusaka]

On 2023/8/9 04:21, Jakub Kicinski wrote:
> On Tue,  8 Aug 2023 05:58:18 +0000 Manjusaka wrote:
>> Signed-off-by: Manjusaka <me@manjusaka.me>
> 
> Is that your name? For Developer's Certificate of Origin
> https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin
> we need something that resembles a real name that'd stand up in court.
Sorry about this, I will update my real name in next patch.

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

[Zheao Li]

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 v3] tracepoint: add new `tcp:tcp_ca_event` trace event

[Manjusaka]

On 2023/8/13 04:12, Zheao Li wrote:
> 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);

For more information, this patch is not passthrough the `./scripts/checkpatch.pl` check 
with the following error message `Macros with complex values should be enclosed in parentheses`.

I have no idea because there is no complex expression and the `include/trace/events/sock.h` files 
also failed in the style check.

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

[Steven Rostedt]

On Sun, 13 Aug 2023 04:17:24 +0800
Manjusaka <me@manjusaka.me> wrote:

> On 2023/8/13 04:12, Zheao Li wrote:
> > 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);  
> 
> For more information, this patch is not passthrough the `./scripts/checkpatch.pl` check 
> with the following error message `Macros with complex values should be enclosed in parentheses`.
> 
> I have no idea because there is no complex expression and the `include/trace/events/sock.h` files 
> also failed in the style check.

Please ignore all checkpatch.pl messages when it comes to the
TRACE_EVENT() macro and pretty much anything it recommends to do with
TRACE_EVENTS() in general.

checkpatch.pl's recommendations on the include/trace code is just
wrong, and makes it worse.

One day I need to add a patch to fix checkpatch.

-- Steve

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

[Steven Rostedt]

On Sat, 12 Aug 2023 20:12:50 +0000
Zheao Li <me@manjusaka.me> wrote:

> +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)

Please DO NOT LISTEN TO CHECKPATCH!

The above looks horrendous! Put it back to:

> +		__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		)

See how much better it looks I can see fields this way.

The "checkpatch" way is a condensed mess.

-- Steve

> +	),
> +
> +	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 */
>  

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

[Steven Rostedt]

On Sat, 12 Aug 2023 20:59:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 12 Aug 2023 20:12:50 +0000
> Zheao Li <me@manjusaka.me> wrote:
> 
> > +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)  
> 
> Please DO NOT LISTEN TO CHECKPATCH!
> 
> The above looks horrendous! Put it back to:
> 
> > +		__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		)  
> 
> See how much better it looks I can see fields this way.
> 
> The "checkpatch" way is a condensed mess.
> 

The below patch makes checkpatch not complain about some of this. But
there's still more to do.

-- Steve

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1e5e66ae5a52..24df11e8c861 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -73,6 +73,7 @@ my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANC
 my $git_command ='export LANGUAGE=en_US.UTF-8; git';
 my $tabsize = 8;
 my ${CONFIG_} = "CONFIG_";
+my $trace_macros = "__array|__dynamic_array|__field|__string|EMe?";
 
 sub help {
 	my ($exitcode) = @_;
@@ -5387,7 +5388,8 @@ sub process {
 
 # check spacing on parentheses
 		if ($line =~ /\(\s/ && $line !~ /\(\s*(?:\\)?$/ &&
-		    $line !~ /for\s*\(\s+;/) {
+		    $line !~ /for\s*\(\s+;/ &&
+		    $line !~ m/$trace_macros/) {
 			if (ERROR("SPACING",
 				  "space prohibited after that open parenthesis '('\n" . $herecurr) &&
 			    $fix) {
@@ -5397,7 +5399,8 @@ sub process {
 		}
 		if ($line =~ /(\s+)\)/ && $line !~ /^.\s*\)/ &&
 		    $line !~ /for\s*\(.*;\s+\)/ &&
-		    $line !~ /:\s+\)/) {
+		    $line !~ /:\s+\)/ &&
+		    $line !~ m/$trace_macros/) {
 			if (ERROR("SPACING",
 				  "space prohibited before that close parenthesis ')'\n" . $herecurr) &&
 			    $fix) {
@@ -5906,6 +5909,7 @@ sub process {
 			    $dstat !~ /^for\s*$Constant$/ &&				# for (...)
 			    $dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ &&	# for (...) bar()
 			    $dstat !~ /^do\s*{/ &&					# do {...
+			    $dstat !~ /^EMe?\s*1u/ &&					# EM( and EMe( are commonly used with TRACE_DEFINE_ENUM
 			    $dstat !~ /^\(\{/ &&						# ({...
 			    $ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
 			{
@@ -6017,7 +6021,8 @@ sub process {
 					WARN("DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON",
 					     "do {} while (0) macros should not be semicolon terminated\n" . "$herectx");
 				}
-			} elsif ($dstat =~ /^\+\s*#\s*define\s+$Ident.*;\s*$/) {
+			} elsif ($dstat =~ /^\+\s*#\s*define\s+$Ident.*;\s*$/ &&
+				 $dstat !~ /TRACE_DEFINE_ENUM\(/) {
 				$ctx =~ s/\n*$//;
 				my $cnt = statement_rawlines($ctx);
 				my $herectx = get_stat_here($linenr, $cnt, $here);

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

[Steven Rostedt]

On Sat, 12 Aug 2023 21:01:40 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 12 Aug 2023 20:59:05 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Sat, 12 Aug 2023 20:12:50 +0000
> > Zheao Li <me@manjusaka.me> wrote:
> >   
> > > +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)    
> > 
> > Please DO NOT LISTEN TO CHECKPATCH!

I forgot to say "for TRACE_EVENT() macros". This is not about what
checkpatch says about other code.

-- Steve


> > 
> > The above looks horrendous! Put it back to:
> >   
> > > +		__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		)    
> > 
> > See how much better it looks I can see fields this way.
> > 
> > The "checkpatch" way is a condensed mess.
> >   
> 

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

[Joe Perches]

On Sat, 2023-08-12 at 21:04 -0400, Steven Rostedt wrote:
> On Sat, 12 Aug 2023 21:01:40 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Sat, 12 Aug 2023 20:59:05 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Sat, 12 Aug 2023 20:12:50 +0000
> > > Zheao Li <me@manjusaka.me> wrote:
> > >   
> > > > +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)    
> > > 
> > > Please DO NOT LISTEN TO CHECKPATCH!
> 
> I forgot to say "for TRACE_EVENT() macros". This is not about what
> checkpatch says about other code.

trace has its own code style and checkpatch needs another
parsing mechanism just for it, including the alignment to
open parenthesis test.

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

[Steven Rostedt]

On Sat, 12 Aug 2023 18:17:17 -0700
Joe Perches <joe@perches.com> wrote:

> > I forgot to say "for TRACE_EVENT() macros". This is not about what
> > checkpatch says about other code.  
> 
> trace has its own code style and checkpatch needs another
> parsing mechanism just for it, including the alignment to
> open parenthesis test.

If you have a template patch to add the parsing mechanism, I'd be happy
to try to fill in the style.

-- Steve

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

[Joe Perches]

On Sat, 2023-08-12 at 21:53 -0400, Steven Rostedt wrote:
> On Sat, 12 Aug 2023 18:17:17 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > I forgot to say "for TRACE_EVENT() macros". This is not about what
> > > checkpatch says about other code.  
> > 
> > trace has its own code style and checkpatch needs another
> > parsing mechanism just for it, including the alignment to
> > open parenthesis test.
> 
> If you have a template patch to add the parsing mechanism, I'd be happy
> to try to fill in the style.

There is no checkpatch mechanism per se.  It's all ad-hoc.

Perhaps something like this though would work well enough
as it just avoids all the other spacing checks and such.
---
 scripts/checkpatch.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 528f619520eb9..3017f4dd09fd2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3947,6 +3947,9 @@ sub process {
 			}
 		}
 
+# trace include files use a completely different grammar
+		next if ($realfile =~ m{(?:include/trace/events/|/trace\.h$/)});
+
 # check multi-line statement indentation matches previous line
 		if ($perl_version_ok &&
 		    $prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:\*\s*)*$Lval\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) {

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

[Manjusaka]

On 2023/8/13 10:08, Joe Perches wrote:
> On Sat, 2023-08-12 at 21:53 -0400, Steven Rostedt wrote:
>> On Sat, 12 Aug 2023 18:17:17 -0700
>> Joe Perches <joe@perches.com> wrote:
>>
>>>> I forgot to say "for TRACE_EVENT() macros". This is not about what
>>>> checkpatch says about other code.  
>>>
>>> trace has its own code style and checkpatch needs another
>>> parsing mechanism just for it, including the alignment to
>>> open parenthesis test.
>>
>> If you have a template patch to add the parsing mechanism, I'd be happy
>> to try to fill in the style.
> 
> There is no checkpatch mechanism per se.  It's all ad-hoc.
> 
> Perhaps something like this though would work well enough
> as it just avoids all the other spacing checks and such.
> ---
>  scripts/checkpatch.pl | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 528f619520eb9..3017f4dd09fd2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3947,6 +3947,9 @@ sub process {
>  			}
>  		}
>  
> +# trace include files use a completely different grammar
> +		next if ($realfile =~ m{(?:include/trace/events/|/trace\.h$/)});
> +
>  # check multi-line statement indentation matches previous line
>  		if ($perl_version_ok &&
>  		    $prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:\*\s*)*$Lval\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) {
> 
> 
> 

Actually, I'm not sure this is the checkpatch style issue or my code style issue.

Seems wired.

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

[Steven Rostedt]

On Wed, 16 Aug 2023 14:09:06 +0800
Manjusaka <me@manjusaka.me> wrote:

> > +# trace include files use a completely different grammar
> > +		next if ($realfile =~ m{(?:include/trace/events/|/trace\.h$/)});
> > +
> >  # check multi-line statement indentation matches previous line
> >  		if ($perl_version_ok &&
> >  		    $prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:\*\s*)*$Lval\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) {
> > 
> > 
> >   
> 
> Actually, I'm not sure this is the checkpatch style issue or my code style issue.
> 
> Seems wired.

The TRACE_EVENT() macro has its own style. I need to document it, and
perhaps one day get checkpatch to understand it as well.

The TRACE_EVENT() typically looks like:


TRACE_EVENT(name,

	TP_PROTO(int arg1, struct foo *arg2, struct bar *arg3),

	TP_ARGS(arg1, arg2, arg3),

	TP_STRUCT__entry(
		__field(	int,		field1				)
		__array(	char,		mystring,	MYSTRLEN	)
		__string(	filename,	arg3->name			)
	),

	TP_fast_assign(
		__entry->field1 = arg1;
		memcpy(__entry->mystring, arg2->string);
		__assign_str(filename, arg3->name);
	),

	TP_printk("field1=%d mystring=%s filename=%s",
		__entry->field1, __entry->mystring, __get_str(filename))
);

The TP_STRUCT__entry() should be considered more of a "struct" layout than
a macro layout, and that's where checkpatch gets confused. The spacing
makes it much easier to see the fields and their types.

-- Steve

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

[Manjusaka]

On 2023/8/16 23:02, Steven Rostedt wrote:
> On Wed, 16 Aug 2023 14:09:06 +0800
> Manjusaka <me@manjusaka.me> wrote:
> 
>>> +# trace include files use a completely different grammar
>>> +		next if ($realfile =~ m{(?:include/trace/events/|/trace\.h$/)});
>>> +
>>>  # check multi-line statement indentation matches previous line
>>>  		if ($perl_version_ok &&
>>>  		    $prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:\*\s*)*$Lval\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) {
>>>
>>>
>>>   
>>
>> Actually, I'm not sure this is the checkpatch style issue or my code style issue.
>>
>> Seems wired.
> 
> The TRACE_EVENT() macro has its own style. I need to document it, and
> perhaps one day get checkpatch to understand it as well.
> 
> The TRACE_EVENT() typically looks like:
> 
> 
> TRACE_EVENT(name,
> 
> 	TP_PROTO(int arg1, struct foo *arg2, struct bar *arg3),
> 
> 	TP_ARGS(arg1, arg2, arg3),
> 
> 	TP_STRUCT__entry(
> 		__field(	int,		field1				)
> 		__array(	char,		mystring,	MYSTRLEN	)
> 		__string(	filename,	arg3->name			)
> 	),
> 
> 	TP_fast_assign(
> 		__entry->field1 = arg1;
> 		memcpy(__entry->mystring, arg2->string);
> 		__assign_str(filename, arg3->name);
> 	),
> 
> 	TP_printk("field1=%d mystring=%s filename=%s",
> 		__entry->field1, __entry->mystring, __get_str(filename))
> );
> 
> The TP_STRUCT__entry() should be considered more of a "struct" layout than
> a macro layout, and that's where checkpatch gets confused. The spacing
> makes it much easier to see the fields and their types.
> 
> -- Steve

Thanks for the explain!

So could I keep the current code without any code style change?

I think it would be a good idea to fix the checkpatch.pl script in another patch

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

[Jakub Kicinski]

On Sat, 12 Aug 2023 20:12:50 +0000 Zheao Li wrote:
> 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

Ah, I completely missed v3 somehow and we got no ack from Eric so maybe
he missed it, too. Could you please resend not as part of this thread
but as a new thread?

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

[Eric Dumazet]

On Sat, Aug 19, 2023 at 3:52 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 12 Aug 2023 20:12:50 +0000 Zheao Li wrote:
> > 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
>
> Ah, I completely missed v3 somehow and we got no ack from Eric so maybe
> he missed it, too. Could you please resend not as part of this thread
> but as a new thread?

I was waiting for a v4, because Steven asked for additional spaces in the macros
for readability ?

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

[Manjusaka]

On 2023/8/19 11:10, Eric Dumazet wrote:
> On Sat, Aug 19, 2023 at 3:52 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Sat, 12 Aug 2023 20:12:50 +0000 Zheao Li wrote:
>>> 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
>>
>> Ah, I completely missed v3 somehow and we got no ack from Eric so maybe
>> he missed it, too. Could you please resend not as part of this thread
>> but as a new thread?
> 
> I was waiting for a v4, because Steven asked for additional spaces in the macros
> for readability ?

I think the additional spaces should not be added in this patch. Because there will
be two code style in one file.

I think it would be a good idea for another patch to adjust the space in this file