On Thu, Jul 06, 2023 at 12:25:10PM +0200, Jesper Dangaard Brouer wrote:
>
>
> On 03/07/2023 20.12, Larysa Zaremba wrote:
> > Verify, whether kfunc in xdp_metadata test correctly returns checksum level
> > of zero.
> >
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> > ---
> > tools/testing/selftests/bpf/prog_tests/xdp_metadata.c | 3 +++
> > tools/testing/selftests/bpf/progs/xdp_metadata.c | 7 +++++++
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> > index 50ac9f570bc5..6c71d712932e 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> > @@ -228,6 +228,9 @@ static int verify_xsk_metadata(struct xsk *xsk)
> > if (!ASSERT_EQ(meta->rx_vlan_proto, VLAN_PID, "rx_vlan_proto"))
> > return -1;
> > + if (!ASSERT_NEQ(meta->rx_csum_lvl, 0, "rx_csum_lvl"))
> > + return -1;
>
> Not-equal ("NEQ") to 0 feels weird here.
> Below you set meta->rx_csum_lvl=1 in case meta->rx_csum_lvl==0.
>
> Thus, test can pass if meta->rx_csum_lvl happens to be a random value.
> We could set meta->rx_csum_lvl to 42 in case meta->rx_csum_lvl==0, and
> then use a ASSERT_EQ==42 to be more certain of the case we are testing are
> fulfilled.
>
I just copied the approach used for timestamp. I think you are right and I
should have make the new code better.
ASSERT_NEQ(0) is also used for rx_hash. It would be a good idea to go and fix
those too, but the patchset has already ballooned too much for me, so I would
leave it for later.
With ASSERT_EQ for checksum level, I think comparing it to "1" should be enough.
Do I guess correctly, the main problem with ASSERT_NEQ is uninitialized memory?
Another value that is less magical than 42 would be "4", because csum_level
takes 2 bits, so it is the smallest value that does not correspod to
any valid checksum level.
>
> > +
> > xsk_ring_cons__release(&xsk->rx, 1);
> > refill_rx(xsk, comp_addr);
> > diff --git a/tools/testing/selftests/bpf/progs/xdp_metadata.c b/tools/testing/selftests/bpf/progs/xdp_metadata.c
> > index 382984a5d1c9..6f7223d581b7 100644
> > --- a/tools/testing/selftests/bpf/progs/xdp_metadata.c
> > +++ b/tools/testing/selftests/bpf/progs/xdp_metadata.c
> > @@ -26,6 +26,8 @@ extern int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, __u32 *hash,
> > extern int bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx,
> > __u16 *vlan_tag,
> > __be16 *vlan_proto) __ksym;
> > +extern int bpf_xdp_metadata_rx_csum_lvl(const struct xdp_md *ctx,
> > + __u8 *csum_level) __ksym;
> > SEC("xdp")
> > int rx(struct xdp_md *ctx)
> > @@ -62,6 +64,11 @@ int rx(struct xdp_md *ctx)
> > bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash, &meta->rx_hash_type);
> > bpf_xdp_metadata_rx_vlan_tag(ctx, &meta->rx_vlan_tag, &meta->rx_vlan_proto);
> > + /* Same as with timestamp, zero is expected */
> > + ret = bpf_xdp_metadata_rx_csum_lvl(ctx, &meta->rx_csum_lvl);
> > + if (!ret && meta->rx_csum_lvl == 0)
> > + meta->rx_csum_lvl = 1;
> > +
>
> IMHO it is more human-readable-code to rename "ret" variable "err".
>
> I know you are just reusing variable "ret", so it's not really your fault.
>
>
>
> > return bpf_redirect_map(&xsk, ctx->rx_queue_index, XDP_PASS);
> > }
>
