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

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

On Mon, Aug 7, 2023 at 2:33 PM Manjusaka [off-list ref] wrote:

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

On 2023/8/8 04:00, Neal Cardwell wrote:

On Mon, Aug 7, 2023 at 2:33 PM 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.

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

On 2023/8/8 04:00, Neal Cardwell wrote:

On Mon, Aug 7, 2023 at 2:33 PM 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.

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

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.

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

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

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)

On Tue, Aug 8, 2023 at 7:59 AM Manjusaka [off-list ref] 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'

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.

On Tue,  8 Aug 2023 05:58:18 +0000 Manjusaka wrote:

Signed-off-by: Manjusaka <redacted>

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.

On 2023/8/9 04:21, Jakub Kicinski wrote:

On Tue,  8 Aug 2023 05:58:18 +0000 Manjusaka wrote:

quoted

Signed-off-by: Manjusaka <redacted>

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.

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 <redacted>
---
 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

On 2023/8/13 04:12, Zheao Li wrote:

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.

On Sun, 13 Aug 2023 04:17:24 +0800
Manjusaka [off-list ref] wrote:

On 2023/8/13 04:12, Zheao Li wrote:

quoted

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 <redacted>
---
 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

On Sat, 12 Aug 2023 20:12:50 +0000
Zheao Li [off-list ref] 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 */
 

On Sat, 12 Aug 2023 20:59:05 -0400
Steven Rostedt [off-list ref] wrote:

On Sat, 12 Aug 2023 20:12:50 +0000
Zheao Li [off-list ref] wrote:

quoted

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

quoted

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

On Sat, 12 Aug 2023 21:01:40 -0400
Steven Rostedt [off-list ref] wrote:

On Sat, 12 Aug 2023 20:59:05 -0400
Steven Rostedt [off-list ref] wrote:

quoted

On Sat, 12 Aug 2023 20:12:50 +0000
Zheao Li [off-list ref] wrote:
  

quoted

+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

quoted

The above looks horrendous! Put it back to:
  

quoted

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

On Sat, 2023-08-12 at 21:04 -0400, Steven Rostedt wrote:

On Sat, 12 Aug 2023 21:01:40 -0400
Steven Rostedt [off-list ref] wrote:

quoted

On Sat, 12 Aug 2023 20:59:05 -0400
Steven Rostedt [off-list ref] wrote:

quoted

On Sat, 12 Aug 2023 20:12:50 +0000
Zheao Li [off-list ref] wrote:
  

quoted

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

On Sat, 12 Aug 2023 18:17:17 -0700
Joe Perches [off-list ref] wrote:

quoted

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

On Sat, 2023-08-12 at 21:53 -0400, Steven Rostedt wrote:

On Sat, 12 Aug 2023 18:17:17 -0700
Joe Perches [off-list ref] wrote:

quoted

quoted

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*$/) {

On 2023/8/13 10:08, Joe Perches wrote:

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

Seems wired.

On Wed, 16 Aug 2023 14:09:06 +0800
Manjusaka [off-list ref] wrote:

quoted

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

On 2023/8/16 23:02, Steven Rostedt wrote:

On Wed, 16 Aug 2023 14:09:06 +0800
Manjusaka [off-list ref] wrote:

quoted

quoted

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

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?

On Sat, Aug 19, 2023 at 3:52 AM Jakub Kicinski [off-list ref] wrote:

On Sat, 12 Aug 2023 20:12:50 +0000 Zheao Li wrote:

quoted

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 ?

On 2023/8/19 11:10, Eric Dumazet wrote:

On Sat, Aug 19, 2023 at 3:52 AM Jakub Kicinski [off-list ref] wrote:

quoted

On Sat, 12 Aug 2023 20:12:50 +0000 Zheao Li wrote:

quoted

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