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

[Thomas Monjalon <thomas@monjalon.net>]

27/10/2020 11:05, Olivier Matz:
> On Mon, Oct 26, 2020 at 11:20:03PM +0100, Thomas Monjalon 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.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> <...>
> >  			/* Save security session */
> > -			pkt->userdata = sess_tbl[port_id];
> > +			if (rte_security_dynfield_is_registered())
> > +				*(struct rte_security_session **)
> > +					rte_security_dynfield(pkt) =
> > +						sess_tbl[port_id];
> >  
> 
> Maybe the last 2 lines can be on the same line (a bit more than 80,
> but less than 100 chars).

I prefer limiting to 80 if possible.

> This is not clear to me why you need to call
> rte_security_dynfield_is_registered() here, and not in other places.

I think other places are paths for rte_security only.

> Would it make sense instead to always register the dynfield at some
> place in rte_security, so that we are sure that as soon as we use
> rte_security, the dynfield is registered. A good place would be an init
> function, but I don't see one.

Indeed there is no such place in rte_security.
I keep thinking this is not a real library.

> > +bool
> > +rte_security_dynfield_is_registered(void)
> > +{
> > +	return rte_security_dynfield_offset >= 0;
> > +}
> > +
> 
> Wouldn't it be better to have it in a static inline function?
> The point is just to check that offset is >= 0, and it is used
> in data path.

Yes I'll make it inline

> > +/** Device-specific metadata field type */
> > +#define RTE_SECURITY_DYNFIELD_TYPE uint64_t
> 
> What about using a typedef instead of a #define?
> It could be in lowercase: rte_security_dynfield_t

Yes