| Message ID | 167940643669.2718137.4624187727245854475.stgit@firesoul (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | XDP-hints kfuncs for Intel driver igc | expand |
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/ 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). With this addressed: Acked-by: Stanislav Fomichev <sdf@google.com> > + * > * 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) { > >
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. > With this addressed: > Acked-by: Stanislav Fomichev <sdf@google.com> > >> + * >> * 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) { >> >> >
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.
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)
On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: > > 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) The only use of bpf_printk is for debugging of bpf progs themselves. It should not be used in any tool.
On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > 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) > > The only use of bpf_printk is for debugging of bpf progs themselves. > It should not be used in any tool. Hmm, good point. I guess it also means we won't have to mess with enabling/dumping ftrace (and don't need this comment about cat'ing the file). Jesper, maybe we can instead pass the status of those bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info from the userspace if needed.
On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev <sdf@google.com> wrote: > > On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > 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) > > > > The only use of bpf_printk is for debugging of bpf progs themselves. > > It should not be used in any tool. > > Hmm, good point. I guess it also means we won't have to mess with > enabling/dumping ftrace (and don't need this comment about cat'ing the > file). > Jesper, maybe we can instead pass the status of those > bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info > from the userspace if needed. There are so many other ways for bpf prog to communicate with user space. Use ringbuf, perf_event buffer, global vars, maps, etc. trace_pipe is debug only because it's global and will conflict with all other debug sessions.
On Wed, Mar 22, 2023 at 12:30 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > 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) > > > > > > The only use of bpf_printk is for debugging of bpf progs themselves. > > > It should not be used in any tool. > > > > Hmm, good point. I guess it also means we won't have to mess with > > enabling/dumping ftrace (and don't need this comment about cat'ing the > > file). > > Jesper, maybe we can instead pass the status of those > > bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info > > from the userspace if needed. > > There are so many other ways for bpf prog to communicate with user space. > Use ringbuf, perf_event buffer, global vars, maps, etc. > trace_pipe is debug only because it's global and will conflict with > all other debug sessions.
On 22/03/2023 20.33, Stanislav Fomichev wrote: > On Wed, Mar 22, 2023 at 12:30 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev <sdf@google.com> wrote: >>> >>> On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov >>> <alexei.starovoitov@gmail.com> wrote: >>>> >>>> On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: >>>>> >>>>> 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. I fully agree 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) My experience when developing these kfuncs for igc (real hardware), this "tool" xdp_hw_metadata was super helpful, because it was very verbose (and I was juggling reading chip registers BE/LE and see patch1 a buggy implementation for RX-hash). As I wrote in cover-letter, I recommend other driver developers to do the same, because it really help speed up the development. In theory xdp_hw_metadata doesn't belong in selftests directory and IMHO it should have been placed in samples/bpf/, but given the relationship with real selftest {progs,prog_tests}/xdp_metadata.c I think it makes sense to keep here. >>>> >>>> The only use of bpf_printk is for debugging of bpf progs themselves. >>>> It should not be used in any tool. >>> >>> Hmm, good point. I guess it also means we won't have to mess with >>> enabling/dumping ftrace (and don't need this comment about cat'ing the >>> file). >>> Jesper, maybe we can instead pass the status of those >>> bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info >>> from the userspace if needed. >> >> There are so many other ways for bpf prog to communicate with user space. >> Use ringbuf, perf_event buffer, global vars, maps, etc. >> trace_pipe is debug only because it's global and will conflict with >> all other debug sessions. I want to highlight above paragraph: It is very true for production code. (Anyone Googling this pay attention to above paragraph). > >
On Thu, Mar 23, 2023 at 1:51 AM Jesper Dangaard Brouer <jbrouer@redhat.com> wrote: > > > On 22/03/2023 20.33, Stanislav Fomichev wrote: > > On Wed, Mar 22, 2023 at 12:30 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > >> > >> On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev <sdf@google.com> wrote: > >>> > >>> On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov > >>> <alexei.starovoitov@gmail.com> wrote: > >>>> > >>>> On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: > >>>>> > >>>>> 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. > > I fully agree 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) > > My experience when developing these kfuncs for igc (real hardware), this > "tool" xdp_hw_metadata was super helpful, because it was very verbose > (and I was juggling reading chip registers BE/LE and see patch1 a buggy > implementation for RX-hash). > > As I wrote in cover-letter, I recommend other driver developers to do > the same, because it really help speed up the development. In theory > xdp_hw_metadata doesn't belong in selftests directory and IMHO it should > have been placed in samples/bpf/, but given the relationship with real > selftest {progs,prog_tests}/xdp_metadata.c I think it makes sense to > keep here. > > > >>>> > >>>> The only use of bpf_printk is for debugging of bpf progs themselves. > >>>> It should not be used in any tool. > >>> > >>> Hmm, good point. I guess it also means we won't have to mess with > >>> enabling/dumping ftrace (and don't need this comment about cat'ing the > >>> file). > >>> Jesper, maybe we can instead pass the status of those > >>> bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info > >>> from the userspace if needed. > >> > >> There are so many other ways for bpf prog to communicate with user space. > >> Use ringbuf, perf_event buffer, global vars, maps, etc. > >> trace_pipe is debug only because it's global and will conflict with > >> all other debug sessions. > > I want to highlight above paragraph: It is very true for production > code. (Anyone Googling this pay attention to above paragraph). > > > > >
On Thu, Mar 23, 2023 at 10:35 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Mar 23, 2023 at 1:51 AM Jesper Dangaard Brouer > <jbrouer@redhat.com> wrote: > > > > > > On 22/03/2023 20.33, Stanislav Fomichev wrote: > > > On Wed, Mar 22, 2023 at 12:30 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > >> > > >> On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev <sdf@google.com> wrote: > > >>> > > >>> On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov > > >>> <alexei.starovoitov@gmail.com> wrote: > > >>>> > > >>>> On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: > > >>>>> > > >>>>> 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. > > > > I fully agree 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) > > > > My experience when developing these kfuncs for igc (real hardware), this > > "tool" xdp_hw_metadata was super helpful, because it was very verbose > > (and I was juggling reading chip registers BE/LE and see patch1 a buggy > > implementation for RX-hash). > > > > As I wrote in cover-letter, I recommend other driver developers to do > > the same, because it really help speed up the development. In theory > > xdp_hw_metadata doesn't belong in selftests directory and IMHO it should > > have been placed in samples/bpf/, but given the relationship with real > > selftest {progs,prog_tests}/xdp_metadata.c I think it makes sense to > > keep here. > > > > > > >>>> > > >>>> The only use of bpf_printk is for debugging of bpf progs themselves. > > >>>> It should not be used in any tool. > > >>> > > >>> Hmm, good point. I guess it also means we won't have to mess with > > >>> enabling/dumping ftrace (and don't need this comment about cat'ing the > > >>> file). > > >>> Jesper, maybe we can instead pass the status of those > > >>> bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info > > >>> from the userspace if needed. > > >> > > >> There are so many other ways for bpf prog to communicate with user space. > > >> Use ringbuf, perf_event buffer, global vars, maps, etc. > > >> trace_pipe is debug only because it's global and will conflict with > > >> all other debug sessions. > > > > I want to highlight above paragraph: It is very true for production > > code. (Anyone Googling this pay attention to above paragraph). > > > > > > > >
On 23/03/2023 18.47, Stanislav Fomichev wrote: > On Thu, Mar 23, 2023 at 10:35 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Thu, Mar 23, 2023 at 1:51 AM Jesper Dangaard Brouer >> <jbrouer@redhat.com> wrote: >>> >>> >>> On 22/03/2023 20.33, Stanislav Fomichev wrote: >>>> On Wed, Mar 22, 2023 at 12:30 PM Alexei Starovoitov >>>> <alexei.starovoitov@gmail.com> wrote: >>>>> >>>>> On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev <sdf@google.com> wrote: >>>>>> >>>>>> On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov >>>>>> <alexei.starovoitov@gmail.com> wrote: >>>>>>> >>>>>>> On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@google.com> wrote: >>>>>>>> >>>>>>>> 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. >>> >>> I fully agree 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) >>> >>> My experience when developing these kfuncs for igc (real hardware), this >>> "tool" xdp_hw_metadata was super helpful, because it was very verbose >>> (and I was juggling reading chip registers BE/LE and see patch1 a buggy >>> implementation for RX-hash). >>> >>> As I wrote in cover-letter, I recommend other driver developers to do >>> the same, because it really help speed up the development. In theory >>> xdp_hw_metadata doesn't belong in selftests directory and IMHO it should >>> have been placed in samples/bpf/, but given the relationship with real >>> selftest {progs,prog_tests}/xdp_metadata.c I think it makes sense to >>> keep here. >>> >>> >>>>>>> >>>>>>> The only use of bpf_printk is for debugging of bpf progs themselves. >>>>>>> It should not be used in any tool. >>>>>> >>>>>> Hmm, good point. I guess it also means we won't have to mess with >>>>>> enabling/dumping ftrace (and don't need this comment about cat'ing the >>>>>> file). >>>>>> Jesper, maybe we can instead pass the status of those >>>>>> bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info >>>>>> from the userspace if needed. >>>>> >>>>> There are so many other ways for bpf prog to communicate with user space. >>>>> Use ringbuf, perf_event buffer, global vars, maps, etc. >>>>> trace_pipe is debug only because it's global and will conflict with >>>>> all other debug sessions. >>> >>> I want to highlight above paragraph: It is very true for production >>> code. (Anyone Googling this pay attention to above paragraph). >>> >>>> >>>>
On 03/24, Jesper Dangaard Brouer wrote: > On 23/03/2023 18.47, Stanislav Fomichev wrote: > > On Thu, Mar 23, 2023 at 10:35 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Thu, Mar 23, 2023 at 1:51 AM Jesper Dangaard Brouer > > > <jbrouer@redhat.com> wrote: > > > > > > > > > > > > On 22/03/2023 20.33, Stanislav Fomichev wrote: > > > > > On Wed, Mar 22, 2023 at 12:30 PM Alexei Starovoitov > > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > > > On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev > <sdf@google.com> wrote: > > > > > > > > > > > > > > On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov > > > > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > > > > > > > On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev > <sdf@google.com> wrote: > > > > > > > > > > > > > > > > > > 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. > > > > > > > > I fully agree 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) > > > > > > > > My experience when developing these kfuncs for igc (real hardware), > this > > > > "tool" xdp_hw_metadata was super helpful, because it was very > verbose > > > > (and I was juggling reading chip registers BE/LE and see patch1 a > buggy > > > > implementation for RX-hash). > > > > > > > > As I wrote in cover-letter, I recommend other driver developers to > do > > > > the same, because it really help speed up the development. In theory > > > > xdp_hw_metadata doesn't belong in selftests directory and IMHO it > should > > > > have been placed in samples/bpf/, but given the relationship with > real > > > > selftest {progs,prog_tests}/xdp_metadata.c I think it makes sense to > > > > keep here. > > > > > > > > > > > > > > > > > > > > > > > > The only use of bpf_printk is for debugging of bpf progs > themselves. > > > > > > > > It should not be used in any tool. > > > > > > > > > > > > > > Hmm, good point. I guess it also means we won't have to mess > with > > > > > > > enabling/dumping ftrace (and don't need this comment about > cat'ing the > > > > > > > file). > > > > > > > Jesper, maybe we can instead pass the status of those > > > > > > > bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump > this info > > > > > > > from the userspace if needed. > > > > > > > > > > > > There are so many other ways for bpf prog to communicate with > user space. > > > > > > Use ringbuf, perf_event buffer, global vars, maps, etc. > > > > > > trace_pipe is debug only because it's global and will conflict > with > > > > > > all other debug sessions. > > > > > > > > I want to highlight above paragraph: It is very true for production > > > > code. (Anyone Googling this pay attention to above paragraph). > > > > > > > > > > > > > >
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 + * * 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) {
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(-)