On 12/11/2014 04:29 PM, Robbie King (robking) wrote:
> Instead of replying to all the other emails I'm just going to hit this on. :-)
>
> I agree with Taras that we can do away with the two API calls by making "ctx"
> a parameter value for the operation (making the completion event an "output
> only").
>
> My changes to make the completion event opaque (allowing the implementation
> freedom to avoid using buffers) was to address Bala's concern. Taras and
> Bala can debate that one, but in general I would like to remove the knowledge
> from the application that a completion event is a buffer. The implementation
> should be free to do whatever it wishes in the synchronous case while
> allowing the
> application to be the same for both models.
>
> I will modify my RFC to:
>
> 1) make "ctx" a parameter to odp_crypto_operation
> 2) eliminate the two function calls
> 3) make "completion event" an output of odp_crypto_operation
So we have an agreement that completion event can to be "output only".
Let's make this change first without introducing a new completion event
abstractions. This will be beneficial for both sync and async modes.
Then we can optimize further.
>
> To Bala's comment about the various "get" routines for accessing
> the completion event (ctx, pkt, and return codes), I would like to think
> these could be inline's in a typical implementation. What I have in
> linux-generic is inefficient due to mixing both approaches for a
> a completion event at run time. A real implementation wouldn't need
> that overhead.
>
> Making one routine with a bunch of outputs means if you ever want to
> add another output, all existing code is broken when you update the API.
>
> As an alternative, we could have one data structure that is used
> to return everything (if someone adds a field it is still backwards
> compatible):
>
> typedef struct {
> odp_crypto_compl_status_t auth;
> odp_crypto_compl_status_t cipher;
> odp_packet_t pkt;
> void *ctx;
> } odp_crypto_compl_event_out_t;
>
> void
> odp_crypto_get_operation_completion(odp_crypto_compl_event_t completion_event,
> odp_crypto_compl_event_out_t *out);
After step one ("output only" completion) we still have a few
inefficient places:
1. Need to deal with a completion event in synchronous case while it is
really needed only for async case. Why can't we just return results
directly?
int
odp_crypto_operation(odp_crypto_op_params_t *params,
odp_crypto_compl_event_out_t* sync_result);
2. API is not optimized for a fast path. Detailed error status need to
be checked before getting an output packet. IMO it would be better to
return ODP_PACKET_INVALID from odp_crypto_get_operation_compl_packet()
in case of any error. If application is interested in cause of error
it can then request a detailed errors status.
>
> -----Original Message-----
> From: Taras Kondratiuk [mailto:taras.kondrat...@linaro.org]
> Sent: Thursday, December 11, 2014 8:27 AM
> To: Bala; Robbie King (robking); lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [RFC v2 1/3] Add completion event updates to
> odp_crypto.h
>
> On 12/11/2014 01:00 PM, Bala wrote:
>>
>> On Thursday 11 December 2014 05:30 AM, Taras Kondratiuk wrote:
>>> On 12/10/2014 05:12 PM, Robbie King wrote:
>>>> Signed-off-by: Robbie King<robk...@cisco.com>
>>>> ---
>>>> platform/linux-generic/include/api/odp_crypto.h | 77
>>>> ++++++++++++++++++++-----
>>>> 1 file changed, 63 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/platform/linux-generic/include/api/odp_crypto.h
>>>> b/platform/linux-generic/include/api/odp_crypto.h
>>>> index 337e7cf..7780a09 100644
>>>> --- a/platform/linux-generic/include/api/odp_crypto.h
>>>> +++ b/platform/linux-generic/include/api/odp_crypto.h
>>>> @@ -210,6 +210,16 @@ typedef struct odp_crypto_compl_status {
>>>> enum crypto_hw_err hw_err; /**< Hardware specific return code */
>>>> } odp_crypto_compl_status_t;
>>>>
>>>> +/**
>>>> + * Cryto API completion event
>>>> + */
>>>> +typedef struct odp_crypto_compl_event {
>>>> + bool is_buffer;
>>>> + union {
>>>> + void *ptr; /**< Sync, single request
>>>> outstanding */
>>>> + odp_buffer_t buffer; /**< Async and multi sync request
>>>> */
>>>> + };
>>>> +} odp_crypto_compl_event_t;
>>>>
>>>> /**
>>>> * Crypto session creation (synchronous)
>>>> @@ -225,6 +235,55 @@ odp_crypto_session_create(odp_crypto_session_params_t
>>>> *params,
>>>> odp_crypto_session_t *session,
>>>> enum odp_crypto_ses_create_err *status);
>>>>
>>>> +/**
>>>> + * Obtain crypto completion event for upcoming request
>>>> + *
>>>> + * Retrieve an appropriate completion event based on the request we are
>>>> + * getting ready to issue. Getting the event handle ahead of time allows
>>>> + * us to perform any extra initialization (such as set user context).
>>> Actually user context is an attribute of a crypto operation. Completion
>>> event is used only as a transport to be able to obtain the context
>>> after operation is completed. It would be more logical to add operation
>>> user context as an additional field in odp_crypto_op_params_t.
>>> Implementation will store it in a proper place if needed.
>>> So two APIs can be dropped without sacrificing any functionality:
>>> - odp_crypto_get_compl_event()
>>> - odp_crypto_set_operation_compl_ctx()
>> compl_event is needed in odp_crypto_operation() in sync crypto mode as
>> otherwise there is no way for the implementation
>> to return the compl_event back to the application as the compl_event
>> does not pass through compl_queue in sync mode.
>
> Completion event can be an output of odp_crypto_operation() if it
> happened to be synonymous. It seems you haven't looked into my RFC :)
>
--
Taras Kondratiuk
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/lng-odp
