[xdp-hints] Re: [PATCH bpf-next V2 3/6] selftests/bpf: xdp_hw_metadata RX hash return code info

[Stanislav Fomichev <sdf@google.com>]

On Wed, Mar 22, 2023 at 9:07 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Mar 22, 2023 at 9:05 AM Jesper Dangaard Brouer
> <jbrouer@redhat.com> wrote:
> >
> >
> >
> > On 21/03/2023 19.47, Stanislav Fomichev wrote:
> > > On Tue, Mar 21, 2023 at 6:47 AM Jesper Dangaard Brouer
> > > <brouer@redhat.com> 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 <brouer@redhat.com>
> > >> ---
> > >>   .../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 */
> > >> +       }
> > >>
> > >>          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..f3ec07ccdc95 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 outout can be access via
> > >> + * /sys/kernel/debug/tracing/trace_pipe
> > >
> > > s/outout/output/
> > >
> >
> > Fixed in V3
> >
> > > But let's maybe drop it? If you want to make it more usable, let's
> > > have a separate patch to enable tracing and periodically dump it to
> > > the console instead (as previously discussed).
> >
> > Cat'ing /sys/kernel/debug/tracing/trace_pipe work for me regardless of
> > setting in
> > /sys/kernel/debug/tracing/events/bpf_trace/bpf_trace_printk/enable
> >
> > We likely need a followup patch that adds a BPF config switch that can
> > disable bpf_printk calls, because this adds overhead and thus affects
> > the timestamps.
>
> No. This is by design.
> Do not use bpf_printk* in production.

But that's not for the production? xdp_hw_metadata is a small tool to
verify that the metadata being dumped is correct (during the
development).
We have a proper (less verbose) selftest in
{progs,prog_tests}/xdp_metadata.c (over veth).
This xdp_hw_metadata was supposed to be used for running it against
the real hardware, so having as much debugging at hand as possible
seems helpful? (at least it was helpful to me when playing with mlx4)