On Sun, 13 Aug 2023 04:17:24 +0800
Manjusaka <me@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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
