| Message ID | 20230808055817.3979-1-me@manjusaka.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] tracepoint: add new `tcp:tcp_ca_event` trace event | expand |
On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <me@manjusaka.me> wrote: > > In normal use case, the tcp_ca_event would be changed in high frequency. > > It's a good indicator to represent the network quanlity. quality ? Honestly, it is more about TCP stack tracing than 'network quality' > > So I propose to add a `tcp:tcp_ca_event` trace event > like `tcp:tcp_cong_state_set` to help the people to > trace the TCP connection status > > Signed-off-by: Manjusaka <me@manjusaka.me> > --- > include/net/tcp.h | 9 ++------ > include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ > net/ipv4/tcp_cong.c | 10 +++++++++ > 3 files changed, 57 insertions(+), 7 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 0ca972ebd3dd..a68c5b61889c 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk) > return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN; > } > > -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event) > -{ > - const struct inet_connection_sock *icsk = inet_csk(sk); > - > - if (icsk->icsk_ca_ops->cwnd_event) > - icsk->icsk_ca_ops->cwnd_event(sk, event); > -} > +/* from tcp_cong.c */ > +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event); > > /* From tcp_cong.c */ > void tcp_set_ca_state(struct sock *sk, const u8 ca_state); > diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h > index bf06db8d2046..b374eb636af9 100644 > --- a/include/trace/events/tcp.h > +++ b/include/trace/events/tcp.h > @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set, > __entry->cong_state) > ); > > +TRACE_EVENT(tcp_ca_event, > + > + TP_PROTO(struct sock *sk, const u8 ca_event), > + > + TP_ARGS(sk, ca_event), > + > + TP_STRUCT__entry( > + __field(const void *, skaddr) > + __field(__u16, sport) > + __field(__u16, dport) > + __array(__u8, saddr, 4) > + __array(__u8, daddr, 4) > + __array(__u8, saddr_v6, 16) > + __array(__u8, daddr_v6, 16) > + __field(__u8, ca_event) > + ), > + Please add the family (look at commit 3dd344ea84e1 ("net: tracepoint: exposing sk_family in all tcp:tracepoints")) > + TP_fast_assign( > + struct inet_sock *inet = inet_sk(sk); > + __be32 *p32; > + > + __entry->skaddr = sk; > + > + __entry->sport = ntohs(inet->inet_sport); > + __entry->dport = ntohs(inet->inet_dport); > + > + p32 = (__be32 *) __entry->saddr; > + *p32 = inet->inet_saddr; > + > + p32 = (__be32 *) __entry->daddr; > + *p32 = inet->inet_daddr; We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/ > + > + TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr, > + sk->sk_v6_rcv_saddr, sk->sk_v6_daddr); I will send a cleanup, because IP_STORE_ADDRS() should really take care of all details. > + > + __entry->ca_event = ca_event; > + ), > + > + TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%u", > + __entry->sport, __entry->dport, > + __entry->saddr, __entry->daddr, > + __entry->saddr_v6, __entry->daddr_v6, > + __entry->ca_event) Please print the symbol instead of numeric ca_event. Look at show_tcp_state_name() for instance.
On 2023/8/8 16:26, Eric Dumazet wrote: > On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <me@manjusaka.me> wrote: >> >> In normal use case, the tcp_ca_event would be changed in high frequency. >> >> It's a good indicator to represent the network quanlity. > > quality ? > > Honestly, it is more about TCP stack tracing than 'network quality' > >> >> So I propose to add a `tcp:tcp_ca_event` trace event >> like `tcp:tcp_cong_state_set` to help the people to >> trace the TCP connection status >> >> Signed-off-by: Manjusaka <me@manjusaka.me> >> --- >> include/net/tcp.h | 9 ++------ >> include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ >> net/ipv4/tcp_cong.c | 10 +++++++++ >> 3 files changed, 57 insertions(+), 7 deletions(-) >> >> diff --git a/include/net/tcp.h b/include/net/tcp.h >> index 0ca972ebd3dd..a68c5b61889c 100644 >> --- a/include/net/tcp.h >> +++ b/include/net/tcp.h >> @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk) >> return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN; >> } >> >> -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event) >> -{ >> - const struct inet_connection_sock *icsk = inet_csk(sk); >> - >> - if (icsk->icsk_ca_ops->cwnd_event) >> - icsk->icsk_ca_ops->cwnd_event(sk, event); >> -} >> +/* from tcp_cong.c */ >> +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event); >> >> /* From tcp_cong.c */ >> void tcp_set_ca_state(struct sock *sk, const u8 ca_state); >> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h >> index bf06db8d2046..b374eb636af9 100644 >> --- a/include/trace/events/tcp.h >> +++ b/include/trace/events/tcp.h >> @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set, >> __entry->cong_state) >> ); >> >> +TRACE_EVENT(tcp_ca_event, >> + >> + TP_PROTO(struct sock *sk, const u8 ca_event), >> + >> + TP_ARGS(sk, ca_event), >> + >> + TP_STRUCT__entry( >> + __field(const void *, skaddr) >> + __field(__u16, sport) >> + __field(__u16, dport) >> + __array(__u8, saddr, 4) >> + __array(__u8, daddr, 4) >> + __array(__u8, saddr_v6, 16) >> + __array(__u8, daddr_v6, 16) >> + __field(__u8, ca_event) >> + ), >> + > > Please add the family (look at commit 3dd344ea84e1 ("net: tracepoint: > exposing sk_family in all tcp:tracepoints")) > > > >> + TP_fast_assign( >> + struct inet_sock *inet = inet_sk(sk); >> + __be32 *p32; >> + >> + __entry->skaddr = sk; >> + >> + __entry->sport = ntohs(inet->inet_sport); >> + __entry->dport = ntohs(inet->inet_dport); >> + >> + p32 = (__be32 *) __entry->saddr; >> + *p32 = inet->inet_saddr; >> + >> + p32 = (__be32 *) __entry->daddr; >> + *p32 = inet->inet_daddr; > > We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/ > >> + >> + TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr, >> + sk->sk_v6_rcv_saddr, sk->sk_v6_daddr); > > I will send a cleanup, because IP_STORE_ADDRS() should really take > care of all details. > > >> + >> + __entry->ca_event = ca_event; >> + ), >> + >> + TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%u", >> + __entry->sport, __entry->dport, >> + __entry->saddr, __entry->daddr, >> + __entry->saddr_v6, __entry->daddr_v6, >> + __entry->ca_event) > > Please print the symbol instead of numeric ca_event. > > Look at show_tcp_state_name() for instance. Thanks for the kindness code review, I still get some issue here(Sorry for the first time to contribute): 1. > We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/ I'm not getting your means, would you mean that we should only save the IPv4 Address here? 2. > I will send a cleanup, because IP_STORE_ADDRS() should really take care of all details. I think you will make the address assignment code in TP_fast_assign as a new function. Should I submit the new change until you send the cleanup patch or I can make this in my patch(cleanup the address assignment)
On Tue, Aug 8, 2023 at 10:46 AM Manjusaka <me@manjusaka.me> wrote: > > > > On 2023/8/8 16:26, Eric Dumazet wrote: > > On Tue, Aug 8, 2023 at 7:59 AM Manjusaka <me@manjusaka.me> wrote: > >> > >> In normal use case, the tcp_ca_event would be changed in high frequency. > >> > >> It's a good indicator to represent the network quanlity. > > > > quality ? > > > > Honestly, it is more about TCP stack tracing than 'network quality' > > > >> > >> So I propose to add a `tcp:tcp_ca_event` trace event > >> like `tcp:tcp_cong_state_set` to help the people to > >> trace the TCP connection status > >> > >> Signed-off-by: Manjusaka <me@manjusaka.me> > >> --- > >> include/net/tcp.h | 9 ++------ > >> include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ > >> net/ipv4/tcp_cong.c | 10 +++++++++ > >> 3 files changed, 57 insertions(+), 7 deletions(-) > >> > >> diff --git a/include/net/tcp.h b/include/net/tcp.h > >> index 0ca972ebd3dd..a68c5b61889c 100644 > >> --- a/include/net/tcp.h > >> +++ b/include/net/tcp.h > >> @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk) > >> return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN; > >> } > >> > >> -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event) > >> -{ > >> - const struct inet_connection_sock *icsk = inet_csk(sk); > >> - > >> - if (icsk->icsk_ca_ops->cwnd_event) > >> - icsk->icsk_ca_ops->cwnd_event(sk, event); > >> -} > >> +/* from tcp_cong.c */ > >> +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event); > >> > >> /* From tcp_cong.c */ > >> void tcp_set_ca_state(struct sock *sk, const u8 ca_state); > >> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h > >> index bf06db8d2046..b374eb636af9 100644 > >> --- a/include/trace/events/tcp.h > >> +++ b/include/trace/events/tcp.h > >> @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set, > >> __entry->cong_state) > >> ); > >> > >> +TRACE_EVENT(tcp_ca_event, > >> + > >> + TP_PROTO(struct sock *sk, const u8 ca_event), > >> + > >> + TP_ARGS(sk, ca_event), > >> + > >> + TP_STRUCT__entry( > >> + __field(const void *, skaddr) > >> + __field(__u16, sport) > >> + __field(__u16, dport) > >> + __array(__u8, saddr, 4) > >> + __array(__u8, daddr, 4) > >> + __array(__u8, saddr_v6, 16) > >> + __array(__u8, daddr_v6, 16) > >> + __field(__u8, ca_event) > >> + ), > >> + > > > > Please add the family (look at commit 3dd344ea84e1 ("net: tracepoint: > > exposing sk_family in all tcp:tracepoints")) > > > > > > > >> + TP_fast_assign( > >> + struct inet_sock *inet = inet_sk(sk); > >> + __be32 *p32; > >> + > >> + __entry->skaddr = sk; > >> + > >> + __entry->sport = ntohs(inet->inet_sport); > >> + __entry->dport = ntohs(inet->inet_dport); > >> + > >> + p32 = (__be32 *) __entry->saddr; > >> + *p32 = inet->inet_saddr; > >> + > >> + p32 = (__be32 *) __entry->daddr; > >> + *p32 = inet->inet_daddr; > > > > We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/ > > > >> + > >> + TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr, > >> + sk->sk_v6_rcv_saddr, sk->sk_v6_daddr); > > > > I will send a cleanup, because IP_STORE_ADDRS() should really take > > care of all details. > > > > > >> + > >> + __entry->ca_event = ca_event; > >> + ), > >> + > >> + TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c ca_event=%u", > >> + __entry->sport, __entry->dport, > >> + __entry->saddr, __entry->daddr, > >> + __entry->saddr_v6, __entry->daddr_v6, > >> + __entry->ca_event) > > > > Please print the symbol instead of numeric ca_event. > > > > Look at show_tcp_state_name() for instance. > > Thanks for the kindness code review, I still get some issue here(Sorry for the first time to contribute): > > 1. > We keep copying IPv4 addresses that might contain garbage for IPv6 sockets :/ > > I'm not getting your means, would you mean that we should only save the IPv4 Address here? > > 2. > I will send a cleanup, because IP_STORE_ADDRS() should really take care of all details. > > I think you will make the address assignment code in TP_fast_assign as a new function. > > Should I submit the new change until you send the cleanup patch or I can make this in my patch(cleanup the address assignment) > Wait a bit, I am sending fixes today, so that no more copy/paste duplicates the issues.
On Tue, 8 Aug 2023 05:58:18 +0000 Manjusaka wrote:
> Signed-off-by: Manjusaka <me@manjusaka.me>
Is that your name? For Developer's Certificate of Origin
https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin
we need something that resembles a real name that'd stand up in court.
On 2023/8/9 04:21, Jakub Kicinski wrote: > On Tue, 8 Aug 2023 05:58:18 +0000 Manjusaka wrote: >> Signed-off-by: Manjusaka <me@manjusaka.me> > > Is that your name? For Developer's Certificate of Origin > https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin > we need something that resembles a real name that'd stand up in court. Sorry about this, I will update my real name in next patch.
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);
In normal use case, the tcp_ca_event would be changed in high frequency. It's a good indicator to represent the network quanlity. So I propose to add a `tcp:tcp_ca_event` trace event like `tcp:tcp_cong_state_set` to help the people to trace the TCP connection status Signed-off-by: Manjusaka <me@manjusaka.me> --- include/net/tcp.h | 9 ++------ include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ net/ipv4/tcp_cong.c | 10 +++++++++ 3 files changed, 57 insertions(+), 7 deletions(-)