RE: [EXT] [PATCH] crypto: use single buffer for asymmetric session

[Anoob Joseph <anoobj@marvell.com>]

Hi Ciara,

+1 to the overall approach. Few comments inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ciara Power <ciara.power@intel.com>
> Sent: Monday, December 13, 2021 8:34 PM
> To: dev@dpdk.org
> Cc: roy.fan.zhang@intel.com; Akhil Goyal <gakhil@marvell.com>; Ciara
> Power <ciara.power@intel.com>; Declan Doherty
> <declan.doherty@intel.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> Anoob Joseph <anoobj@marvell.com>; Tejasree Kondoj
> <ktejasree@marvell.com>; John Griffin <john.griffin@intel.com>; Fiona
> Trahe <fiona.trahe@intel.com>; Deepak Kumar Jain
> <deepak.k.jain@intel.com>; Ray Kinsella <mdr@ashroe.eu>
> Subject: [EXT] [PATCH] crypto: use single buffer for asymmetric session
> 
> External Email
> 
> ----------------------------------------------------------------------
> Rather than using a session buffer that contains pointers to private session
> data elsewhere, have a single session buffer.
> This session is created for a driver ID, and the mempool element contains
> space for the max session private data needed for any driver.
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> 
> ---
> Hiding the asym session structure by moving it to an internal header will be
> implemented in a later version of this patch.
> ---
>  app/test-crypto-perf/cperf_ops.c              |  14 +-
>  app/test/test_cryptodev_asym.c                | 204 ++++--------------
>  drivers/crypto/cnxk/cn10k_cryptodev_ops.c     |   6 +-
>  drivers/crypto/cnxk/cn9k_cryptodev_ops.c      |   6 +-
>  drivers/crypto/cnxk/cnxk_cryptodev_ops.c      |  11 +-
>  drivers/crypto/octeontx/otx_cryptodev_ops.c   |  29 +--
>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c |  25 +--
>  drivers/crypto/openssl/rte_openssl_pmd.c      |   5 +-
>  drivers/crypto/openssl/rte_openssl_pmd_ops.c  |  23 +-
>  drivers/crypto/qat/qat_asym.c                 |  35 +--
>  lib/cryptodev/cryptodev_pmd.h                 |  11 +-
>  lib/cryptodev/cryptodev_trace_points.c        |   3 +
>  lib/cryptodev/rte_cryptodev.c                 | 199 +++++++++++------
>  lib/cryptodev/rte_cryptodev.h                 | 107 ++++++---
>  lib/cryptodev/rte_cryptodev_trace.h           |  12 ++
>  lib/cryptodev/version.map                     |   6 +-
>  16 files changed, 302 insertions(+), 394 deletions(-)
> 

[snip]

> diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
> index 59ea5a54df..11a62bb555 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -919,9 +919,15 @@ struct rte_cryptodev_sym_session {  };
> 
>  /** Cryptodev asymmetric crypto session */ -struct
> rte_cryptodev_asym_session {
> -	__extension__ void *sess_private_data[0];
> -	/**< Private asymmetric session material */
> +__extension__ struct rte_cryptodev_asym_session {
> +	uint8_t driver_id;
> +	/**< Session driver ID. */
> +	uint8_t max_priv_session_sz;
> +	/**< size of private session data used when creating mempool */
> +	uint16_t user_data_sz;
> +	/**< session user data will be placed after sess_data */
> +	uint8_t padding[4];
> +	uint8_t sess_private_data[0];
>  };

[Anoob] Should we add a uint64_t member to hold IOVA address of, may be, rte_cryptodev_asym_session()? IOVA address could be required for hardware PMDs. And typically rte_mempool_virt2iova() used to help in that. Also, did you consider whether this layout of crypto session can be kept uniform across sym, asym & security? There is no asym specific field in this struct, right?

> 
>  /**
> @@ -956,6 +962,31 @@ rte_cryptodev_sym_session_pool_create(const
> char *name, uint32_t nb_elts,
>  	uint32_t elt_size, uint32_t cache_size, uint16_t priv_size,
>  	int socket_id);
> 
> +/**
> + * Create an asymmetric session mempool.
> + *
> + * @param name
> + *   The unique mempool name.
> + * @param nb_elts
> + *   The number of elements in the mempool.
> + * @param cache_size
> + *   The number of per-lcore cache elements
> + * @param user_data_size
> + *   The size of user data to be placed after session private data.
> + * @param socket_id
> + *   The *socket_id* argument is the socket identifier in the case of
> + *   NUMA. The value can be *SOCKET_ID_ANY* if there is no NUMA
> + *   constraint for the reserved zone.
> + *
> + * @return
> + *  - On success return size of the session
> + *  - On failure returns 0
> + */
> +__rte_experimental
> +struct rte_mempool *
> +rte_cryptodev_asym_session_pool_create(const char *name, uint32_t
> nb_elts,
> +	uint32_t cache_size, uint16_t user_data_size, int socket_id);
> +
>  /**
>   * Create symmetric crypto session header (generic with no private data)
>   *
> @@ -973,13 +1004,17 @@ rte_cryptodev_sym_session_create(struct
> rte_mempool *mempool);
>   *
>   * @param   mempool    mempool to allocate asymmetric session
>   *                     objects from
> + * @param   dev_id   ID of device that we want the session to be used on
> + * @param   xforms   Asymmetric crypto transform operations to apply on
> flow
> + *                   processed with this session
>   * @return
>   *  - On success return pointer to asym-session
>   *  - On failure returns NULL
>   */
>  __rte_experimental
>  struct rte_cryptodev_asym_session *
> -rte_cryptodev_asym_session_create(struct rte_mempool *mempool);
> +rte_cryptodev_asym_session_create(struct rte_mempool *mempool,
> +		uint8_t dev_id, struct rte_crypto_asym_xform *xforms);
> 
>  /**
>   * Frees symmetric crypto session header, after checking that all @@ -
> 1034,28 +1069,6 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
>  			struct rte_crypto_sym_xform *xforms,
>  			struct rte_mempool *mempool);
> 
> -/**
> - * Initialize asymmetric session on a device with specific asymmetric xform
> - *
> - * @param   dev_id   ID of device that we want the session to be used on
> - * @param   sess     Session to be set up on a device
> - * @param   xforms   Asymmetric crypto transform operations to apply on
> flow
> - *                   processed with this session
> - * @param   mempool  Mempool to be used for internal allocation.
> - *
> - * @return
> - *  - On success, zero.
> - *  - -EINVAL if input parameters are invalid.
> - *  - -ENOTSUP if crypto device does not support the crypto transform.

[Anoob] API rte_cryptodev_asym_session_create() returning NULL is treated as an error. But error can be either due to -EINVAL/-ENOMEM/-ENOTSUP, in which -ENOTSUP is typically used by PMD to declare unsupported combinations of xforms. Should we clarify this in the API description? 

Also, none of rte_cryptodev_asym_session_create() calls in validation tests consider the API returning NULL due to -ENOTSUP. For sym crypto test cases, API returning -ENOTSUP was used to skip the test. Can you update the tests such that returning NULL would mean test is skipped? Agreed that current code also doesn't handle -ENOTSUP case returned by init API.
 
> - *  - -ENOMEM if the private session could not be allocated.
> - */
> -__rte_experimental
> -int
> -rte_cryptodev_asym_session_init(uint8_t dev_id,
> -			struct rte_cryptodev_asym_session *sess,
> -			struct rte_crypto_asym_xform *xforms,
> -			struct rte_mempool *mempool);
> -
>  /**
>   * Frees private data for the device id, based on its device type,
>   * returning it to its mempool. It is the application's responsibility @@ -
> 1075,14 +1088,13 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
>  			struct rte_cryptodev_sym_session *sess);
> 

[snip]

> diff --git a/lib/cryptodev/version.map b/lib/cryptodev/version.map index
> c50745fa8c..00b1c9ae35 100644
> --- a/lib/cryptodev/version.map
> +++ b/lib/cryptodev/version.map
> @@ -58,7 +58,6 @@ EXPERIMENTAL {
>  	rte_cryptodev_asym_session_clear;
>  	rte_cryptodev_asym_session_create;
>  	rte_cryptodev_asym_session_free;
> -	rte_cryptodev_asym_session_init;
>  	rte_cryptodev_asym_xform_capability_check_modlen;
>  	rte_cryptodev_asym_xform_capability_check_optype;
>  	rte_cryptodev_sym_cpu_crypto_process;
> @@ -104,6 +103,11 @@ EXPERIMENTAL {
>  	rte_cryptodev_remove_deq_callback;
>  	rte_cryptodev_remove_enq_callback;
> 
> +	# added 22.03

+1 for get & set user_data API. Ideally it should have been a separate series but I agree that it's better getting addressed along with the session rework.

> +	rte_cryptodev_asym_session_pool_create;
> +	rte_cryptodev_asym_session_get_user_data;
> +	rte_cryptodev_asym_session_set_user_data;
> +	__rte_cryptodev_trace_asym_session_pool_create;
>  };
> 
>  INTERNAL {
> --
> 2.25.1