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

[Thomas Monjalon <thomas@monjalon.net>]

26/10/2020 16:06, Andrew Rybchenko:
> On 10/26/20 8:20 AM, Thomas Monjalon wrote:
> > --- a/drivers/net/ixgbe/ixgbe_ipsec.c
> > +++ b/drivers/net/ixgbe/ixgbe_ipsec.c
> > -			(union ixgbe_crypto_tx_desc_md *)&m->udata64;
> > +			(union ixgbe_crypto_tx_desc_md *)
> > +			rte_security_dynfield(m);
> 
> IMHO alignment looks a bit confusing here, may be add one more
> TAB?

OK, no opinion

> [snip]
> > -			sa = pkt->userdata;
> > +			sa = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> > +					struct ipsec_sa *);
> 
> I think it should be de-reference above, i.e.
> sa = (struct ipsec_sa *)*RTE_MBUF_DYNFIELD(pkt,
> security_dynfield_offset, RTE_SECURITY_DYNFIELD_TYPE *);
> or just
> sa = *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, struct ipsec_sa **);

You're right something is wrong.
I should add a dereference.

> and why not rte_security_dynfield()?

Because I need good reviews from you and David :)
Will fix one and the other occurences.
I've prepared a v2 using a macro, but I think I will follow
your suggestion of having static inline functions.

> It all looks very fragile. May be at least add
>    RTE_BUILD_BUG_ON(sizeof(RTE_SECURITY_DYNFIELD_TYPE) ==
>                     sizeof(ipsec_sa *));
> and similar checks when an application or a library does
> lookup for a dynamic field.

You mean adding this after the lookup in the app?

> In general since lookup should not happen on data path,
> may be lookup should return size of the field which must
> be checked by the caller for consistency.

It does. I can add a check.

> Is it really different types of value in one example
> application? It looks like it should be different
> dynamic fields. Otherwise, I don't understand how
> to work with it.

Yes, udata64 was a trash bin used to store different kind of data,
even inside librte_security.

> In fact may be above is out of scope of the patch series...

Yes, I think moving to a dedicated field is a first step.
Second step (not by me) will be to split in different fields.