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

[Akhil Goyal <gakhil@marvell.com>]

Hi Ciara,
Few minor comments.

> -	/* setup asym session pool */
> -	unsigned int session_size = RTE_MAX(
> -		rte_cryptodev_asym_get_private_session_size(dev_id),
> -		rte_cryptodev_asym_get_header_session_size());
> -	/*
> -	 * Create mempool with TEST_NUM_SESSIONS * 2,
> -	 * to include the session headers
> -	 */
> -	ts_params->session_mpool = rte_mempool_create(
> -				"test_asym_sess_mp",
> -				TEST_NUM_SESSIONS * 2,
> -				session_size,
> -				0, 0, NULL, NULL, NULL,
> -				NULL, SOCKET_ID_ANY,
> -				0);
> +	ts_params->session_mpool =
> rte_cryptodev_asym_session_pool_create(
> +			"test_asym_sess_mp", TEST_NUM_SESSIONS * 2, 0,
> +			SOCKET_ID_ANY);

I believe TEST_NUM_SESSIONS * 2 is no more required.
It can be simply TEST_NUM_SESSIONS.

> 
>  	TEST_ASSERT_NOT_NULL(ts_params->session_mpool,
>  			"session mempool allocation failed");
> @@ -1107,14 +1052,6 @@ test_dh_gen_shared_sec(struct

<snip> 

> @@ -628,16 +626,9 @@ set_sym_session_private_data(struct
> rte_cryptodev_sym_session *sess,
>  }
> 
>  static inline void *
> -get_asym_session_private_data(const struct rte_cryptodev_asym_session
> *sess,
> -		uint8_t driver_id) {
> -	return sess->sess_private_data[driver_id];
> -}
> -
> -static inline void
> -set_asym_session_private_data(struct rte_cryptodev_asym_session *sess,
> -		uint8_t driver_id, void *private_data)
> +get_asym_session_private_data(struct rte_cryptodev_asym_session *sess)
>  {
> -	sess->sess_private_data[driver_id] = private_data;
> +	return sess->sess_private_data;
>  }

I think we can safely remove this get API as well and use sess->sess_private_data
Directly.
Since we are removing set API, get can also be removed as session struct is visible to PMD.

>  /**
>   * Create symmetric crypto session header (generic with no private data)
>   *
> @@ -971,15 +998,19 @@ rte_cryptodev_sym_session_create(struct
> rte_mempool *mempool);
>  /**
>   * Create asymmetric crypto session header (generic with no private data)

This line need an update.


>   *
> - * @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
> + * @param   mp       mempool to allocate asymmetric session
> + *                   objects from
>   * @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(uint8_t dev_id,
> +		struct rte_crypto_asym_xform *xforms, struct rte_mempool
> *mp);

<snip>

> diff --git a/lib/cryptodev/rte_cryptodev_trace.h
> b/lib/cryptodev/rte_cryptodev_trace.h
> index d1f4f069a3..f4e1c870df 100644
> --- a/lib/cryptodev/rte_cryptodev_trace.h
> +++ b/lib/cryptodev/rte_cryptodev_trace.h
> @@ -83,10 +83,22 @@ RTE_TRACE_POINT(
>  	rte_trace_point_emit_u16(sess->user_data_sz);
>  )
> 
> +RTE_TRACE_POINT(
> +	rte_cryptodev_trace_asym_session_pool_create,
> +	RTE_TRACE_POINT_ARGS(const char *name, uint32_t nb_elts,
> +		uint32_t cache_size, void *mempool),
> +	rte_trace_point_emit_string(name);
> +	rte_trace_point_emit_u32(nb_elts);
> +	rte_trace_point_emit_u32(cache_size);
> +	rte_trace_point_emit_ptr(mempool);
> +)
> +
>  RTE_TRACE_POINT(
>  	rte_cryptodev_trace_asym_session_create,
> -	RTE_TRACE_POINT_ARGS(void *mempool,
> -		struct rte_cryptodev_asym_session *sess),
> +	RTE_TRACE_POINT_ARGS(uint8_t dev_id, void *xforms,
> +		void *mempool, struct rte_cryptodev_asym_session *sess),

Sess is not an argument to sess_create API yet. This would be updated in your later patch.

> +	rte_trace_point_emit_u8(dev_id);
> +	rte_trace_point_emit_ptr(xforms);
>  	rte_trace_point_emit_ptr(mempool);
>  	rte_trace_point_emit_ptr(sess);
>  )