On Tue, Aug 8, 2023 at 10:46 AM Manjusaka <[email protected]> wrote:
>
>
>
> On 2023/8/8 16:26, Eric Dumazet wrote:
> > On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.
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 <[email protected]>
---
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:
> 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 <[email protected]>
> ---
> 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.
On Sun, 13 Aug 2023 04:17:24 +0800
Manjusaka <[email protected]> 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 <[email protected]>
> > ---
> > 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, 2023-08-12 at 21:04 -0400, Steven Rostedt wrote:
> On Sat, 12 Aug 2023 21:01:40 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Sat, 12 Aug 2023 20:59:05 -0400
> > Steven Rostedt <[email protected]> wrote:
> >
> > > On Sat, 12 Aug 2023 20:12:50 +0000
> > > Zheao Li <[email protected]> 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.
On Sat, 12 Aug 2023 21:01:40 -0400
Steven Rostedt <[email protected]> wrote:
> On Sat, 12 Aug 2023 20:59:05 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Sat, 12 Aug 2023 20:12:50 +0000
> > Zheao Li <[email protected]> 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.
> >
>
On Sat, 12 Aug 2023 20:12:50 +0000
Zheao Li <[email protected]> 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 18:17:17 -0700
Joe Perches <[email protected]> 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
On Sat, 2023-08-12 at 21:53 -0400, Steven Rostedt wrote:
> On Sat, 12 Aug 2023 18:17:17 -0700
> Joe Perches <[email protected]> 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*$/) {
On Sat, 12 Aug 2023 20:59:05 -0400
Steven Rostedt <[email protected]> wrote:
> On Sat, 12 Aug 2023 20:12:50 +0000
> Zheao Li <[email protected]> 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);
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 <[email protected]> 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.
On Wed, 16 Aug 2023 14:09:06 +0800
Manjusaka <[email protected]> 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
On 2023/8/16 23:02, Steven Rostedt wrote:
> On Wed, 16 Aug 2023 14:09:06 +0800
> Manjusaka <[email protected]> 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