Re: [PATCH net-next 4/8] net: psample: add tracepoint

[Ido Schimmel]

On Wed, Apr 24, 2024 at 03:50:51PM +0200, Adrian Moreno wrote:
> Currently there are no widely-available tools to dump the metadata and
> group information when a packet is sampled, making it difficult to
> troubleshoot related issues.
> 
> This makes psample use the event tracing framework to log the sampling
> of a packet so that it's easier to quickly identify the source
> (i.e: group) and context (i.e: metadata) of a packet being sampled.
> 
> This patch creates some checkpatch splats, but the style of the
> tracepoint definition mimics that of other modules so it seems
> acceptable.

I don't see a good reason to add this tracepoint (which we won't be able
to remove) when you can easily do that with bpftrace which by now should
be widely available:

#!/usr/bin/bpftrace

kfunc:psample_sample_packet
{
        $ts_us = nsecs() / 1000;
        $secs = $ts_us / 1000000;
        $us = $ts_us % 1000000;
        $group = args.group;
        $skb = args.skb;
        $md = args.md;

        printf("%-16s %-6d %6llu.%6llu group_num = %u refcount=%u seq=%u skbaddr=%p len=%u data_len=%u sample_rate=%u in_ifindex=%d out_ifindex=%d user_cookie=%rx\n",
               comm, pid, $secs, $us, $group->group_num, $group->refcount, $group->seq,
               $skb, $skb->len, $skb->data_len, args.sample_rate,
               $md->in_ifindex, $md->out_ifindex,
               buf($md->user_cookie, $md->user_cookie_len));
}

Example output:

mausezahn        984      3299.200626 group_num = 1 refcount=1 seq=13775 skbaddr=0xffffa21143fd4000 len=42 data_len=0 sample_rate=10 in_ifindex=0 out_ifindex=20 user_cookie=
\xde\xad\xbe\xef
mausezahn        984      3299.281424 group_num = 1 refcount=1 seq=13776 skbaddr=0xffffa21143fd4000 len=42 data_len=0 sample_rate=10 in_ifindex=0 out_ifindex=20 user_cookie=
\xde\xad\xbe\xef

Note that it prints the cookie itself unlike the tracepoint which only
prints the hashed pointer.

> 
> Signed-off-by: Adrian Moreno <amorenoz@xxxxxxxxxx>
> ---
>  net/psample/psample.c |  9 +++++++
>  net/psample/trace.h   | 62 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 net/psample/trace.h
> 
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index 476aaad7a885..92db8ffa2ba2 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -18,6 +18,12 @@
>  #include <net/ip_tunnels.h>
>  #include <net/dst_metadata.h>
>  
> +#ifndef __CHECKER__
> +#define CREATE_TRACE_POINTS
> +#endif
> +
> +#include "trace.h"
> +
>  #define PSAMPLE_MAX_PACKET_SIZE 0xffff
>  
>  static LIST_HEAD(psample_groups_list);
> @@ -470,6 +476,9 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>  	void *data;
>  	int ret;
>  
> +	if (trace_psample_sample_packet_enabled())
> +		trace_psample_sample_packet(group, skb, sample_rate, md);

My understanding is that trace_x_enabled() should only be used if you
need to do some work to prepare parameters for the tracepoint.

> +
>  	meta_len = (in_ifindex ? nla_total_size(sizeof(u16)) : 0) +
>  		   (out_ifindex ? nla_total_size(sizeof(u16)) : 0) +
>  		   (md->out_tc_valid ? nla_total_size(sizeof(u16)) : 0) +