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

[Steven Rostedt <rostedt@goodmis.org>]

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 */
>