Re: [PATCH v3] tracepoint: add new `tcp:tcp_ca_event` trace event

From: Steven Rostedt
Date: Sat Aug 12 2023 - 20:59:13 EST


On Sat, 12 Aug 2023 20:12:50 +0000
Zheao Li <me@xxxxxxxxxxxx> 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 */
>