Re: [PATCH bpf V8 2/7] selftests/bpf: Add counters to xdp_hw_metadata

[Jesper Dangaard Brouer]

On 12/04/2023 18.56, Stanislav Fomichev wrote:
> On 04/12, Jesper Dangaard Brouer wrote:
> > Add counters for skipped, failed and redirected packets.
> > The xdp_hw_metadata program only redirects UDP port 9091.
> > This helps users to quickly identify then packets are
> > skipped and identify failures of bpf_xdp_adjust_meta.
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@xxxxxxxxxx>
> > ---
> >   .../testing/selftests/bpf/progs/xdp_hw_metadata.c  |   15 +++++++++++++--
> >   tools/testing/selftests/bpf/xdp_hw_metadata.c      |    4 +++-
> >   2 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > index b0104763405a..a07ef7534013 100644
> > --- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > +++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > @@ -25,6 +25,10 @@ struct {
> >   	__type(value, __u32);
> >   } xsk SEC(".maps");
> >   
> > +volatile __u64 pkts_skip = 0;
> > +volatile __u64 pkts_fail = 0;
> > +volatile __u64 pkts_redir = 0;
> > +
> >   extern int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx,
> >   					 __u64 *timestamp) __ksym;
> >   extern int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx,
> > @@ -59,16 +63,21 @@ int rx(struct xdp_md *ctx)
> >   			udp = NULL;
> >   	}
> >   
> > -	if (!udp)
> > +	if (!udp) {
> > +		pkts_skip++;
> >   		return XDP_PASS;
> > +	}
> >   
> >   	/* Forwarding UDP:9091 to AF_XDP */
> > -	if (udp->dest != bpf_htons(9091))
> > +	if (udp->dest != bpf_htons(9091)) {
> > +		pkts_skip++;
> >   		return XDP_PASS;
> > +	}
> >   
> >   	ret = bpf_xdp_adjust_meta(ctx, -(int)sizeof(struct xdp_meta));
> >   	if (ret != 0) {
>
> [..]
>
> >   		bpf_printk("bpf_xdp_adjust_meta returned %d", ret);
>
> Maybe let's remove these completely? Merge patch 1 and 2, remove printk,
> add counters. We can add more counters in the future if the existing
> ones are not enough.. WDYT?

Sure, lets just remove all of the bpf_printk, and add these counter instead.
Rolling V9.

> > +		pkts_fail++;

This fail counter should be enough for driver devel to realize that they
also need to implement/setup XDP metadata pointers correctly (for
bpf_xdp_adjust_meta to work).

> >   		return XDP_PASS;
> >   	}