| Message ID | 20230703181226.19380-21-larysa.zaremba@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | XDP metadata via kfuncs for ice | expand |
On 07/03, Larysa Zaremba wrote: > Verify, whether kfunc in xdp_metadata test correctly returns checksum level > of zero. > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> Acked-by: Stanislav Fomichev <sdf@google.com> > --- > 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; > + > 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; > + > return bpf_redirect_map(&xsk, ctx->rx_queue_index, XDP_PASS); > } > > -- > 2.41.0 >
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@intel.com> > --- > 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. > + > 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); > } >
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@intel.com> > > --- > > 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); > > } >
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; + 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; + return bpf_redirect_map(&xsk, ctx->rx_queue_index, XDP_PASS); }
Verify, whether kfunc in xdp_metadata test correctly returns checksum level of zero. Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- tools/testing/selftests/bpf/prog_tests/xdp_metadata.c | 3 +++ tools/testing/selftests/bpf/progs/xdp_metadata.c | 7 +++++++ 2 files changed, 10 insertions(+)