Re: [dpdk-dev] [PATCH v6 05/15] security: switch metadata to dynamic mbuf field

[Thomas Monjalon <thomas@monjalon.net>]

31/10/2020 09:56, David Marchand:
> On Fri, Oct 30, 2020 at 6:45 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > The device-specific metadata was stored in the deprecated field udata64.
> > It is moved to a dynamic mbuf field in order to allow removal of udata64.
> >
> > The name rte_security_dynfield is not very descriptive
> > but it should be replaced later by separate fields for each type of data
> > that drivers pass to the upper layer.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> 
> We have a regression in the mbuf unit test when running with probed
> ixgbe devices (UNH lab).
> 
> The problem is that the security dyn field is registered when probing
> the ixgbe devices.
> The unit test fails later while checking one of its own registered
> dynfield offset.
> 
> I can reproduce the issue by stopping at this patch in the series and
> adding the below diff.
> The problem can be seen with the whole series applied, there is only a
> difference in reported dynfield offsets.
> 
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index 80d1850da9..0e4f895187 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -33,6 +33,7 @@
>  #include <rte_ip.h>
>  #include <rte_tcp.h>
>  #include <rte_mbuf_dyn.h>
> +#include <rte_security_driver.h>
> 
>  #include "test.h"
> 
> @@ -2589,6 +2590,7 @@ test_mbuf_dyn(struct rte_mempool *pktmbuf_pool)
>         int ret;
> 
>         printf("Test mbuf dynamic fields and flags\n");
> +       rte_security_dynfield_register();
>         rte_mbuf_dyn_dump(stdout);
> 
>         offset = rte_mbuf_dynfield_register(&dynfield);
> 
> 
> Then:
> 
> # DPDK_TEST=mbuf_autotest ./build/app/test/dpdk-test
> EAL: Detected 8 lcore(s)
> EAL: Detected 1 NUMA nodes
> EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'PA'
> EAL: No available hugepages reported in hugepages-1048576kB
> EAL: Probing VFIO support...
> APP: HPET is not enabled, using TSC as default timer
> RTE>>mbuf_autotest
> Test mbuf dynamic fields and flags
> Reserved fields:
>   name=rte_security_dynfield_metadata offset=112 size=8 align=8 flags=0
> Reserved flags:
> Free space in mbuf (0 = occupied, value = free zone alignment):
>   0000: 00 00 00 00 00 00 00 00
>   0008: 00 00 00 00 00 00 00 00
>   0010: 00 00 00 00 00 00 00 00
>   0018: 00 00 00 00 00 00 00 00
>   0020: 00 00 00 00 00 00 00 00
>   0028: 00 00 00 00 00 00 00 00
>   0030: 00 00 00 00 00 00 00 00
>   0038: 00 00 00 00 00 00 00 00
>   0040: 00 00 00 00 00 00 00 00
>   0048: 00 00 00 00 00 00 00 00
>   0050: 00 00 00 00 00 00 00 00
>   0058: 00 00 00 00 00 00 00 00
>   0060: 00 00 00 00 00 00 00 00
>   0068: 00 00 00 00 00 00 00 00
>   0070: 00 00 00 00 00 00 00 00
>   0078: 08 08 08 08 08 08 08 08
> Free bit in mbuf->ol_flags (0 = occupied, 1 = free):
>   0000: 0 0 0 0 0 0 0 0
>   0008: 0 0 0 0 0 0 0 0
>   0010: 0 0 0 0 0 0 0 1
>   0018: 1 1 1 1 1 1 1 1
>   0020: 1 1 1 1 1 1 1 1
>   0028: 1 0 0 0 0 0 0 0
>   0030: 0 0 0 0 0 0 0 0
>   0038: 0 0 0 0 0 0 0 0
> mbuf test FAILED (l.2615): <failed to register dynamic field 3,
> offset=-1: No such file or directory>
> mbuf dynflag test failed
> Test Failed
> 
> A lazy fix is to simply ask for registering a field at offset
> sizeof(dynfield) - 1 .. what do you think?

I think any offset could be registered,
so it looks more correct to skip the failure if rte_errno is EBUSY.
I did this fix:
	https://patches.dpdk.org/patch/83175/