| Message ID | 20230807183308.9015-1-me@manjusaka.me (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | tracepoint: add new `tcp:tcp_ca_event_set` trace event | expand |
On Mon, Aug 7, 2023 at 2:33 PM Manjusaka <me@manjusaka.me> wrote: > > In normal use case, the tcp_ca_event would be changed in high frequency. > > It's a good indicator to represent the network quanlity. > > So I propose to add a `tcp:tcp_ca_event_set` trace event > like `tcp:tcp_cong_state_set` to help the people to > trace the TCP connection status > > Signed-off-by: Manjusaka <me@manjusaka.me> > --- > include/net/tcp.h | 9 ++------ > include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ > net/ipv4/tcp_cong.c | 10 +++++++++ > 3 files changed, 57 insertions(+), 7 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 0ca972ebd3dd..a68c5b61889c 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk) > return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN; > } > > -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event) > -{ > - const struct inet_connection_sock *icsk = inet_csk(sk); > - > - if (icsk->icsk_ca_ops->cwnd_event) > - icsk->icsk_ca_ops->cwnd_event(sk, event); > -} > +/* from tcp_cong.c */ > +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event); > > /* From tcp_cong.c */ > void tcp_set_ca_state(struct sock *sk, const u8 ca_state); > diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h > index bf06db8d2046..38415c5f1d52 100644 > --- a/include/trace/events/tcp.h > +++ b/include/trace/events/tcp.h > @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set, > __entry->cong_state) > ); > > +TRACE_EVENT(tcp_ca_event_set, Regarding the proposed name, "tcp_ca_event_set"... including "set" in the name is confusing, since the tcp_ca_event() function is not really setting anything. :-) The trace_tcp_cong_state_set() call you are using as a model has "set" in the name because the function it is tracing, tcp_set_ca_state(), has "set" in the name. :-) Would it work to use something like: TRACE_EVENT(tcp_ca_event, thanks, neal
On 2023/8/8 04:00, Neal Cardwell wrote: > On Mon, Aug 7, 2023 at 2:33 PM Manjusaka <me@manjusaka.me> wrote: >> In normal use case, the tcp_ca_event would be changed in high frequency. >> >> It's a good indicator to represent the network quanlity. >> >> So I propose to add a `tcp:tcp_ca_event_set` trace event >> like `tcp:tcp_cong_state_set` to help the people to >> trace the TCP connection status >> >> Signed-off-by: Manjusaka <me@manjusaka.me> >> --- >> include/net/tcp.h | 9 ++------ >> include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ >> net/ipv4/tcp_cong.c | 10 +++++++++ >> 3 files changed, 57 insertions(+), 7 deletions(-) >> >> diff --git a/include/net/tcp.h b/include/net/tcp.h >> index 0ca972ebd3dd..a68c5b61889c 100644 >> --- a/include/net/tcp.h >> +++ b/include/net/tcp.h >> @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk) >> return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN; >> } >> >> -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event) >> -{ >> - const struct inet_connection_sock *icsk = inet_csk(sk); >> - >> - if (icsk->icsk_ca_ops->cwnd_event) >> - icsk->icsk_ca_ops->cwnd_event(sk, event); >> -} >> +/* from tcp_cong.c */ >> +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event); >> >> /* From tcp_cong.c */ >> void tcp_set_ca_state(struct sock *sk, const u8 ca_state); >> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h >> index bf06db8d2046..38415c5f1d52 100644 >> --- a/include/trace/events/tcp.h >> +++ b/include/trace/events/tcp.h >> @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set, >> __entry->cong_state) >> ); >> >> +TRACE_EVENT(tcp_ca_event_set, > Regarding the proposed name, "tcp_ca_event_set"... including "set" in > the name is confusing, since the tcp_ca_event() function is not really > setting anything. :-) > > The trace_tcp_cong_state_set() call you are using as a model has "set" > in the name because the function it is tracing, tcp_set_ca_state(), > has "set" in the name. :-) > > Would it work to use something like: > TRACE_EVENT(tcp_ca_event, > > thanks, > neal Thanks for the review! LGTM, I will send a new patch ASAP
On 2023/8/8 04:00, Neal Cardwell wrote: > On Mon, Aug 7, 2023 at 2:33 PM Manjusaka <me@manjusaka.me> wrote: >> >> In normal use case, the tcp_ca_event would be changed in high frequency. >> >> It's a good indicator to represent the network quanlity. >> >> So I propose to add a `tcp:tcp_ca_event_set` trace event >> like `tcp:tcp_cong_state_set` to help the people to >> trace the TCP connection status >> >> Signed-off-by: Manjusaka <me@manjusaka.me> >> --- >> include/net/tcp.h | 9 ++------ >> include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ >> net/ipv4/tcp_cong.c | 10 +++++++++ >> 3 files changed, 57 insertions(+), 7 deletions(-) >> >> diff --git a/include/net/tcp.h b/include/net/tcp.h >> index 0ca972ebd3dd..a68c5b61889c 100644 >> --- a/include/net/tcp.h >> +++ b/include/net/tcp.h >> @@ -1154,13 +1154,8 @@ static inline bool tcp_ca_needs_ecn(const struct sock *sk) >> return icsk->icsk_ca_ops->flags & TCP_CONG_NEEDS_ECN; >> } >> >> -static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event) >> -{ >> - const struct inet_connection_sock *icsk = inet_csk(sk); >> - >> - if (icsk->icsk_ca_ops->cwnd_event) >> - icsk->icsk_ca_ops->cwnd_event(sk, event); >> -} >> +/* from tcp_cong.c */ >> +void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event); >> >> /* From tcp_cong.c */ >> void tcp_set_ca_state(struct sock *sk, const u8 ca_state); >> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h >> index bf06db8d2046..38415c5f1d52 100644 >> --- a/include/trace/events/tcp.h >> +++ b/include/trace/events/tcp.h >> @@ -416,6 +416,51 @@ TRACE_EVENT(tcp_cong_state_set, >> __entry->cong_state) >> ); >> >> +TRACE_EVENT(tcp_ca_event_set, > > Regarding the proposed name, "tcp_ca_event_set"... including "set" in > the name is confusing, since the tcp_ca_event() function is not really > setting anything. :-) > > The trace_tcp_cong_state_set() call you are using as a model has "set" > in the name because the function it is tracing, tcp_set_ca_state(), > has "set" in the name. :-) > > Would it work to use something like: > TRACE_EVENT(tcp_ca_event, > > thanks, > neal Emmmm, sorry about the confusing var name and thanks for the review. I will update my patch ASAP. best regards Manjusaka
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);
In normal use case, the tcp_ca_event would be changed in high frequency. It's a good indicator to represent the network quanlity. So I propose to add a `tcp:tcp_ca_event_set` trace event like `tcp:tcp_cong_state_set` to help the people to trace the TCP connection status Signed-off-by: Manjusaka <me@manjusaka.me> --- include/net/tcp.h | 9 ++------ include/trace/events/tcp.h | 45 ++++++++++++++++++++++++++++++++++++++ net/ipv4/tcp_cong.c | 10 +++++++++ 3 files changed, 57 insertions(+), 7 deletions(-)