Re: [dpdk-dev] [RFC v2, 1/2] cryptodev: add support to set session private data

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

> -----Original Message-----
> From: Gujjar, Abhinandan S
> Sent: Tuesday, January 23, 2018 8:54 AM
> To: Doherty, Declan <declan.doherty@intel.com>; akhil.goyal@nxp.com; De
> Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> Jerin.JacobKollanukkaran@cavium.com
> Cc: dev@dpdk.org; Vangati, Narender <narender.vangati@intel.com>;
> Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Rao, Nikhil
> <nikhil.rao@intel.com>
> Subject: [RFC v2, 1/2] cryptodev: add support to set session private data
> 
> Update rte_crypto_op to indicate private data offset.
> 
> The application may want to store private data along with the
> rte_cryptodev that is transparent to the rte_cryptodev layer.
> For e.g., If an eventdev based application is submitting a
> rte_cryptodev_sym_session operation and wants to indicate event
> information required to construct a new event that will be enqueued to
> eventdev after completion of the rte_cryptodev_sym_session operation.
> This patch provides a mechanism for the application to associate this
> information with the rte_cryptodev_sym_session session.
> The application can set the private data using
> rte_cryptodev_sym_session_set_private_data() and retrieve it using
> rte_cryptodev_sym_session_get_private_data().

Hi Abhinandan,

> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> ---
> Notes:
>         V2:
> 	1. Removed enum rte_crypto_op_private_data_type
> 	2. Corrected formatting
> 
>  lib/librte_cryptodev/rte_crypto.h    |  8 ++++++--
>  lib/librte_cryptodev/rte_cryptodev.h | 32
> ++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_cryptodev/rte_crypto.h
> b/lib/librte_cryptodev/rte_crypto.h
> index 95cf861..14c87c8 100644
> --- a/lib/librte_cryptodev/rte_crypto.h
> +++ b/lib/librte_cryptodev/rte_crypto.h
> @@ -84,8 +84,12 @@ struct rte_crypto_op {
>  	 */
>  	uint8_t sess_type;
>  	/**< operation session type */
> -
> -	uint8_t reserved[5];
> +	uint16_t private_data_offset;
> +	/**< Offset to indicate start of private data (if any). The private
> +	 * data may be used by the application to store information which
> +	 * should remain untouched in the library/driver

Is this the offset for the private data after the crypto operation?
>From your title, it looks like it is for the session private data, but then, this shouldn't be here.
If it is for the crypto operation, I suggest you to separate it in another patch.
Also, you should indicate where the offset starts from. For the IV, the offset is counted
from the start of the rte_crypto_op, so I think it should be the same, to keep consistency.

For the session private data, we see two options:

1 - Add a  "valid" private data field in the rte_cryptodev_sym_session structure,
so when it is set, it indicates that the session contains private data
(a single bit would be enough, 1 to indicate there is, and 0 to indicate there is not).
This would go into the beginning of the structure, so this would require an ABI deprecation notice.
This also assumes that the private data starts just after the session header

2 -  Do not add an extra "valid" private data field in rte_cryptodev_sym_session structure,
and add a small header in the private data, which contains the "valid" bit.
Then, when calling sym_session_get_private_data, this bit should be checked.
Note that the object that holds the session structure needs to be big enough to hold this value.
If the object has only space for the sess_private_data array, then the session has no private data.
Therefore, this approach might be less performant, but with no ABI deprecation required.

I would recommend you to send a deprecation notice for option 1, then check the performance of both option,
and if needed, make the change in the structure, in 18.05.

Regards,
Pablo