Re: [dpdk-dev] [PATCH v4 02/12] cryptodev: add sym session mempool create

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

> -----Original Message-----
> From: Zhang, Roy Fan
> Sent: Wednesday, January 9, 2019 10:56 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: [PATCH v4 02/12] cryptodev: add sym session mempool create
> 
> This patch adds a new API "rte_cryptodev_sym_session_pool_create()" to
> cryptodev library. All applications are required to use this API to create sym
> session mempool as it adds private data and nb_drivers information to the
> mempool private data.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> Acked-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
>  doc/guides/prog_guide/cryptodev_lib.rst        | 28 ++++++++++-----
>  doc/guides/rel_notes/release_19_02.rst         |  5 +++
>  lib/librte_cryptodev/rte_cryptodev.c           | 50
> ++++++++++++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev.h           | 31 ++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev_version.map |  7 ++++
>  5 files changed, 112 insertions(+), 9 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/cryptodev_lib.rst
> b/doc/guides/prog_guide/cryptodev_lib.rst
> index 050a58dc4..366508618 100644
> --- a/doc/guides/prog_guide/cryptodev_lib.rst
> +++ b/doc/guides/prog_guide/cryptodev_lib.rst
> @@ -455,13 +455,15 @@ Crypto workloads.
> 
>  .. figure:: img/cryptodev_sym_sess.*
> 
> -The Crypto device framework provides APIs to allocate and initialize
> sessions -for crypto devices, where sessions are mempool objects.
> +The Crypto device framework provides APIs to create session mempool and
> +allocate and initialize sessions for crypto devices, where sessions are
> mempool objects.
>  It is the application's responsibility to create and manage the session
> mempools.
>  This approach allows for different scenarios such as having a single session
> mempool for all crypto devices (where the mempool object size is big
> enough to hold the private session of any crypto device), as well as having -
> multiple session mempools of different sizes for better memory usage.
> +multiple session mempools of different sizes for better memory usage.
> +However, the application is required to use
> +``rte_cryptodev_sym_session_pool_create()``
> +to create symmetric session mempool.
> 
>  An application can use ``rte_cryptodev_sym_get_private_session_size()`` to
> get the private session size of given crypto device. This function would allow
> @@ -623,7 +625,8 @@ using one of the crypto PMDs available in DPDK.
>      #define IV_OFFSET            (sizeof(struct rte_crypto_op) + \
>                                   sizeof(struct rte_crypto_sym_op))
> 
> -    struct rte_mempool *mbuf_pool, *crypto_op_pool, *session_pool;
> +    struct rte_mempool *mbuf_pool, *crypto_op_pool;
> +    struct *session_pool, *session_priv_pool;

Missing rte_mempool here.

>      unsigned int session_size;
>      int ret;
> 

...

> +++ b/doc/guides/rel_notes/release_19_02.rst
> @@ -167,6 +167,11 @@ API Changes
>  * cryptodev: as shown in the the 18.11 deprecation notice, the last
> parameter
>    of ``rte_cryptodev_queue_pair_setup()`` is removed.
> 
> +* cryptodev: a new function ``rte_cryptodev_sym_session_pool_create()``
> +is
> +  introduced. This function is used to create symmetric session mempool
> +safely
> +  and all crypto applications are required to use this function to
> +create
> +  symmetric session mempool from now on.

Even though this is new API, it is replacing a necessary call to
rte_mempool_create() which was used to create the session pools.
I think we should also include that it needs to be used instead of mempool_create (being more explicit).

> +
> 
>  ABI Changes
>  -----------

...

> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> @@ -88,6 +88,13 @@ DPDK_18.05 {
> 
>  } DPDK_17.11;
> 
> +DPDK_19.02 {
> +	global:
> +
> +	rte_cryptodev_sym_session_pool_create;

Just saw that check-symbol-change is complaining about this new API being stable.
Apologies for having you changing from experimental to stable, when the API needs to be experimental.

> +
> +} DPDK_18.05;
> +
>  EXPERIMENTAL {
>  	global:
> 
> --
> 2.13.6

Apart from this, patch looks ok to me:

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>