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

[Olivier Matz <olivier.matz@6wind.com>]

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>
> ---
>  doc/guides/prog_guide/rte_security.rst        |  9 +++---
>  drivers/crypto/octeontx2/otx2_cryptodev_sec.c |  5 ++-
>  drivers/net/ixgbe/ixgbe_ipsec.c               |  5 ++-
>  drivers/net/ixgbe/ixgbe_rxtx.c                |  6 ++--
>  drivers/net/octeontx2/otx2_ethdev.h           |  1 +
>  drivers/net/octeontx2/otx2_ethdev_sec.c       |  5 ++-
>  drivers/net/octeontx2/otx2_ethdev_sec_tx.h    |  2 +-
>  drivers/net/octeontx2/otx2_rx.h               |  2 +-
>  examples/ipsec-secgw/ipsec-secgw.c            |  9 +++---
>  examples/ipsec-secgw/ipsec_worker.c           | 12 ++++---
>  lib/librte_security/rte_security.c            | 22 +++++++++++++
>  lib/librte_security/rte_security.h            | 32 +++++++++++++++++++
>  lib/librte_security/rte_security_driver.h     |  3 ++
>  lib/librte_security/version.map               |  3 ++
>  14 files changed, 96 insertions(+), 20 deletions(-)
> 

<...>

> --- a/examples/ipsec-secgw/ipsec_worker.c
> +++ b/examples/ipsec-secgw/ipsec_worker.c
> @@ -208,7 +208,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  					"Inbound security offload failed\n");
>  				goto drop_pkt_and_exit;
>  			}
> -			sa = pkt->userdata;
> +			sa = *(struct ipsec_sa **)rte_security_dynfield(pkt);
>  		}
>  
>  		/* Check if we have a match */
> @@ -226,7 +226,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  					"Inbound security offload failed\n");
>  				goto drop_pkt_and_exit;
>  			}
> -			sa = pkt->userdata;
> +			sa = *(struct ipsec_sa **)rte_security_dynfield(pkt);
>  		}
>  
>  		/* Check if we have a match */
> @@ -357,7 +357,8 @@ process_ipsec_ev_outbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  	}
>  
>  	if (sess->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA)
> -		pkt->userdata = sess->security.ses;
> +		*(struct rte_security_session **)rte_security_dynfield(pkt) =
> +				sess->security.ses;
>  
>  	/* Mark the packet for Tx security offload */
>  	pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> @@ -465,7 +466,10 @@ ipsec_wrkr_non_burst_int_port_drv_mode(struct eh_event_link_info *links,
>  			}
>  
>  			/* 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).

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

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.


>  			/* Mark the packet for Tx security offload */
>  			pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> index ee4666026a..4fb0b797e9 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -23,6 +23,28 @@
>  	RTE_PTR_OR_ERR_RET(p1->p2->p3, last_retval);			\
>  } while (0)
>  
> +#define RTE_SECURITY_DYNFIELD_NAME "rte_security_dynfield_metadata"
> +int rte_security_dynfield_offset = -1;
> +
> +int
> +rte_security_dynfield_register(void)
> +{
> +	static const struct rte_mbuf_dynfield dynfield_desc = {
> +		.name = RTE_SECURITY_DYNFIELD_NAME,
> +		.size = sizeof(RTE_SECURITY_DYNFIELD_TYPE),
> +		.align = __alignof__(RTE_SECURITY_DYNFIELD_TYPE),
> +	};
> +	rte_security_dynfield_offset =
> +		rte_mbuf_dynfield_register(&dynfield_desc);
> +	return rte_security_dynfield_offset;
> +}
> +
> +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.


>  struct rte_security_session *
>  rte_security_session_create(struct rte_security_ctx *instance,
>  			    struct rte_security_session_conf *conf,
> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
> index 271531af12..7fbdee99cc 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -27,6 +27,7 @@ extern "C" {
>  #include <rte_common.h>
>  #include <rte_crypto.h>
>  #include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
>  #include <rte_memory.h>
>  #include <rte_mempool.h>
>  
> @@ -451,6 +452,37 @@ int
>  rte_security_session_destroy(struct rte_security_ctx *instance,
>  			     struct rte_security_session *sess);
>  
> +/** 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