diff mbox

[RFC] Adding Multi variant for Async crypto operation.

Message ID 1464880064-32591-1-git-send-email-nikhil.agarwal@linaro.org
State New
Headers show

Commit Message

Nikhil Agarwal June 2, 2016, 3:07 p.m. UTC
Signed-off-by: Nikhil Agarwal <nikhil.agarwal@linaro.org>
---
 include/odp/api/spec/crypto.h | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Comments

Bill Fischofer June 3, 2016, 12:23 a.m. UTC | #1
This should be tagged RFC API-NEXT since it proposes an API change



On Thu, Jun 2, 2016 at 10:07 AM, Nikhil Agarwal <nikhil.agarwal@linaro.org>
wrote:

> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@linaro.org>
> ---
>  include/odp/api/spec/crypto.h | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
> index d8123e9..1c2b384 100644
> --- a/include/odp/api/spec/crypto.h
> +++ b/include/odp/api/spec/crypto.h
> @@ -214,7 +214,6 @@ typedef struct odp_crypto_session_params {
>   * @todo Clarify who zero's ICV and how this relates to
> "hash_result_offset"
>

Not part of this patch, but we really need to remove the @todo from an API
file. Doesn't look good here. Is this something that can be done as a doc
patch for Monarch?


>   */
>  typedef struct odp_crypto_op_params {
> -       odp_crypto_session_t session;   /**< Session handle from creation
> */
>         void *ctx;                      /**< User context */
>         odp_packet_t pkt;               /**< Input packet buffer */
>         odp_packet_t out_pkt;           /**< Output packet buffer */
> @@ -406,6 +405,7 @@ void odp_crypto_compl_free(odp_crypto_compl_t
> completion_event);
>   * If "posted" returns TRUE the result will be delivered via the
> completion
>   * queue specified when the session was created.
>   *
> + * @param session           Session handle
>   * @param params            Operation parameters
>   * @param posted            Pointer to return posted, TRUE for async
> operation
>   * @param result            Results of operation (when posted returns
> FALSE)
> @@ -413,10 +413,31 @@ void odp_crypto_compl_free(odp_crypto_compl_t
> completion_event);
>   * @retval 0 on success
>   * @retval <0 on failure
>   */
> -int odp_crypto_operation(odp_crypto_op_params_t *params,
> +int odp_crypto_operation(odp_crypto_session_t session,
> +                        odp_crypto_op_params_t *params,
>                          odp_bool_t *posted,
>                          odp_crypto_op_result_t *result);
>
> +
> +/**
> + * Crypto packet burst operation
> + *
> + * Performs the cryptographic operations specified during session creation
> + * on the list of packets. Burst operation can only be performed
> asynchronously,
> + * and the result will be delivered via the completion queue specified
> when the
> + * session was created.
> + *
> + * @param session           Session handle
> + * @param params            Operation parameters
> + * @param num               Number of operations requested
> + *
> + * @return Number of operations actually enqueued (0 ... num)
> + * @retval <0 on failure
> + */
> +int odp_crypto_operation_multi(odp_crypto_session_t session,
>

Why is is necessary/desirable to have all operations share the same
session? If the application wanted them to share the same session wouldn't
that also be accomplished by simply having the same session in each
crypto_op_param_t (using old definition of this struct)?


> +                              odp_crypto_op_params_t *params[],
> +                              int num);
> +
>  /**
>   * Crypto per packet operation query result from completion event
>   *
> --
> 2.8.2
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Balasubramanian Manoharan June 3, 2016, 9:01 a.m. UTC | #2
On 2 June 2016 at 20:37, Nikhil Agarwal <nikhil.agarwal@linaro.org> wrote:
> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@linaro.org>
> ---
>  include/odp/api/spec/crypto.h | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
> index d8123e9..1c2b384 100644
> --- a/include/odp/api/spec/crypto.h
> +++ b/include/odp/api/spec/crypto.h
> @@ -214,7 +214,6 @@ typedef struct odp_crypto_session_params {
>   * @todo Clarify who zero's ICV and how this relates to "hash_result_offset"
>   */
>  typedef struct odp_crypto_op_params {
> -       odp_crypto_session_t session;   /**< Session handle from creation */
>         void *ctx;                      /**< User context */
>         odp_packet_t pkt;               /**< Input packet buffer */
>         odp_packet_t out_pkt;           /**< Output packet buffer */
> @@ -406,6 +405,7 @@ void odp_crypto_compl_free(odp_crypto_compl_t completion_event);
>   * If "posted" returns TRUE the result will be delivered via the completion
>   * queue specified when the session was created.
>   *
> + * @param session           Session handle
>   * @param params            Operation parameters
>   * @param posted            Pointer to return posted, TRUE for async operation
>   * @param result            Results of operation (when posted returns FALSE)
> @@ -413,10 +413,31 @@ void odp_crypto_compl_free(odp_crypto_compl_t completion_event);
>   * @retval 0 on success
>   * @retval <0 on failure
>   */
> -int odp_crypto_operation(odp_crypto_op_params_t *params,
> +int odp_crypto_operation(odp_crypto_session_t session,
> +                        odp_crypto_op_params_t *params,
>                          odp_bool_t *posted,
>                          odp_crypto_op_result_t *result);

Since odp_crypto_session_t is already available inside
crypto_op_params_t,  What is the advantage of sending the session
separately in this API?

Regards,
Bala
>
> +
> +/**
> + * Crypto packet burst operation
> + *
> + * Performs the cryptographic operations specified during session creation
> + * on the list of packets. Burst operation can only be performed asynchronously,
> + * and the result will be delivered via the completion queue specified when the
> + * session was created.
> + *
> + * @param session           Session handle
> + * @param params            Operation parameters
> + * @param num               Number of operations requested
> + *
> + * @return Number of operations actually enqueued (0 ... num)
> + * @retval <0 on failure
> + */
> +int odp_crypto_operation_multi(odp_crypto_session_t session,
> +                              odp_crypto_op_params_t *params[],
> +                              int num);
> +
>  /**
>   * Crypto per packet operation query result from completion event
>   *
> --
> 2.8.2
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
Bill Fischofer June 3, 2016, 11:45 a.m. UTC | #3
I've added this to the agenda for Monday's ARCH call.

On Fri, Jun 3, 2016 at 4:59 AM, Nikhil Agarwal <nikhil.agarwal@nxp.com>
wrote:

>
>
> -----Original Message-----
> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bala
> Manoharan
> Sent: Friday, June 03, 2016 2:32 PM
> To: Nikhil Agarwal <nikhil.agarwal@linaro.org>
> Cc: LNG ODP Mailman List <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [RFC] Adding Multi variant for Async crypto
> operation.
>
> On 2 June 2016 at 20:37, Nikhil Agarwal <nikhil.agarwal@linaro.org> wrote:
> > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@linaro.org>
> > ---
> >  include/odp/api/spec/crypto.h | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/odp/api/spec/crypto.h
> > b/include/odp/api/spec/crypto.h index d8123e9..1c2b384 100644
> > --- a/include/odp/api/spec/crypto.h
> > +++ b/include/odp/api/spec/crypto.h
> > @@ -214,7 +214,6 @@ typedef struct odp_crypto_session_params {
> >   * @todo Clarify who zero's ICV and how this relates to
> "hash_result_offset"
> >   */
> >  typedef struct odp_crypto_op_params {
> > -       odp_crypto_session_t session;   /**< Session handle from
> creation */
> >         void *ctx;                      /**< User context */
> >         odp_packet_t pkt;               /**< Input packet buffer */
> >         odp_packet_t out_pkt;           /**< Output packet buffer */
> > @@ -406,6 +405,7 @@ void odp_crypto_compl_free(odp_crypto_compl_t
> completion_event);
> >   * If "posted" returns TRUE the result will be delivered via the
> completion
> >   * queue specified when the session was created.
> >   *
> > + * @param session           Session handle
> >   * @param params            Operation parameters
> >   * @param posted            Pointer to return posted, TRUE for async
> operation
> >   * @param result            Results of operation (when posted returns
> FALSE)
> > @@ -413,10 +413,31 @@ void odp_crypto_compl_free(odp_crypto_compl_t
> completion_event);
> >   * @retval 0 on success
> >   * @retval <0 on failure
> >   */
> > -int odp_crypto_operation(odp_crypto_op_params_t *params,
> > +int odp_crypto_operation(odp_crypto_session_t session,
> > +                        odp_crypto_op_params_t *params,
> >                          odp_bool_t *posted,
> >                          odp_crypto_op_result_t *result);
>
> Since odp_crypto_session_t is already available inside
> crypto_op_params_t,  What is the advantage of sending the session
> separately in this API?
> [Nikhil]RFC proposes to remove session from crypto_op_params_t to have
> synchronization between single and multi variant of the API. Since multi
> should have all the packet belonging to same crypto sessions and rest all
> params may differ for all packets.
>
> Regards,
> Bala
> >
> > +
> > +/**
> > + * Crypto packet burst operation
> > + *
> > + * Performs the cryptographic operations specified during session
> > +creation
> > + * on the list of packets. Burst operation can only be performed
> > +asynchronously,
> > + * and the result will be delivered via the completion queue
> > +specified when the
> > + * session was created.
> > + *
> > + * @param session           Session handle
> > + * @param params            Operation parameters
> > + * @param num               Number of operations requested
> > + *
> > + * @return Number of operations actually enqueued (0 ... num)
> > + * @retval <0 on failure
> > + */
> > +int odp_crypto_operation_multi(odp_crypto_session_t session,
> > +                              odp_crypto_op_params_t *params[],
> > +                              int num);
> > +
> >  /**
> >   * Crypto per packet operation query result from completion event
> >   *
> > --
> > 2.8.2
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > https://lists.linaro.org/mailman/listinfo/lng-odp
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Balasubramanian Manoharan June 3, 2016, 12:10 p.m. UTC | #4
On 3 June 2016 at 15:29, Nikhil Agarwal <nikhil.agarwal@nxp.com> wrote:
>
>
> -----Original Message-----
> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bala Manoharan
> Sent: Friday, June 03, 2016 2:32 PM
> To: Nikhil Agarwal <nikhil.agarwal@linaro.org>
> Cc: LNG ODP Mailman List <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [RFC] Adding Multi variant for Async crypto operation.
>
> On 2 June 2016 at 20:37, Nikhil Agarwal <nikhil.agarwal@linaro.org> wrote:
>> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@linaro.org>
>> ---
>>  include/odp/api/spec/crypto.h | 25 +++++++++++++++++++++++--
>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/odp/api/spec/crypto.h
>> b/include/odp/api/spec/crypto.h index d8123e9..1c2b384 100644
>> --- a/include/odp/api/spec/crypto.h
>> +++ b/include/odp/api/spec/crypto.h
>> @@ -214,7 +214,6 @@ typedef struct odp_crypto_session_params {
>>   * @todo Clarify who zero's ICV and how this relates to "hash_result_offset"
>>   */
>>  typedef struct odp_crypto_op_params {
>> -       odp_crypto_session_t session;   /**< Session handle from creation */
>>         void *ctx;                      /**< User context */
>>         odp_packet_t pkt;               /**< Input packet buffer */
>>         odp_packet_t out_pkt;           /**< Output packet buffer */
>> @@ -406,6 +405,7 @@ void odp_crypto_compl_free(odp_crypto_compl_t completion_event);
>>   * If "posted" returns TRUE the result will be delivered via the completion
>>   * queue specified when the session was created.
>>   *
>> + * @param session           Session handle
>>   * @param params            Operation parameters
>>   * @param posted            Pointer to return posted, TRUE for async operation
>>   * @param result            Results of operation (when posted returns FALSE)
>> @@ -413,10 +413,31 @@ void odp_crypto_compl_free(odp_crypto_compl_t completion_event);
>>   * @retval 0 on success
>>   * @retval <0 on failure
>>   */
>> -int odp_crypto_operation(odp_crypto_op_params_t *params,
>> +int odp_crypto_operation(odp_crypto_session_t session,
>> +                        odp_crypto_op_params_t *params,
>>                          odp_bool_t *posted,
>>                          odp_crypto_op_result_t *result);
>
> Since odp_crypto_session_t is already available inside crypto_op_params_t,  What is the advantage of sending the session separately in this API?
> [Nikhil]RFC proposes to remove session from crypto_op_params_t to have synchronization between single and multi variant of the API. Since multi should have all the packet belonging to same crypto sessions and rest all params may differ for all packets.

The multi API proposal can not be used for synchronous crypto
operation. I would prefer not to remove session from
crypto_op_params_t struct.
Also the underlying requirement during definition of crypto API was to
have a single odp_cryto_operation function for both sync and async
crypto operations so that the application code remains same for
platforms supporting either synchronous or asynchronous crypto
operations.

Regards,
Bala
>
> Regards,
> Bala
>>
>> +
>> +/**
>> + * Crypto packet burst operation
>> + *
>> + * Performs the cryptographic operations specified during session
>> +creation
>> + * on the list of packets. Burst operation can only be performed
>> +asynchronously,
>> + * and the result will be delivered via the completion queue
>> +specified when the
>> + * session was created.
>> + *
>> + * @param session           Session handle
>> + * @param params            Operation parameters
>> + * @param num               Number of operations requested
>> + *
>> + * @return Number of operations actually enqueued (0 ... num)
>> + * @retval <0 on failure
>> + */
>> +int odp_crypto_operation_multi(odp_crypto_session_t session,
>> +                              odp_crypto_op_params_t *params[],
>> +                              int num);
>> +
>>  /**
>>   * Crypto per packet operation query result from completion event
>>   *
>> --
>> 2.8.2
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/lng-odp
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
index d8123e9..1c2b384 100644
--- a/include/odp/api/spec/crypto.h
+++ b/include/odp/api/spec/crypto.h
@@ -214,7 +214,6 @@  typedef struct odp_crypto_session_params {
  * @todo Clarify who zero's ICV and how this relates to "hash_result_offset"
  */
 typedef struct odp_crypto_op_params {
-	odp_crypto_session_t session;   /**< Session handle from creation */
 	void *ctx;                      /**< User context */
 	odp_packet_t pkt;               /**< Input packet buffer */
 	odp_packet_t out_pkt;           /**< Output packet buffer */
@@ -406,6 +405,7 @@  void odp_crypto_compl_free(odp_crypto_compl_t completion_event);
  * If "posted" returns TRUE the result will be delivered via the completion
  * queue specified when the session was created.
  *
+ * @param session           Session handle
  * @param params            Operation parameters
  * @param posted            Pointer to return posted, TRUE for async operation
  * @param result            Results of operation (when posted returns FALSE)
@@ -413,10 +413,31 @@  void odp_crypto_compl_free(odp_crypto_compl_t completion_event);
  * @retval 0 on success
  * @retval <0 on failure
  */
-int odp_crypto_operation(odp_crypto_op_params_t *params,
+int odp_crypto_operation(odp_crypto_session_t session,
+			 odp_crypto_op_params_t *params,
 			 odp_bool_t *posted,
 			 odp_crypto_op_result_t *result);
 
+
+/**
+ * Crypto packet burst operation
+ *
+ * Performs the cryptographic operations specified during session creation
+ * on the list of packets. Burst operation can only be performed asynchronously,
+ * and the result will be delivered via the completion queue specified when the
+ * session was created.
+ *
+ * @param session           Session handle
+ * @param params            Operation parameters
+ * @param num               Number of operations requested
+ *
+ * @return Number of operations actually enqueued (0 ... num)
+ * @retval <0 on failure
+ */
+int odp_crypto_operation_multi(odp_crypto_session_t session,
+			       odp_crypto_op_params_t *params[],
+			       int num);
+
 /**
  * Crypto per packet operation query result from completion event
  *