Hi Ciara,
+1 to the overall approach. Few comments inline.
Thanks,
Anoob
> -----Original Message-----
> From: Ciara Power <ciara.po...@intel.com>
> Sent: Monday, December 13, 2021 8:34 PM
> To: dev@dpdk.org
> Cc: roy.fan.zh...@intel.com; Akhil Goyal <gak...@marvell.com>; Ciara
> Power <ciara.po...@intel.com>; Declan Doherty
> <declan.dohe...@intel.com>; Ankur Dwivedi <adwiv...@marvell.com>;
> Anoob Joseph <ano...@marvell.com>; Tejasree Kondoj
> <ktejas...@marvell.com>; John Griffin <john.grif...@intel.com>; Fiona
> Trahe <fiona.tr...@intel.com>; Deepak Kumar Jain
> <deepak.k.j...@intel.com>; Ray Kinsella <m...@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.po...@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
