[dpdk-dev,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().

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(-)
  

> -----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
  

On 1/25/2018 1:16 AM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----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
> 

My thoughts are also inline with Pablo.

-Akhil
  

Hi Pablo & Declan,

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Thursday, January 25, 2018 1:17 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; akhil.goyal@nxp.com;
> Jerin.JacobKollanukkaran@cavium.com
> Cc: dev@dpdk.org; Vangati, Narender <narender.vangati@intel.com>; Rao,
> Nikhil <nikhil.rao@intel.com>
> Subject: RE: [RFC v2, 1/2] cryptodev: add support to set session private data
> 
> 
> 
> > -----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?
Yes. This is private date is meant for sessionless case.
> From your title, it looks like it is for the session private data, but then, this
> shouldn't be here.
Agree.
> 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.
Sure. I will make a separate patch for this changes. Add some more information to make it clear.
> 
> 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 am with option 2 with slight changes as below:
rte_cryptodev_sym_session_create() will have a flag as below
indicating private data exits or not.
{ 
- memset(sess, 0, (sizeof(void *) * nb_drivers));
+memset(sess, 0, (sizeof(void *) * nb_drivers ) + sizeof(private_data_flag));
}
and in
rte_cryptodev_get_header_session_size(void)
{
  /*
   * Header contains pointers to the private data
   * of all registered drivers
   */
  -return (sizeof(void *) * nb_drivers);
  +return ((sizeof(void *) * nb_drivers) + sizeof(private_data_flag));
}
With this, a flag indicating private data exists or not will always have valid value.

> 
> 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
  

Hi Abhinandan,

> -----Original Message-----
> From: Gujjar, Abhinandan S
> Sent: Thursday, January 25, 2018 3:38 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty,
> Declan <declan.doherty@intel.com>; akhil.goyal@nxp.com;
> Jerin.JacobKollanukkaran@cavium.com
> Cc: dev@dpdk.org; Vangati, Narender <narender.vangati@intel.com>; Rao,
> Nikhil <nikhil.rao@intel.com>
> Subject: RE: [RFC v2, 1/2] cryptodev: add support to set session private
> data
> 
> Hi Pablo & Declan,
> 
> > -----Original Message-----
> > From: De Lara Guarch, Pablo
> > Sent: Thursday, January 25, 2018 1:17 AM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Doherty,
> > Declan <declan.doherty@intel.com>; akhil.goyal@nxp.com;
> > Jerin.JacobKollanukkaran@cavium.com
> > Cc: dev@dpdk.org; Vangati, Narender <narender.vangati@intel.com>;
> Rao,
> > Nikhil <nikhil.rao@intel.com>
> > Subject: RE: [RFC v2, 1/2] cryptodev: add support to set session
> > private data
> >
> >
> >
> > > -----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?
> Yes. This is private date is meant for sessionless case.
> > From your title, it looks like it is for the session private data, but
> > then, this shouldn't be here.
> Agree.
> > 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.
> Sure. I will make a separate patch for this changes. Add some more
> information to make it clear.
> >
> > 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 am with option 2 with slight changes as below:
> rte_cryptodev_sym_session_create() will have a flag as below indicating
> private data exits or not.
> {
> - memset(sess, 0, (sizeof(void *) * nb_drivers));
> +memset(sess, 0, (sizeof(void *) * nb_drivers ) +
> +sizeof(private_data_flag));
> }
> and in
> rte_cryptodev_get_header_session_size(void)
> {
>   /*
>    * Header contains pointers to the private data
>    * of all registered drivers
>    */
>   -return (sizeof(void *) * nb_drivers);
>   +return ((sizeof(void *) * nb_drivers) + sizeof(private_data_flag)); } With
> this, a flag indicating private data exists or not will always have valid value.

Sure, this should work. Go ahead and send a v3 with this change, separating the changes
made in the session from the changes made in the crypto operation (so you will have 3 patches in total).

Pablo

> 
> >
> > 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