RE: [EXT] [PATCH v5 2/5] crypto: use single buffer for asymmetric session

[Anoob Joseph <anoobj@marvell.com>]

Hi Ciara,

Minor nit inline. With that

Series Acked-by: Anoob Joseph <anoobj@marvell.com>

Thanks,
Anoob

> -----Original Message-----
> From: Ciara Power <ciara.power@intel.com>
> Sent: Thursday, February 10, 2022 7:32 PM
> To: dev@dpdk.org
> Cc: roy.fan.zhang@intel.com; Akhil Goyal <gakhil@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; mdr@ashroe.eu; Ciara Power
> <ciara.power@intel.com>; Declan Doherty <declan.doherty@intel.com>; Ankur
> Dwivedi <adwivedi@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>
> Subject: [EXT] [PATCH v5 2/5] 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>
> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
> 
> ---
> v5:
>   - Removed get API for session private data, can be accessed directly.
>   - Modified test application to create a session mempool for
>     TEST_NUM_SESSIONS rather than TEST_NUM_SESSIONS * 2.
>   - Reworded create session function description.
>   - Removed sess parameter from create session trace,
>     to be added in a later patch.
> v4:
>   - Merged asym crypto session clear and free functions.
>   - Reordered some function parameters.
>   - Updated trace function for asym crypto session create.
>   - Fixed cnxk clear, the PMD no longer needs to put private data
>     back into a mempool.
>   - Renamed struct field for max private session size.
>   - Replaced __extension__ with RTE_STD_C11.
>   - Moved some parameter validity checks to before functional code.
>   - Reworded release note.
>   - Removed mempool parameter from session configure function.
>   - Removed docs code additions, these are included due to patch 1
>     changing sample doc to use literal includes.
> v3:
>   - Corrected formatting of struct comments.
>   - Increased size of max_priv_session_sz to uint16_t.
>   - Removed trace for asym session init function that was
>     previously removed.
>   - Added documentation.
> v2:
>   - Renamed function typedef from "free" to "clear" as session private
>     data isn't being freed in that function.
>   - Moved user data API to separate patch.
>   - Minor fixes to comments, formatting, return values.
> ---
>  app/test-crypto-perf/cperf_ops.c             |  14 +-
>  app/test-crypto-perf/cperf_test_throughput.c |   8 +-
>  app/test/test_cryptodev_asym.c               | 272 +++++--------------
>  doc/guides/prog_guide/cryptodev_lib.rst      |  21 +-
>  doc/guides/rel_notes/release_22_03.rst       |   7 +
>  drivers/crypto/cnxk/cn10k_cryptodev_ops.c    |   8 +-
>  drivers/crypto/cnxk/cn9k_cryptodev_ops.c     |   8 +-
>  drivers/crypto/cnxk/cnxk_cryptodev_ops.c     |  22 +-
>  drivers/crypto/cnxk/cnxk_cryptodev_ops.h     |   3 +-
>  drivers/crypto/octeontx/otx_cryptodev_ops.c  |  32 +--
>  drivers/crypto/openssl/rte_openssl_pmd.c     |   4 +-
>  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  24 +-
>  drivers/crypto/qat/qat_asym.c                |  54 +---
>  drivers/crypto/qat/qat_asym.h                |   5 +-
>  lib/cryptodev/cryptodev_pmd.h                |  23 +-
>  lib/cryptodev/cryptodev_trace_points.c       |   9 +-
>  lib/cryptodev/rte_cryptodev.c                | 213 ++++++++-------
>  lib/cryptodev/rte_cryptodev.h                |  97 ++++---
>  lib/cryptodev/rte_cryptodev_trace.h          |  38 ++-
>  lib/cryptodev/version.map                    |   7 +-
>  20 files changed, 315 insertions(+), 554 deletions(-)
> 

[snip]

> diff --git a/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
> b/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
> index a5fb68da02..72f5f1a6fe 100644
> --- a/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
> +++ b/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
> @@ -658,10 +658,9 @@ void
>  cnxk_ae_session_clear(struct rte_cryptodev *dev,
>  		      struct rte_cryptodev_asym_session *sess)
>  {
> -	struct rte_mempool *sess_mp;
>  	struct cnxk_ae_sess *priv;
> 
> -	priv = get_asym_session_private_data(sess, dev->driver_id);
> +	priv = (struct cnxk_ae_sess *) sess->sess_private_data;
>  	if (priv == NULL)
>  		return;
> 
> @@ -670,40 +669,29 @@ cnxk_ae_session_clear(struct rte_cryptodev *dev,
> 
>  	/* Reset and free object back to pool */
>  	memset(priv, 0, cnxk_ae_session_size_get(dev));
> -	sess_mp = rte_mempool_from_obj(priv);
> -	set_asym_session_private_data(sess, dev->driver_id, NULL);
> -	rte_mempool_put(sess_mp, priv);
>  }
> 
>  int
>  cnxk_ae_session_cfg(struct rte_cryptodev *dev,
>  		    struct rte_crypto_asym_xform *xform,
> -		    struct rte_cryptodev_asym_session *sess,
> -		    struct rte_mempool *pool)
> +		    struct rte_cryptodev_asym_session *sess)
>  {
>  	struct cnxk_cpt_vf *vf = dev->data->dev_private;
>  	struct roc_cpt *roc_cpt = &vf->cpt;
> -	struct cnxk_ae_sess *priv;
> +	struct cnxk_ae_sess *priv =
> +			(struct cnxk_ae_sess *) sess->sess_private_data;

[Anoob] Rest of the code in cnxk follows reverse xmas tree. May be, can you move this up? Or may be just split declaration and assignment and you can retain the existing order?