[bpf-next,V3,3/6] selftests/bpf: xdp_hw_metadata RX hash return code info

Commit Message

Jesper Dangaard Brouer March 22, 2023, 4:01 p.m. UTC

When driver developers add XDP-hints kfuncs for RX hash it is
practical to print the return code in bpf_printk trace pipe log.

Print hash value as a hex value, both AF_XDP userspace and bpf_prog,
as this makes it easier to spot poor quality hashes.

Signed-off-by: Jesper Dangaard Brouer <[e-mail]>
Acked-by: Stanislav Fomichev <[e-mail]>
---
 .../testing/selftests/bpf/progs/xdp_hw_metadata.c  |    9 ++++++---
 tools/testing/selftests/bpf/xdp_hw_metadata.c      |    5 ++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

Comments

Alexei Starovoitov March 22, 2023, 4:09 p.m. UTC | #1

On Wed, Mar 22, 2023 at 9:01 AM Jesper Dangaard Brouer
<[e-mail]> wrote:
>
> When driver developers add XDP-hints kfuncs for RX hash it is
> practical to print the return code in bpf_printk trace pipe log.
>
> Print hash value as a hex value, both AF_XDP userspace and bpf_prog,
> as this makes it easier to spot poor quality hashes.
>
> Signed-off-by: Jesper Dangaard Brouer <[e-mail]>
> Acked-by: Stanislav Fomichev <[e-mail]>
> ---
>  .../testing/selftests/bpf/progs/xdp_hw_metadata.c  |    9 ++++++---
>  tools/testing/selftests/bpf/xdp_hw_metadata.c      |    5 ++++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> index 40c17adbf483..ce07010e4d48 100644
> --- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> +++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> @@ -77,10 +77,13 @@ int rx(struct xdp_md *ctx)
>                 meta->rx_timestamp = 0; /* Used by AF_XDP as not avail signal */
>         }
>
> -       if (!bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash))
> -               bpf_printk("populated rx_hash with %u", meta->rx_hash);
> -       else
> +       ret = bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash);
> +       if (ret >= 0) {
> +               bpf_printk("populated rx_hash with 0x%08X", meta->rx_hash);
> +       } else {
> +               bpf_printk("rx_hash not-avail errno:%d", ret);
>                 meta->rx_hash = 0; /* Used by AF_XDP as not avail signal */
> +       }

Just noticed this mess of printks.
Please remove them all. selftests should not have them.

Stanislav Fomichev March 22, 2023, 7 p.m. UTC | #2

On Wed, Mar 22, 2023 at 9:09 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Wed, Mar 22, 2023 at 9:01 AM Jesper Dangaard Brouer
> <[e-mail]> wrote:
> >
> > When driver developers add XDP-hints kfuncs for RX hash it is
> > practical to print the return code in bpf_printk trace pipe log.
> >
> > Print hash value as a hex value, both AF_XDP userspace and bpf_prog,
> > as this makes it easier to spot poor quality hashes.
> >
> > Signed-off-by: Jesper Dangaard Brouer <[e-mail]>
> > Acked-by: Stanislav Fomichev <[e-mail]>
> > ---
> >  .../testing/selftests/bpf/progs/xdp_hw_metadata.c  |    9 ++++++---
> >  tools/testing/selftests/bpf/xdp_hw_metadata.c      |    5 ++++-
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > index 40c17adbf483..ce07010e4d48 100644
> > --- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > +++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > @@ -77,10 +77,13 @@ int rx(struct xdp_md *ctx)
> >                 meta->rx_timestamp = 0; /* Used by AF_XDP as not avail signal */
> >         }
> >
> > -       if (!bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash))
> > -               bpf_printk("populated rx_hash with %u", meta->rx_hash);
> > -       else
> > +       ret = bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash);
> > +       if (ret >= 0) {
> > +               bpf_printk("populated rx_hash with 0x%08X", meta->rx_hash);
> > +       } else {
> > +               bpf_printk("rx_hash not-avail errno:%d", ret);
> >                 meta->rx_hash = 0; /* Used by AF_XDP as not avail signal */
> > +       }
>
> Just noticed this mess of printks.
> Please remove them all. selftests should not have them.

See my reply in the v2.

Patch

diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
index 40c17adbf483..ce07010e4d48 100644
--- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
@@ -77,10 +77,13 @@  int rx(struct xdp_md *ctx)
 		meta->rx_timestamp = 0; /* Used by AF_XDP as not avail signal */
 	}
 
-	if (!bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash))
-		bpf_printk("populated rx_hash with %u", meta->rx_hash);
-	else
+	ret = bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash);
+	if (ret >= 0) {
+		bpf_printk("populated rx_hash with 0x%08X", meta->rx_hash);
+	} else {
+		bpf_printk("rx_hash not-avail errno:%d", ret);
 		meta->rx_hash = 0; /* Used by AF_XDP as not avail signal */
+	}
 
 	return bpf_redirect_map(&xsk, ctx->rx_queue_index, XDP_PASS);
 }
diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
index 400bfe19abfe..b7e39ff15788 100644
--- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
@@ -3,6 +3,9 @@ 
 /* Reference program for verifying XDP metadata on real HW. Functional test
  * only, doesn't test the performance.
  *
+ * BPF-prog bpf_printk info output can be access via
+ * /sys/kernel/debug/tracing/trace_pipe
+ *
  * RX:
  * - UDP 9091 packets are diverted into AF_XDP
  * - Metadata verified:
@@ -156,7 +159,7 @@  static void verify_xdp_metadata(void *data, clockid_t clock_id)
 
 	meta = data - sizeof(*meta);
 
-	printf("rx_hash: %u\n", meta->rx_hash);
+	printf("rx_hash: 0x%08X\n", meta->rx_hash);
 	printf("rx_timestamp:  %llu (sec:%0.4f)\n", meta->rx_timestamp,
 	       (double)meta->rx_timestamp / NANOSEC_PER_SEC);
 	if (meta->rx_timestamp) {