| Message ID | 20230812201249.62237-1-me@manjusaka.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] tracepoint: add new `tcp:tcp_ca_event` trace event | expand |
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.
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
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 */ >
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);
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. > > >
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.
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
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*$/) {
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.
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
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
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 <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 ?
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
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);
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(-)