| Message ID | 1433904647-7151-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> wrote: > odp_crypto_operation() should not change underlying buffer types for > completions as these are purely internal and not needed, and doing > so is confusing and error-prome. > > Yes, Bill, I would prefer your approach: my patch felt like a hack to correct for another (hence the RFC). However, this patch looks incomplete to me: -example/ipsec/odp_ipsec.c is using the buffer type to distinguish between events. -example/timer/odp_timer_test.c also uses it. (I am honestely not really sure why...) -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from event_types.h if it is not meant to be used. ... and thanks for taking the time to look at it. Christophe Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/odp_buffer_internal.h | 9 --------- > platform/linux-generic/odp_buffer.c | 7 ------- > platform/linux-generic/odp_crypto.c | 5 ----- > 3 files changed, 21 deletions(-) > > diff --git a/platform/linux-generic/include/odp_buffer_internal.h > b/platform/linux-generic/include/odp_buffer_internal.h > index e7b7568..2595b2d 100644 > --- a/platform/linux-generic/include/odp_buffer_internal.h > +++ b/platform/linux-generic/include/odp_buffer_internal.h > @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t > size); > */ > int _odp_buffer_type(odp_buffer_t buf); > > -/* > - * Buffer type set > - * > - * @param buf Buffer handle > - * @param type New type value > - * > - */ > - void _odp_buffer_type_set(odp_buffer_t buf, int type); > - > #ifdef __cplusplus > } > #endif > diff --git a/platform/linux-generic/odp_buffer.c > b/platform/linux-generic/odp_buffer.c > index 0803805..19b9dbd 100644 > --- a/platform/linux-generic/odp_buffer.c > +++ b/platform/linux-generic/odp_buffer.c > @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) > return hdr->type; > } > > -void _odp_buffer_type_set(odp_buffer_t buf, int type) > -{ > - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); > - > - hdr->type = type; > -} > - > > int odp_buffer_is_valid(odp_buffer_t buf) > { > diff --git a/platform/linux-generic/odp_crypto.c > b/platform/linux-generic/odp_crypto.c > index b16316c..e103066 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params, > > /* Linux generic will always use packet for completion > event */ > completion_event = odp_packet_to_event(params->out_pkt); > - > _odp_buffer_type_set(odp_buffer_from_event(completion_event), > - ODP_EVENT_CRYPTO_COMPL); > > /* Asynchronous, build result (no HW so no errors) and > send it*/ > op_result = get_op_result_from_event(completion_event); > @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, odp_bool_t > use_entropy ODP_UNUSED) > > odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) > { > - /* This check not mandated by the API specification */ > - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) > - ODP_ABORT("Event not a crypto completion"); > return (odp_crypto_compl_t)ev; > } > > -- > 2.1.0 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
OK, let me take another look at this. I notice odp_crypto.c has this
routine:
/*
* @todo This is a serious hack to allow us to use packet buffer to convey
* crypto operation results by placing them at the very end of the
* packet buffer. The issue should be resolved shortly once the issue
* of packets versus events on completion queues is closed.
*/
static
odp_crypto_generic_op_result_t *get_op_result_from_event(odp_event_t ev)
{
uint8_t *temp;
odp_crypto_generic_op_result_t *result;
odp_buffer_t buf;
/* HACK: Buffer is not packet any more in the API.
* Implementation still works that way. */
buf = odp_buffer_from_event(ev);
temp = odp_buffer_addr(buf);
temp += odp_buffer_size(buf);
temp -= sizeof(*result);
result = (odp_crypto_generic_op_result_t *)(void *)temp;
return result;
}
This sounds like all of this really should be packet metadata. Alex: Any
thoughts on this?
On Wed, Jun 10, 2015 at 3:11 AM, Savolainen, Petri (Nokia - FI/Espoo) <
petri.savolainen@nokia.com> wrote:
>
>
>
>
> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *ext
> Christophe Milard
> *Sent:* Wednesday, June 10, 2015 10:18 AM
> *To:* Bill Fischofer
> *Cc:* LNG ODP Mailman List
> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate buffer
> type hack for completions
>
>
>
>
>
>
>
> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org>
> wrote:
>
> odp_crypto_operation() should not change underlying buffer types for
> completions as these are purely internal and not needed, and doing
> so is confusing and error-prome.
>
>
>
> Yes, Bill, I would prefer your approach: my patch felt like a hack to
> correct for another (hence the RFC).
>
> However, this patch looks incomplete to me:
>
> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish between
> events.
>
> -example/timer/odp_timer_test.c also uses it. (I am honestely not really
> sure why...)
>
> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from
> event_types.h if it is not meant to be used.
>
> ... and thanks for taking the time to look at it.
>
>
>
> Crypto completion event type is certainly meant to be used. If
> implementation misuses the event type, the implementation needs to be
> fixed. When an application receives an event for crypto completion
> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL.
>
>
>
>
>
> Christophe
>
>
>
> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
> ---
> platform/linux-generic/include/odp_buffer_internal.h | 9 ---------
> platform/linux-generic/odp_buffer.c | 7 -------
> platform/linux-generic/odp_crypto.c | 5 -----
> 3 files changed, 21 deletions(-)
>
> diff --git a/platform/linux-generic/include/odp_buffer_internal.h
> b/platform/linux-generic/include/odp_buffer_internal.h
> index e7b7568..2595b2d 100644
> --- a/platform/linux-generic/include/odp_buffer_internal.h
> +++ b/platform/linux-generic/include/odp_buffer_internal.h
> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t
> size);
> */
> int _odp_buffer_type(odp_buffer_t buf);
>
> -/*
> - * Buffer type set
> - *
> - * @param buf Buffer handle
> - * @param type New type value
> - *
> - */
> - void _odp_buffer_type_set(odp_buffer_t buf, int type);
> -
> #ifdef __cplusplus
> }
> #endif
> diff --git a/platform/linux-generic/odp_buffer.c
> b/platform/linux-generic/odp_buffer.c
> index 0803805..19b9dbd 100644
> --- a/platform/linux-generic/odp_buffer.c
> +++ b/platform/linux-generic/odp_buffer.c
> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf)
> return hdr->type;
> }
>
> -void _odp_buffer_type_set(odp_buffer_t buf, int type)
> -{
> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf);
> -
> - hdr->type = type;
> -}
> -
>
> int odp_buffer_is_valid(odp_buffer_t buf)
> {
> diff --git a/platform/linux-generic/odp_crypto.c
> b/platform/linux-generic/odp_crypto.c
> index b16316c..e103066 100644
> --- a/platform/linux-generic/odp_crypto.c
> +++ b/platform/linux-generic/odp_crypto.c
> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params,
>
> /* Linux generic will always use packet for completion
> event */
> completion_event = odp_packet_to_event(params->out_pkt);
> -
> _odp_buffer_type_set(odp_buffer_from_event(completion_event),
> - ODP_EVENT_CRYPTO_COMPL);
>
> /* Asynchronous, build result (no HW so no errors) and
> send it*/
> op_result = get_op_result_from_event(completion_event);
> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, odp_bool_t
> use_entropy ODP_UNUSED)
>
> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev)
> {
> - /* This check not mandated by the API specification */
> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL)
> - ODP_ABORT("Event not a crypto completion");
>
>
>
> This check should be there in the reference implementation to catch bugs.
> Event type must be CRYPTO_COMPL when entering this function.
>
>
>
> -Petri
>
>
>
>
> return (odp_crypto_compl_t)ev;
> }
>
> --
> 2.1.0
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
As Petri writes, the type of a crypto completion event must be visible to the user (who is receiving the event and responsible for figuring out what to do with those events). Different types of events may be received from the same queue and require different action. I think it is useful for an implementation to allow buffer pool and buffer event code to be used for other types of events as well, e.g. crypto completion events and timeout events. The timer patch I did on Taras' request based timeouts on buffers with the timeout-specific data stored in the buffer data so no need for a timeout-specific header. This design simplifies reuse on other platforms where the event (HW buffer) definition is fixed and there might be no room for additional event-type specific headers. On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > > > *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *ext > Christophe Milard > *Sent:* Wednesday, June 10, 2015 10:18 AM > *To:* Bill Fischofer > *Cc:* LNG ODP Mailman List > *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate buffer > type hack for completions > > > > > > > > On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > odp_crypto_operation() should not change underlying buffer types for > completions as these are purely internal and not needed, and doing > so is confusing and error-prome. > > > > Yes, Bill, I would prefer your approach: my patch felt like a hack to > correct for another (hence the RFC). > > However, this patch looks incomplete to me: > > -example/ipsec/odp_ipsec.c is using the buffer type to distinguish between > events. > > -example/timer/odp_timer_test.c also uses it. (I am honestely not really > sure why...) > > -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from > event_types.h if it is not meant to be used. > > ... and thanks for taking the time to look at it. > > > > Crypto completion event type is certainly meant to be used. If > implementation misuses the event type, the implementation needs to be > fixed. When an application receives an event for crypto completion > odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. > > > > > > Christophe > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/odp_buffer_internal.h | 9 --------- > platform/linux-generic/odp_buffer.c | 7 ------- > platform/linux-generic/odp_crypto.c | 5 ----- > 3 files changed, 21 deletions(-) > > diff --git a/platform/linux-generic/include/odp_buffer_internal.h > b/platform/linux-generic/include/odp_buffer_internal.h > index e7b7568..2595b2d 100644 > --- a/platform/linux-generic/include/odp_buffer_internal.h > +++ b/platform/linux-generic/include/odp_buffer_internal.h > @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t > size); > */ > int _odp_buffer_type(odp_buffer_t buf); > > -/* > - * Buffer type set > - * > - * @param buf Buffer handle > - * @param type New type value > - * > - */ > - void _odp_buffer_type_set(odp_buffer_t buf, int type); > - > #ifdef __cplusplus > } > #endif > diff --git a/platform/linux-generic/odp_buffer.c > b/platform/linux-generic/odp_buffer.c > index 0803805..19b9dbd 100644 > --- a/platform/linux-generic/odp_buffer.c > +++ b/platform/linux-generic/odp_buffer.c > @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) > return hdr->type; > } > > -void _odp_buffer_type_set(odp_buffer_t buf, int type) > -{ > - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); > - > - hdr->type = type; > -} > - > > int odp_buffer_is_valid(odp_buffer_t buf) > { > diff --git a/platform/linux-generic/odp_crypto.c > b/platform/linux-generic/odp_crypto.c > index b16316c..e103066 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params, > > /* Linux generic will always use packet for completion > event */ > completion_event = odp_packet_to_event(params->out_pkt); > - > _odp_buffer_type_set(odp_buffer_from_event(completion_event), > - ODP_EVENT_CRYPTO_COMPL); > > /* Asynchronous, build result (no HW so no errors) and > send it*/ > op_result = get_op_result_from_event(completion_event); > @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, odp_bool_t > use_entropy ODP_UNUSED) > > odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) > { > - /* This check not mandated by the API specification */ > - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) > - ODP_ABORT("Event not a crypto completion"); > > > > This check should be there in the reference implementation to catch bugs. > Event type must be CRYPTO_COMPL when entering this function. > > > > -Petri > > > > > return (odp_crypto_compl_t)ev; > } > > -- > 2.1.0 > > _______________________________________________ > 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 > >
Hi, in my opinion crypto completion event is required as the packet output only is not enough for the application to get the operation status in the async operation case. Also for out-of-place mode the input packet may be returned to the application - the fact is that output from crypto is always an aggregate. In the beginning, the completion event was provided by the application but later on it was moved to the implementation. However, re-using the packet buffer to return the crypto operation status is not a hack in my opinion if this is the best way for a given platform (I do the same for my platform but I use the both the headroom and tailroom), is just an implementation detail. On 10 June 2015 at 14:22, Bill Fischofer <bill.fischofer@linaro.org> wrote: > OK, let me take another look at this. I notice odp_crypto.c has this > routine: > > /* > * @todo This is a serious hack to allow us to use packet buffer to convey > * crypto operation results by placing them at the very end of the > * packet buffer. The issue should be resolved shortly once the > issue > * of packets versus events on completion queues is closed. > */ > static > odp_crypto_generic_op_result_t *get_op_result_from_event(odp_event_t ev) > { > uint8_t *temp; > odp_crypto_generic_op_result_t *result; > odp_buffer_t buf; > > /* HACK: Buffer is not packet any more in the API. > * Implementation still works that way. */ > buf = odp_buffer_from_event(ev); > > temp = odp_buffer_addr(buf); > temp += odp_buffer_size(buf); > temp -= sizeof(*result); > result = (odp_crypto_generic_op_result_t *)(void *)temp; > return result; > } > > This sounds like all of this really should be packet metadata. Alex: Any > thoughts on this? > > On Wed, Jun 10, 2015 at 3:11 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > >> >> >> >> >> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *ext >> Christophe Milard >> *Sent:* Wednesday, June 10, 2015 10:18 AM >> *To:* Bill Fischofer >> *Cc:* LNG ODP Mailman List >> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate buffer >> type hack for completions >> >> >> >> >> >> >> >> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >> odp_crypto_operation() should not change underlying buffer types for >> completions as these are purely internal and not needed, and doing >> so is confusing and error-prome. >> >> >> >> Yes, Bill, I would prefer your approach: my patch felt like a hack to >> correct for another (hence the RFC). >> >> However, this patch looks incomplete to me: >> >> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >> between events. >> >> -example/timer/odp_timer_test.c also uses it. (I am honestely not really >> sure why...) >> >> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >> event_types.h if it is not meant to be used. >> >> ... and thanks for taking the time to look at it. >> >> >> >> Crypto completion event type is certainly meant to be used. If >> implementation misuses the event type, the implementation needs to be >> fixed. When an application receives an event for crypto completion >> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >> >> >> >> >> >> Christophe >> >> >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >> platform/linux-generic/odp_buffer.c | 7 ------- >> platform/linux-generic/odp_crypto.c | 5 ----- >> 3 files changed, 21 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >> b/platform/linux-generic/include/odp_buffer_internal.h >> index e7b7568..2595b2d 100644 >> --- a/platform/linux-generic/include/odp_buffer_internal.h >> +++ b/platform/linux-generic/include/odp_buffer_internal.h >> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t >> size); >> */ >> int _odp_buffer_type(odp_buffer_t buf); >> >> -/* >> - * Buffer type set >> - * >> - * @param buf Buffer handle >> - * @param type New type value >> - * >> - */ >> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >> - >> #ifdef __cplusplus >> } >> #endif >> diff --git a/platform/linux-generic/odp_buffer.c >> b/platform/linux-generic/odp_buffer.c >> index 0803805..19b9dbd 100644 >> --- a/platform/linux-generic/odp_buffer.c >> +++ b/platform/linux-generic/odp_buffer.c >> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >> return hdr->type; >> } >> >> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >> -{ >> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >> - >> - hdr->type = type; >> -} >> - >> >> int odp_buffer_is_valid(odp_buffer_t buf) >> { >> diff --git a/platform/linux-generic/odp_crypto.c >> b/platform/linux-generic/odp_crypto.c >> index b16316c..e103066 100644 >> --- a/platform/linux-generic/odp_crypto.c >> +++ b/platform/linux-generic/odp_crypto.c >> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params, >> >> /* Linux generic will always use packet for completion >> event */ >> completion_event = odp_packet_to_event(params->out_pkt); >> - >> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >> - ODP_EVENT_CRYPTO_COMPL); >> >> /* Asynchronous, build result (no HW so no errors) and >> send it*/ >> op_result = get_op_result_from_event(completion_event); >> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, odp_bool_t >> use_entropy ODP_UNUSED) >> >> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >> { >> - /* This check not mandated by the API specification */ >> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >> - ODP_ABORT("Event not a crypto completion"); >> >> >> >> This check should be there in the reference implementation to catch bugs. >> Event type must be CRYPTO_COMPL when entering this function. >> >> >> >> -Petri >> >> >> >> >> return (odp_crypto_compl_t)ev; >> } >> >> -- >> 2.1.0 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> > >
On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > As Petri writes, the type of a crypto completion event must be visible to > the user (who is receiving the event and responsible for figuring out what > to do with those events). Different types of events may be received from > the same queue and require different action. > > I think it is useful for an implementation to allow buffer pool and buffer > event code to be used for other types of events as well, e.g. crypto > completion events and timeout events. The timer patch I did on Taras' > request based timeouts on buffers with the timeout-specific data stored in > the buffer data so no need for a timeout-specific header. This design > simplifies reuse on other platforms where the event (HW buffer) definition > is fixed and there might be no room for additional event-type specific > headers. > Just to clearify. An implementation doing the above would still have specific pools for crypto completion and timeout events. No need to dynamically change the type of an event. > > On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > >> >> >> >> >> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *ext >> Christophe Milard >> *Sent:* Wednesday, June 10, 2015 10:18 AM >> *To:* Bill Fischofer >> *Cc:* LNG ODP Mailman List >> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate buffer >> type hack for completions >> >> >> >> >> >> >> >> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >> odp_crypto_operation() should not change underlying buffer types for >> completions as these are purely internal and not needed, and doing >> so is confusing and error-prome. >> >> >> >> Yes, Bill, I would prefer your approach: my patch felt like a hack to >> correct for another (hence the RFC). >> >> However, this patch looks incomplete to me: >> >> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >> between events. >> >> -example/timer/odp_timer_test.c also uses it. (I am honestely not really >> sure why...) >> >> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >> event_types.h if it is not meant to be used. >> >> ... and thanks for taking the time to look at it. >> >> >> >> Crypto completion event type is certainly meant to be used. If >> implementation misuses the event type, the implementation needs to be >> fixed. When an application receives an event for crypto completion >> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >> >> >> >> >> >> Christophe >> >> >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >> platform/linux-generic/odp_buffer.c | 7 ------- >> platform/linux-generic/odp_crypto.c | 5 ----- >> 3 files changed, 21 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >> b/platform/linux-generic/include/odp_buffer_internal.h >> index e7b7568..2595b2d 100644 >> --- a/platform/linux-generic/include/odp_buffer_internal.h >> +++ b/platform/linux-generic/include/odp_buffer_internal.h >> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t >> size); >> */ >> int _odp_buffer_type(odp_buffer_t buf); >> >> -/* >> - * Buffer type set >> - * >> - * @param buf Buffer handle >> - * @param type New type value >> - * >> - */ >> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >> - >> #ifdef __cplusplus >> } >> #endif >> diff --git a/platform/linux-generic/odp_buffer.c >> b/platform/linux-generic/odp_buffer.c >> index 0803805..19b9dbd 100644 >> --- a/platform/linux-generic/odp_buffer.c >> +++ b/platform/linux-generic/odp_buffer.c >> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >> return hdr->type; >> } >> >> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >> -{ >> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >> - >> - hdr->type = type; >> -} >> - >> >> int odp_buffer_is_valid(odp_buffer_t buf) >> { >> diff --git a/platform/linux-generic/odp_crypto.c >> b/platform/linux-generic/odp_crypto.c >> index b16316c..e103066 100644 >> --- a/platform/linux-generic/odp_crypto.c >> +++ b/platform/linux-generic/odp_crypto.c >> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params, >> >> /* Linux generic will always use packet for completion >> event */ >> completion_event = odp_packet_to_event(params->out_pkt); >> - >> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >> - ODP_EVENT_CRYPTO_COMPL); >> >> /* Asynchronous, build result (no HW so no errors) and >> send it*/ >> op_result = get_op_result_from_event(completion_event); >> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, odp_bool_t >> use_entropy ODP_UNUSED) >> >> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >> { >> - /* This check not mandated by the API specification */ >> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >> - ODP_ABORT("Event not a crypto completion"); >> >> >> >> This check should be there in the reference implementation to catch bugs. >> Event type must be CRYPTO_COMPL when entering this function. >> >> >> >> -Petri >> >> >> >> >> return (odp_crypto_compl_t)ev; >> } >> >> -- >> 2.1.0 >> >> _______________________________________________ >> 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 >> >> >
The proposal I described during today's ARCH meeting is to add an explicit event type to buffers so that when buffers (of whatever flavor) are being used as events this field can be interrogated without needing to overwrite the buffer type field. As I mentioned, I'll post a patch for this later today for review. On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > >> As Petri writes, the type of a crypto completion event must be visible to >> the user (who is receiving the event and responsible for figuring out what >> to do with those events). Different types of events may be received from >> the same queue and require different action. >> >> I think it is useful for an implementation to allow buffer pool and >> buffer event code to be used for other types of events as well, e.g. crypto >> completion events and timeout events. The timer patch I did on Taras' >> request based timeouts on buffers with the timeout-specific data stored in >> the buffer data so no need for a timeout-specific header. This design >> simplifies reuse on other platforms where the event (HW buffer) definition >> is fixed and there might be no room for additional event-type specific >> headers. >> > Just to clearify. An implementation doing the above would still have > specific pools for crypto completion and timeout events. No need to > dynamically change the type of an event. > > >> >> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >> petri.savolainen@nokia.com> wrote: >> >>> >>> >>> >>> >>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of >>> *ext Christophe Milard >>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>> *To:* Bill Fischofer >>> *Cc:* LNG ODP Mailman List >>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>> buffer type hack for completions >>> >>> >>> >>> >>> >>> >>> >>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>> wrote: >>> >>> odp_crypto_operation() should not change underlying buffer types for >>> completions as these are purely internal and not needed, and doing >>> so is confusing and error-prome. >>> >>> >>> >>> Yes, Bill, I would prefer your approach: my patch felt like a hack to >>> correct for another (hence the RFC). >>> >>> However, this patch looks incomplete to me: >>> >>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>> between events. >>> >>> -example/timer/odp_timer_test.c also uses it. (I am honestely not really >>> sure why...) >>> >>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>> event_types.h if it is not meant to be used. >>> >>> ... and thanks for taking the time to look at it. >>> >>> >>> >>> Crypto completion event type is certainly meant to be used. If >>> implementation misuses the event type, the implementation needs to be >>> fixed. When an application receives an event for crypto completion >>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>> >>> >>> >>> >>> >>> Christophe >>> >>> >>> >>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>> --- >>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>> platform/linux-generic/odp_buffer.c | 7 ------- >>> platform/linux-generic/odp_crypto.c | 5 ----- >>> 3 files changed, 21 deletions(-) >>> >>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>> b/platform/linux-generic/include/odp_buffer_internal.h >>> index e7b7568..2595b2d 100644 >>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t >>> size); >>> */ >>> int _odp_buffer_type(odp_buffer_t buf); >>> >>> -/* >>> - * Buffer type set >>> - * >>> - * @param buf Buffer handle >>> - * @param type New type value >>> - * >>> - */ >>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>> - >>> #ifdef __cplusplus >>> } >>> #endif >>> diff --git a/platform/linux-generic/odp_buffer.c >>> b/platform/linux-generic/odp_buffer.c >>> index 0803805..19b9dbd 100644 >>> --- a/platform/linux-generic/odp_buffer.c >>> +++ b/platform/linux-generic/odp_buffer.c >>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>> return hdr->type; >>> } >>> >>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>> -{ >>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>> - >>> - hdr->type = type; >>> -} >>> - >>> >>> int odp_buffer_is_valid(odp_buffer_t buf) >>> { >>> diff --git a/platform/linux-generic/odp_crypto.c >>> b/platform/linux-generic/odp_crypto.c >>> index b16316c..e103066 100644 >>> --- a/platform/linux-generic/odp_crypto.c >>> +++ b/platform/linux-generic/odp_crypto.c >>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params, >>> >>> /* Linux generic will always use packet for completion >>> event */ >>> completion_event = odp_packet_to_event(params->out_pkt); >>> - >>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>> - ODP_EVENT_CRYPTO_COMPL); >>> >>> /* Asynchronous, build result (no HW so no errors) and >>> send it*/ >>> op_result = get_op_result_from_event(completion_event); >>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>> odp_bool_t use_entropy ODP_UNUSED) >>> >>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>> { >>> - /* This check not mandated by the API specification */ >>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>> - ODP_ABORT("Event not a crypto completion"); >>> >>> >>> >>> This check should be there in the reference implementation to catch >>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>> >>> >>> >>> -Petri >>> >>> >>> >>> >>> return (odp_crypto_compl_t)ev; >>> } >>> >>> -- >>> 2.1.0 >>> >>> _______________________________________________ >>> 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 >>> >>> >> >
On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> wrote: > The proposal I described during today's ARCH meeting is to add an explicit > event type to buffers so that when buffers (of whatever flavor) are being > used as events this field can be interrogated without needing to overwrite > the buffer type field. As I mentioned, I'll post a patch for this later > today for review. > So the original "buffer" type field is there to information the ODP implementation while the new event type field is there to information the application? This allows e.g. the crypto code to allocate a buffer (with a suitable data size) from a buffer pool and pretend it is a crypto completion event so that the application treats it like a crypto completion event and not as a buffer. Also we don't need to create any explicit crypto completion event pool, they are all allocated from some suitable buffer pool. Did I understand this correctly? > On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: > >> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> >>> As Petri writes, the type of a crypto completion event must be visible >>> to the user (who is receiving the event and responsible for figuring out >>> what to do with those events). Different types of events may be received >>> from the same queue and require different action. >>> >>> I think it is useful for an implementation to allow buffer pool and >>> buffer event code to be used for other types of events as well, e.g. crypto >>> completion events and timeout events. The timer patch I did on Taras' >>> request based timeouts on buffers with the timeout-specific data stored in >>> the buffer data so no need for a timeout-specific header. This design >>> simplifies reuse on other platforms where the event (HW buffer) definition >>> is fixed and there might be no room for additional event-type specific >>> headers. >>> >> Just to clearify. An implementation doing the above would still have >> specific pools for crypto completion and timeout events. No need to >> dynamically change the type of an event. >> >> >>> >>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>> petri.savolainen@nokia.com> wrote: >>> >>>> >>>> >>>> >>>> >>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf >>>> Of *ext Christophe Milard >>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>> *To:* Bill Fischofer >>>> *Cc:* LNG ODP Mailman List >>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>> buffer type hack for completions >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>> wrote: >>>> >>>> odp_crypto_operation() should not change underlying buffer types for >>>> completions as these are purely internal and not needed, and doing >>>> so is confusing and error-prome. >>>> >>>> >>>> >>>> Yes, Bill, I would prefer your approach: my patch felt like a hack to >>>> correct for another (hence the RFC). >>>> >>>> However, this patch looks incomplete to me: >>>> >>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>> between events. >>>> >>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>> really sure why...) >>>> >>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>> event_types.h if it is not meant to be used. >>>> >>>> ... and thanks for taking the time to look at it. >>>> >>>> >>>> >>>> Crypto completion event type is certainly meant to be used. If >>>> implementation misuses the event type, the implementation needs to be >>>> fixed. When an application receives an event for crypto completion >>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>> >>>> >>>> >>>> >>>> >>>> Christophe >>>> >>>> >>>> >>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>> --- >>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>> 3 files changed, 21 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>> index e7b7568..2595b2d 100644 >>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t >>>> size); >>>> */ >>>> int _odp_buffer_type(odp_buffer_t buf); >>>> >>>> -/* >>>> - * Buffer type set >>>> - * >>>> - * @param buf Buffer handle >>>> - * @param type New type value >>>> - * >>>> - */ >>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>> - >>>> #ifdef __cplusplus >>>> } >>>> #endif >>>> diff --git a/platform/linux-generic/odp_buffer.c >>>> b/platform/linux-generic/odp_buffer.c >>>> index 0803805..19b9dbd 100644 >>>> --- a/platform/linux-generic/odp_buffer.c >>>> +++ b/platform/linux-generic/odp_buffer.c >>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>> return hdr->type; >>>> } >>>> >>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>> -{ >>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>> - >>>> - hdr->type = type; >>>> -} >>>> - >>>> >>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>> { >>>> diff --git a/platform/linux-generic/odp_crypto.c >>>> b/platform/linux-generic/odp_crypto.c >>>> index b16316c..e103066 100644 >>>> --- a/platform/linux-generic/odp_crypto.c >>>> +++ b/platform/linux-generic/odp_crypto.c >>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params, >>>> >>>> /* Linux generic will always use packet for completion >>>> event */ >>>> completion_event = odp_packet_to_event(params->out_pkt); >>>> - >>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>> - ODP_EVENT_CRYPTO_COMPL); >>>> >>>> /* Asynchronous, build result (no HW so no errors) and >>>> send it*/ >>>> op_result = get_op_result_from_event(completion_event); >>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>> odp_bool_t use_entropy ODP_UNUSED) >>>> >>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>> { >>>> - /* This check not mandated by the API specification */ >>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>> - ODP_ABORT("Event not a crypto completion"); >>>> >>>> >>>> >>>> This check should be there in the reference implementation to catch >>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>> >>>> >>>> >>>> -Petri >>>> >>>> >>>> >>>> >>>> return (odp_crypto_compl_t)ev; >>>> } >>>> >>>> -- >>>> 2.1.0 >>>> >>>> _______________________________________________ >>>> 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 >>>> >>>> >>> >> >
Completion events are now implementation's responsibility. Application does not provide any pool for completion events, only for output packet in case the crypto HW engine is able to allocate output packets. I think the whole completion event thing was misunderstood as a HW provided thing. I'm not aware of (async) crypto engines which in addition to input/output packets/buffers allocate a separate "thing" which we could abstract as a completion event, at least in the current ODP implementations. Initially the completion event was specified by the application and its meaning was a schedulable/queue-able entity which should return the aggregate output from and the context of a crypto operation. A plain buffer and some accessor functions seemed to be fit for this purpose. The problem was the allocation of this buffer so the input packet buffer was reused for this, initially explicitly in the application and now hidden into the implementation. This was/is regarded as a hack but I would like to understand why is considered so. Moving a hack from application to implementation still is a hack. Alex On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > > > On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> The proposal I described during today's ARCH meeting is to add an >> explicit event type to buffers so that when buffers (of whatever flavor) >> are being used as events this field can be interrogated without needing to >> overwrite the buffer type field. As I mentioned, I'll post a patch for >> this later today for review. >> > So the original "buffer" type field is there to information the ODP > implementation while the new event type field is there to information the > application? This allows e.g. the crypto code to allocate a buffer (with a > suitable data size) from a buffer pool and pretend it is a crypto > completion event so that the application treats it like a crypto completion > event and not as a buffer. Also we don't need to create any explicit crypto > completion event pool, they are all allocated from some suitable buffer > pool. > > Did I understand this correctly? > > > >> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl <ola.liljedahl@linaro.org> >> wrote: >> >>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>> wrote: >>> >>>> As Petri writes, the type of a crypto completion event must be visible >>>> to the user (who is receiving the event and responsible for figuring out >>>> what to do with those events). Different types of events may be received >>>> from the same queue and require different action. >>>> >>>> I think it is useful for an implementation to allow buffer pool and >>>> buffer event code to be used for other types of events as well, e.g. crypto >>>> completion events and timeout events. The timer patch I did on Taras' >>>> request based timeouts on buffers with the timeout-specific data stored in >>>> the buffer data so no need for a timeout-specific header. This design >>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>> is fixed and there might be no room for additional event-type specific >>>> headers. >>>> >>> Just to clearify. An implementation doing the above would still have >>> specific pools for crypto completion and timeout events. No need to >>> dynamically change the type of an event. >>> >>> >>>> >>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>> petri.savolainen@nokia.com> wrote: >>>> >>>>> >>>>> >>>>> >>>>> >>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf >>>>> Of *ext Christophe Milard >>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>> *To:* Bill Fischofer >>>>> *Cc:* LNG ODP Mailman List >>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>> buffer type hack for completions >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>>> wrote: >>>>> >>>>> odp_crypto_operation() should not change underlying buffer types for >>>>> completions as these are purely internal and not needed, and doing >>>>> so is confusing and error-prome. >>>>> >>>>> >>>>> >>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack to >>>>> correct for another (hence the RFC). >>>>> >>>>> However, this patch looks incomplete to me: >>>>> >>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>> between events. >>>>> >>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>> really sure why...) >>>>> >>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>> event_types.h if it is not meant to be used. >>>>> >>>>> ... and thanks for taking the time to look at it. >>>>> >>>>> >>>>> >>>>> Crypto completion event type is certainly meant to be used. If >>>>> implementation misuses the event type, the implementation needs to be >>>>> fixed. When an application receives an event for crypto completion >>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> Christophe >>>>> >>>>> >>>>> >>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>> --- >>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>> 3 files changed, 21 deletions(-) >>>>> >>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>> index e7b7568..2595b2d 100644 >>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t >>>>> size); >>>>> */ >>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>> >>>>> -/* >>>>> - * Buffer type set >>>>> - * >>>>> - * @param buf Buffer handle >>>>> - * @param type New type value >>>>> - * >>>>> - */ >>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>> - >>>>> #ifdef __cplusplus >>>>> } >>>>> #endif >>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>> b/platform/linux-generic/odp_buffer.c >>>>> index 0803805..19b9dbd 100644 >>>>> --- a/platform/linux-generic/odp_buffer.c >>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>> return hdr->type; >>>>> } >>>>> >>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>> -{ >>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>> - >>>>> - hdr->type = type; >>>>> -} >>>>> - >>>>> >>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>> { >>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>> b/platform/linux-generic/odp_crypto.c >>>>> index b16316c..e103066 100644 >>>>> --- a/platform/linux-generic/odp_crypto.c >>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>> *params, >>>>> >>>>> /* Linux generic will always use packet for completion >>>>> event */ >>>>> completion_event = >>>>> odp_packet_to_event(params->out_pkt); >>>>> - >>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>> >>>>> /* Asynchronous, build result (no HW so no errors) and >>>>> send it*/ >>>>> op_result = get_op_result_from_event(completion_event); >>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>> >>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>> { >>>>> - /* This check not mandated by the API specification */ >>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>> - ODP_ABORT("Event not a crypto completion"); >>>>> >>>>> >>>>> >>>>> This check should be there in the reference implementation to catch >>>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>> >>>>> >>>>> >>>>> -Petri >>>>> >>>>> >>>>> >>>>> >>>>> return (odp_crypto_compl_t)ev; >>>>> } >>>>> >>>>> -- >>>>> 2.1.0 >>>>> >>>>> _______________________________________________ >>>>> 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 > >
This is for internal use of buffers as events, specifically in the case of crypto, which overloads events onto packets. Essentially a packet containing data is sent off for a crypto operation as is returned as a crypto completion event, whereupon it can resume its use as a packet. There were two issues with the current hack that this patch addresses: 1. Changing the buffer type, which is confusing and led to the bug that Christophe originally identified. 2. Trying to fit metadata into the packet data. This is a brittle solution since if I try to perform a crypto operation on a "full" buffer such attempts will overwrite application data. The explicit event_type solves the first issue while the latter is solved by making the completion data explicit packet metadata. Since any packet may be subject to crypto processing this is needed in any event. On Fri, Jun 12, 2015 at 3:22 AM, Alexandru Badicioiu < alexandru.badicioiu@linaro.org> wrote: > Completion events are now implementation's responsibility. Application > does not provide any pool for completion events, only for output packet in > case the crypto HW engine is able to allocate output packets. > I think the whole completion event thing was misunderstood as a HW > provided thing. I'm not aware of (async) crypto engines which in addition > to input/output packets/buffers allocate a separate "thing" which we could > abstract as a completion event, at least in the current ODP implementations. > Initially the completion event was specified by the application and its > meaning was a schedulable/queue-able entity which should return the > aggregate output from and the context of a crypto operation. A plain buffer > and some accessor functions seemed to be fit for this purpose. The problem > was the allocation of this buffer so the input packet buffer was reused for > this, initially explicitly in the application and now hidden into the > implementation. This was/is regarded as a hack but I would like to > understand why is considered so. Moving a hack from application to > implementation still is a hack. > > Alex > > On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > >> >> >> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >>> The proposal I described during today's ARCH meeting is to add an >>> explicit event type to buffers so that when buffers (of whatever flavor) >>> are being used as events this field can be interrogated without needing to >>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>> this later today for review. >>> >> So the original "buffer" type field is there to information the ODP >> implementation while the new event type field is there to information the >> application? This allows e.g. the crypto code to allocate a buffer (with a >> suitable data size) from a buffer pool and pretend it is a crypto >> completion event so that the application treats it like a crypto completion >> event and not as a buffer. Also we don't need to create any explicit crypto >> completion event pool, they are all allocated from some suitable buffer >> pool. >> >> Did I understand this correctly? >> >> >> >>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl <ola.liljedahl@linaro.org >>> > wrote: >>> >>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>> wrote: >>>> >>>>> As Petri writes, the type of a crypto completion event must be visible >>>>> to the user (who is receiving the event and responsible for figuring out >>>>> what to do with those events). Different types of events may be received >>>>> from the same queue and require different action. >>>>> >>>>> I think it is useful for an implementation to allow buffer pool and >>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>> completion events and timeout events. The timer patch I did on Taras' >>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>> the buffer data so no need for a timeout-specific header. This design >>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>> is fixed and there might be no room for additional event-type specific >>>>> headers. >>>>> >>>> Just to clearify. An implementation doing the above would still have >>>> specific pools for crypto completion and timeout events. No need to >>>> dynamically change the type of an event. >>>> >>>> >>>>> >>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>> petri.savolainen@nokia.com> wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf >>>>>> Of *ext Christophe Milard >>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>> *To:* Bill Fischofer >>>>>> *Cc:* LNG ODP Mailman List >>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>> buffer type hack for completions >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>>>> wrote: >>>>>> >>>>>> odp_crypto_operation() should not change underlying buffer types for >>>>>> completions as these are purely internal and not needed, and doing >>>>>> so is confusing and error-prome. >>>>>> >>>>>> >>>>>> >>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack to >>>>>> correct for another (hence the RFC). >>>>>> >>>>>> However, this patch looks incomplete to me: >>>>>> >>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>> between events. >>>>>> >>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>> really sure why...) >>>>>> >>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>> event_types.h if it is not meant to be used. >>>>>> >>>>>> ... and thanks for taking the time to look at it. >>>>>> >>>>>> >>>>>> >>>>>> Crypto completion event type is certainly meant to be used. If >>>>>> implementation misuses the event type, the implementation needs to be >>>>>> fixed. When an application receives an event for crypto completion >>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Christophe >>>>>> >>>>>> >>>>>> >>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>> --- >>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>> 3 files changed, 21 deletions(-) >>>>>> >>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>> index e7b7568..2595b2d 100644 >>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>> size_t size); >>>>>> */ >>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>> >>>>>> -/* >>>>>> - * Buffer type set >>>>>> - * >>>>>> - * @param buf Buffer handle >>>>>> - * @param type New type value >>>>>> - * >>>>>> - */ >>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>> - >>>>>> #ifdef __cplusplus >>>>>> } >>>>>> #endif >>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>> b/platform/linux-generic/odp_buffer.c >>>>>> index 0803805..19b9dbd 100644 >>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>> return hdr->type; >>>>>> } >>>>>> >>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>> -{ >>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>> - >>>>>> - hdr->type = type; >>>>>> -} >>>>>> - >>>>>> >>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>> { >>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>> b/platform/linux-generic/odp_crypto.c >>>>>> index b16316c..e103066 100644 >>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>> *params, >>>>>> >>>>>> /* Linux generic will always use packet for >>>>>> completion event */ >>>>>> completion_event = >>>>>> odp_packet_to_event(params->out_pkt); >>>>>> - >>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>> >>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>> and send it*/ >>>>>> op_result = >>>>>> get_op_result_from_event(completion_event); >>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>> >>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>> { >>>>>> - /* This check not mandated by the API specification */ >>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>> >>>>>> >>>>>> >>>>>> This check should be there in the reference implementation to catch >>>>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>> >>>>>> >>>>>> >>>>>> -Petri >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> return (odp_crypto_compl_t)ev; >>>>>> } >>>>>> >>>>>> -- >>>>>> 2.1.0 >>>>>> >>>>>> _______________________________________________ >>>>>> 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 >> >> >
But are you OK with the architecture (API) Alex? Isn't it good from an implementation point of view that it is the implementation that is responsible for the crypto completion event? If this is the input packet reused or a separate event is determined by the implementation according to the design and requirements of the HW. If the application specified the crypto completion event, it is likely that this event would have to be allocated and freed for every crypto operation. This seems wasteful. On 12 June 2015 at 10:22, Alexandru Badicioiu < alexandru.badicioiu@linaro.org> wrote: > Completion events are now implementation's responsibility. Application > does not provide any pool for completion events, only for output packet in > case the crypto HW engine is able to allocate output packets. > I think the whole completion event thing was misunderstood as a HW > provided thing. I'm not aware of (async) crypto engines which in addition > to input/output packets/buffers allocate a separate "thing" which we could > abstract as a completion event, at least in the current ODP implementations. > Initially the completion event was specified by the application and its > meaning was a schedulable/queue-able entity which should return the > aggregate output from and the context of a crypto operation. A plain buffer > and some accessor functions seemed to be fit for this purpose. The problem > was the allocation of this buffer so the input packet buffer was reused for > this, initially explicitly in the application and now hidden into the > implementation. This was/is regarded as a hack but I would like to > understand why is considered so. Moving a hack from application to > implementation still is a hack. > > Alex > > On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > >> >> >> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >>> The proposal I described during today's ARCH meeting is to add an >>> explicit event type to buffers so that when buffers (of whatever flavor) >>> are being used as events this field can be interrogated without needing to >>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>> this later today for review. >>> >> So the original "buffer" type field is there to information the ODP >> implementation while the new event type field is there to information the >> application? This allows e.g. the crypto code to allocate a buffer (with a >> suitable data size) from a buffer pool and pretend it is a crypto >> completion event so that the application treats it like a crypto completion >> event and not as a buffer. Also we don't need to create any explicit crypto >> completion event pool, they are all allocated from some suitable buffer >> pool. >> >> Did I understand this correctly? >> >> >> >>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl <ola.liljedahl@linaro.org >>> > wrote: >>> >>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>> wrote: >>>> >>>>> As Petri writes, the type of a crypto completion event must be visible >>>>> to the user (who is receiving the event and responsible for figuring out >>>>> what to do with those events). Different types of events may be received >>>>> from the same queue and require different action. >>>>> >>>>> I think it is useful for an implementation to allow buffer pool and >>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>> completion events and timeout events. The timer patch I did on Taras' >>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>> the buffer data so no need for a timeout-specific header. This design >>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>> is fixed and there might be no room for additional event-type specific >>>>> headers. >>>>> >>>> Just to clearify. An implementation doing the above would still have >>>> specific pools for crypto completion and timeout events. No need to >>>> dynamically change the type of an event. >>>> >>>> >>>>> >>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>> petri.savolainen@nokia.com> wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf >>>>>> Of *ext Christophe Milard >>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>> *To:* Bill Fischofer >>>>>> *Cc:* LNG ODP Mailman List >>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>> buffer type hack for completions >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>>>> wrote: >>>>>> >>>>>> odp_crypto_operation() should not change underlying buffer types for >>>>>> completions as these are purely internal and not needed, and doing >>>>>> so is confusing and error-prome. >>>>>> >>>>>> >>>>>> >>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack to >>>>>> correct for another (hence the RFC). >>>>>> >>>>>> However, this patch looks incomplete to me: >>>>>> >>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>> between events. >>>>>> >>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>> really sure why...) >>>>>> >>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>> event_types.h if it is not meant to be used. >>>>>> >>>>>> ... and thanks for taking the time to look at it. >>>>>> >>>>>> >>>>>> >>>>>> Crypto completion event type is certainly meant to be used. If >>>>>> implementation misuses the event type, the implementation needs to be >>>>>> fixed. When an application receives an event for crypto completion >>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Christophe >>>>>> >>>>>> >>>>>> >>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>> --- >>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>> 3 files changed, 21 deletions(-) >>>>>> >>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>> index e7b7568..2595b2d 100644 >>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>> size_t size); >>>>>> */ >>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>> >>>>>> -/* >>>>>> - * Buffer type set >>>>>> - * >>>>>> - * @param buf Buffer handle >>>>>> - * @param type New type value >>>>>> - * >>>>>> - */ >>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>> - >>>>>> #ifdef __cplusplus >>>>>> } >>>>>> #endif >>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>> b/platform/linux-generic/odp_buffer.c >>>>>> index 0803805..19b9dbd 100644 >>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>> return hdr->type; >>>>>> } >>>>>> >>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>> -{ >>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>> - >>>>>> - hdr->type = type; >>>>>> -} >>>>>> - >>>>>> >>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>> { >>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>> b/platform/linux-generic/odp_crypto.c >>>>>> index b16316c..e103066 100644 >>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>> *params, >>>>>> >>>>>> /* Linux generic will always use packet for >>>>>> completion event */ >>>>>> completion_event = >>>>>> odp_packet_to_event(params->out_pkt); >>>>>> - >>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>> >>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>> and send it*/ >>>>>> op_result = >>>>>> get_op_result_from_event(completion_event); >>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>> >>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>> { >>>>>> - /* This check not mandated by the API specification */ >>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>> >>>>>> >>>>>> >>>>>> This check should be there in the reference implementation to catch >>>>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>> >>>>>> >>>>>> >>>>>> -Petri >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> return (odp_crypto_compl_t)ev; >>>>>> } >>>>>> >>>>>> -- >>>>>> 2.1.0 >>>>>> >>>>>> _______________________________________________ >>>>>> 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 >> >> >
I'm fine with the completion event being implementation responsibility, but I don't see much difference from being application responsibility, at least with current implementations, from the HW or performance point of view. Using the input packet or other thing explicitly in the application or hidden in the implementation is the same. As I mentioned, I'm not aware of crypto support for HW provided "completion events" - in fact HW sends events only by generating interrupts. Async crypto operation is essentially an async I/O read operation - regular async I/O uses user buffers to return the results. Initial design followed this approach. Crypto operation results would fit more the packet context rather than the metadata. Not all packets are likely to be subject to crypto operations - for example ARP packets. odp_ipsec application already allocates and frees a context per packet input. On 12 June 2015 at 17:11, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > But are you OK with the architecture (API) Alex? > Isn't it good from an implementation point of view that it is the > implementation that is responsible for the crypto completion event? If this > is the input packet reused or a separate event is determined by the > implementation according to the design and requirements of the HW. > If the application specified the crypto completion event, it is likely > that this event would have to be allocated and freed for every crypto > operation. This seems wasteful. > > On 12 June 2015 at 10:22, Alexandru Badicioiu < > alexandru.badicioiu@linaro.org> wrote: > >> Completion events are now implementation's responsibility. Application >> does not provide any pool for completion events, only for output packet in >> case the crypto HW engine is able to allocate output packets. >> I think the whole completion event thing was misunderstood as a HW >> provided thing. I'm not aware of (async) crypto engines which in addition >> to input/output packets/buffers allocate a separate "thing" which we could >> abstract as a completion event, at least in the current ODP implementations. >> Initially the completion event was specified by the application and its >> meaning was a schedulable/queue-able entity which should return the >> aggregate output from and the context of a crypto operation. A plain buffer >> and some accessor functions seemed to be fit for this purpose. The problem >> was the allocation of this buffer so the input packet buffer was reused for >> this, initially explicitly in the application and now hidden into the >> implementation. This was/is regarded as a hack but I would like to >> understand why is considered so. Moving a hack from application to >> implementation still is a hack. >> >> Alex >> >> On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> >>> >>> >>> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >>> wrote: >>> >>>> The proposal I described during today's ARCH meeting is to add an >>>> explicit event type to buffers so that when buffers (of whatever flavor) >>>> are being used as events this field can be interrogated without needing to >>>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>>> this later today for review. >>>> >>> So the original "buffer" type field is there to information the ODP >>> implementation while the new event type field is there to information the >>> application? This allows e.g. the crypto code to allocate a buffer (with a >>> suitable data size) from a buffer pool and pretend it is a crypto >>> completion event so that the application treats it like a crypto completion >>> event and not as a buffer. Also we don't need to create any explicit crypto >>> completion event pool, they are all allocated from some suitable buffer >>> pool. >>> >>> Did I understand this correctly? >>> >>> >>> >>>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl < >>>> ola.liljedahl@linaro.org> wrote: >>>> >>>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>>> wrote: >>>>> >>>>>> As Petri writes, the type of a crypto completion event must be >>>>>> visible to the user (who is receiving the event and responsible for >>>>>> figuring out what to do with those events). Different types of events may >>>>>> be received from the same queue and require different action. >>>>>> >>>>>> I think it is useful for an implementation to allow buffer pool and >>>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>>> completion events and timeout events. The timer patch I did on Taras' >>>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>>> the buffer data so no need for a timeout-specific header. This design >>>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>>> is fixed and there might be no room for additional event-type specific >>>>>> headers. >>>>>> >>>>> Just to clearify. An implementation doing the above would still have >>>>> specific pools for crypto completion and timeout events. No need to >>>>> dynamically change the type of an event. >>>>> >>>>> >>>>>> >>>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>>> petri.savolainen@nokia.com> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On >>>>>>> Behalf Of *ext Christophe Milard >>>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>>> *To:* Bill Fischofer >>>>>>> *Cc:* LNG ODP Mailman List >>>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>>> buffer type hack for completions >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>>>>> wrote: >>>>>>> >>>>>>> odp_crypto_operation() should not change underlying buffer types for >>>>>>> completions as these are purely internal and not needed, and doing >>>>>>> so is confusing and error-prome. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack >>>>>>> to correct for another (hence the RFC). >>>>>>> >>>>>>> However, this patch looks incomplete to me: >>>>>>> >>>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>>> between events. >>>>>>> >>>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>>> really sure why...) >>>>>>> >>>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>>> event_types.h if it is not meant to be used. >>>>>>> >>>>>>> ... and thanks for taking the time to look at it. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Crypto completion event type is certainly meant to be used. If >>>>>>> implementation misuses the event type, the implementation needs to be >>>>>>> fixed. When an application receives an event for crypto completion >>>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Christophe >>>>>>> >>>>>>> >>>>>>> >>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>>> --- >>>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>>> 3 files changed, 21 deletions(-) >>>>>>> >>>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> index e7b7568..2595b2d 100644 >>>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>>> size_t size); >>>>>>> */ >>>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>>> >>>>>>> -/* >>>>>>> - * Buffer type set >>>>>>> - * >>>>>>> - * @param buf Buffer handle >>>>>>> - * @param type New type value >>>>>>> - * >>>>>>> - */ >>>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>>> - >>>>>>> #ifdef __cplusplus >>>>>>> } >>>>>>> #endif >>>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>>> b/platform/linux-generic/odp_buffer.c >>>>>>> index 0803805..19b9dbd 100644 >>>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>>> return hdr->type; >>>>>>> } >>>>>>> >>>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>>> -{ >>>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>>> - >>>>>>> - hdr->type = type; >>>>>>> -} >>>>>>> - >>>>>>> >>>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>>> { >>>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>>> b/platform/linux-generic/odp_crypto.c >>>>>>> index b16316c..e103066 100644 >>>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>>> *params, >>>>>>> >>>>>>> /* Linux generic will always use packet for >>>>>>> completion event */ >>>>>>> completion_event = >>>>>>> odp_packet_to_event(params->out_pkt); >>>>>>> - >>>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>>> >>>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>>> and send it*/ >>>>>>> op_result = >>>>>>> get_op_result_from_event(completion_event); >>>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>>> >>>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>>> { >>>>>>> - /* This check not mandated by the API specification */ >>>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>>> >>>>>>> >>>>>>> >>>>>>> This check should be there in the reference implementation to catch >>>>>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>>> >>>>>>> >>>>>>> >>>>>>> -Petri >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> return (odp_crypto_compl_t)ev; >>>>>>> } >>>>>>> >>>>>>> -- >>>>>>> 2.1.0 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> 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 >>> >>> >> > On 12 June 2015 at 17:11, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > But are you OK with the architecture (API) Alex? > Isn't it good from an implementation point of view that it is the > implementation that is responsible for the crypto completion event? If this > is the input packet reused or a separate event is determined by the > implementation according to the design and requirements of the HW. > If the application specified the crypto completion event, it is likely > that this event would have to be allocated and freed for every crypto > operation. This seems wasteful. > > On 12 June 2015 at 10:22, Alexandru Badicioiu < > alexandru.badicioiu@linaro.org> wrote: > >> Completion events are now implementation's responsibility. Application >> does not provide any pool for completion events, only for output packet in >> case the crypto HW engine is able to allocate output packets. >> I think the whole completion event thing was misunderstood as a HW >> provided thing. I'm not aware of (async) crypto engines which in addition >> to input/output packets/buffers allocate a separate "thing" which we could >> abstract as a completion event, at least in the current ODP implementations. >> Initially the completion event was specified by the application and its >> meaning was a schedulable/queue-able entity which should return the >> aggregate output from and the context of a crypto operation. A plain buffer >> and some accessor functions seemed to be fit for this purpose. The problem >> was the allocation of this buffer so the input packet buffer was reused for >> this, initially explicitly in the application and now hidden into the >> implementation. This was/is regarded as a hack but I would like to >> understand why is considered so. Moving a hack from application to >> implementation still is a hack. >> >> Alex >> >> On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> >>> >>> >>> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >>> wrote: >>> >>>> The proposal I described during today's ARCH meeting is to add an >>>> explicit event type to buffers so that when buffers (of whatever flavor) >>>> are being used as events this field can be interrogated without needing to >>>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>>> this later today for review. >>>> >>> So the original "buffer" type field is there to information the ODP >>> implementation while the new event type field is there to information the >>> application? This allows e.g. the crypto code to allocate a buffer (with a >>> suitable data size) from a buffer pool and pretend it is a crypto >>> completion event so that the application treats it like a crypto completion >>> event and not as a buffer. Also we don't need to create any explicit crypto >>> completion event pool, they are all allocated from some suitable buffer >>> pool. >>> >>> Did I understand this correctly? >>> >>> >>> >>>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl < >>>> ola.liljedahl@linaro.org> wrote: >>>> >>>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>>> wrote: >>>>> >>>>>> As Petri writes, the type of a crypto completion event must be >>>>>> visible to the user (who is receiving the event and responsible for >>>>>> figuring out what to do with those events). Different types of events may >>>>>> be received from the same queue and require different action. >>>>>> >>>>>> I think it is useful for an implementation to allow buffer pool and >>>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>>> completion events and timeout events. The timer patch I did on Taras' >>>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>>> the buffer data so no need for a timeout-specific header. This design >>>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>>> is fixed and there might be no room for additional event-type specific >>>>>> headers. >>>>>> >>>>> Just to clearify. An implementation doing the above would still have >>>>> specific pools for crypto completion and timeout events. No need to >>>>> dynamically change the type of an event. >>>>> >>>>> >>>>>> >>>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>>> petri.savolainen@nokia.com> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On >>>>>>> Behalf Of *ext Christophe Milard >>>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>>> *To:* Bill Fischofer >>>>>>> *Cc:* LNG ODP Mailman List >>>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>>> buffer type hack for completions >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>>>>> wrote: >>>>>>> >>>>>>> odp_crypto_operation() should not change underlying buffer types for >>>>>>> completions as these are purely internal and not needed, and doing >>>>>>> so is confusing and error-prome. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack >>>>>>> to correct for another (hence the RFC). >>>>>>> >>>>>>> However, this patch looks incomplete to me: >>>>>>> >>>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>>> between events. >>>>>>> >>>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>>> really sure why...) >>>>>>> >>>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>>> event_types.h if it is not meant to be used. >>>>>>> >>>>>>> ... and thanks for taking the time to look at it. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Crypto completion event type is certainly meant to be used. If >>>>>>> implementation misuses the event type, the implementation needs to be >>>>>>> fixed. When an application receives an event for crypto completion >>>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Christophe >>>>>>> >>>>>>> >>>>>>> >>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>>> --- >>>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>>> 3 files changed, 21 deletions(-) >>>>>>> >>>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> index e7b7568..2595b2d 100644 >>>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>>> size_t size); >>>>>>> */ >>>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>>> >>>>>>> -/* >>>>>>> - * Buffer type set >>>>>>> - * >>>>>>> - * @param buf Buffer handle >>>>>>> - * @param type New type value >>>>>>> - * >>>>>>> - */ >>>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>>> - >>>>>>> #ifdef __cplusplus >>>>>>> } >>>>>>> #endif >>>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>>> b/platform/linux-generic/odp_buffer.c >>>>>>> index 0803805..19b9dbd 100644 >>>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>>> return hdr->type; >>>>>>> } >>>>>>> >>>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>>> -{ >>>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>>> - >>>>>>> - hdr->type = type; >>>>>>> -} >>>>>>> - >>>>>>> >>>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>>> { >>>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>>> b/platform/linux-generic/odp_crypto.c >>>>>>> index b16316c..e103066 100644 >>>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>>> *params, >>>>>>> >>>>>>> /* Linux generic will always use packet for >>>>>>> completion event */ >>>>>>> completion_event = >>>>>>> odp_packet_to_event(params->out_pkt); >>>>>>> - >>>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>>> >>>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>>> and send it*/ >>>>>>> op_result = >>>>>>> get_op_result_from_event(completion_event); >>>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>>> >>>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>>> { >>>>>>> - /* This check not mandated by the API specification */ >>>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>>> >>>>>>> >>>>>>> >>>>>>> This check should be there in the reference implementation to catch >>>>>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>>> >>>>>>> >>>>>>> >>>>>>> -Petri >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> return (odp_crypto_compl_t)ev; >>>>>>> } >>>>>>> >>>>>>> -- >>>>>>> 2.1.0 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> 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 >>> >>> >> >
On 14 June 2015 at 14:28, Alexandru Badicioiu < alexandru.badicioiu@linaro.org> wrote: > I'm fine with the completion event being implementation responsibility, > but I don't see much difference from being application responsibility, at > least with current implementations, from the HW or performance point of > view. Using the input packet or other thing explicitly in the application > or hidden in the implementation is the same. As I mentioned, I'm not aware > of crypto support for HW provided "completion events" - in fact HW sends > events only by generating interrupts. > The HW you are working with perhaps but not necessarily all HW crypto engines, current of future. We want to keep the door open for future HW designs which are more streamlined and require less (perp-packet) overhead. The overhead of the crypto (ciphering) processing itself is going down so per-packet overheads may become bottlenecks (especially if they require update of shared data structures, e.g. when allocating/freeing buffers). > > Async crypto operation is essentially an async I/O read operation - > regular async I/O uses user buffers to return the results. > Initial design followed this approach. Crypto operation results would fit > more the packet context rather than the metadata. Not all packets are > likely to be subject to crypto operations - for example ARP packets. > odp_ipsec application already allocates and frees a context per packet > input. > This does not seem optimal. I hope this is just the way it happens to be done and not required design. I need to take a closer look. -- Ola > > > > > > > > On 12 June 2015 at 17:11, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > >> But are you OK with the architecture (API) Alex? >> Isn't it good from an implementation point of view that it is the >> implementation that is responsible for the crypto completion event? If this >> is the input packet reused or a separate event is determined by the >> implementation according to the design and requirements of the HW. >> If the application specified the crypto completion event, it is likely >> that this event would have to be allocated and freed for every crypto >> operation. This seems wasteful. >> >> On 12 June 2015 at 10:22, Alexandru Badicioiu < >> alexandru.badicioiu@linaro.org> wrote: >> >>> Completion events are now implementation's responsibility. Application >>> does not provide any pool for completion events, only for output packet in >>> case the crypto HW engine is able to allocate output packets. >>> I think the whole completion event thing was misunderstood as a HW >>> provided thing. I'm not aware of (async) crypto engines which in addition >>> to input/output packets/buffers allocate a separate "thing" which we could >>> abstract as a completion event, at least in the current ODP implementations. >>> Initially the completion event was specified by the application and its >>> meaning was a schedulable/queue-able entity which should return the >>> aggregate output from and the context of a crypto operation. A plain buffer >>> and some accessor functions seemed to be fit for this purpose. The problem >>> was the allocation of this buffer so the input packet buffer was reused for >>> this, initially explicitly in the application and now hidden into the >>> implementation. This was/is regarded as a hack but I would like to >>> understand why is considered so. Moving a hack from application to >>> implementation still is a hack. >>> >>> Alex >>> >>> On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> >>> wrote: >>> >>>> >>>> >>>> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >>>> wrote: >>>> >>>>> The proposal I described during today's ARCH meeting is to add an >>>>> explicit event type to buffers so that when buffers (of whatever flavor) >>>>> are being used as events this field can be interrogated without needing to >>>>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>>>> this later today for review. >>>>> >>>> So the original "buffer" type field is there to information the ODP >>>> implementation while the new event type field is there to information the >>>> application? This allows e.g. the crypto code to allocate a buffer (with a >>>> suitable data size) from a buffer pool and pretend it is a crypto >>>> completion event so that the application treats it like a crypto completion >>>> event and not as a buffer. Also we don't need to create any explicit crypto >>>> completion event pool, they are all allocated from some suitable buffer >>>> pool. >>>> >>>> Did I understand this correctly? >>>> >>>> >>>> >>>>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl < >>>>> ola.liljedahl@linaro.org> wrote: >>>>> >>>>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>>>> wrote: >>>>>> >>>>>>> As Petri writes, the type of a crypto completion event must be >>>>>>> visible to the user (who is receiving the event and responsible for >>>>>>> figuring out what to do with those events). Different types of events may >>>>>>> be received from the same queue and require different action. >>>>>>> >>>>>>> I think it is useful for an implementation to allow buffer pool and >>>>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>>>> completion events and timeout events. The timer patch I did on Taras' >>>>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>>>> the buffer data so no need for a timeout-specific header. This design >>>>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>>>> is fixed and there might be no room for additional event-type specific >>>>>>> headers. >>>>>>> >>>>>> Just to clearify. An implementation doing the above would still have >>>>>> specific pools for crypto completion and timeout events. No need to >>>>>> dynamically change the type of an event. >>>>>> >>>>>> >>>>>>> >>>>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>>>> petri.savolainen@nokia.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On >>>>>>>> Behalf Of *ext Christophe Milard >>>>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>>>> *To:* Bill Fischofer >>>>>>>> *Cc:* LNG ODP Mailman List >>>>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>>>> buffer type hack for completions >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>> odp_crypto_operation() should not change underlying buffer types for >>>>>>>> completions as these are purely internal and not needed, and doing >>>>>>>> so is confusing and error-prome. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack >>>>>>>> to correct for another (hence the RFC). >>>>>>>> >>>>>>>> However, this patch looks incomplete to me: >>>>>>>> >>>>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>>>> between events. >>>>>>>> >>>>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>>>> really sure why...) >>>>>>>> >>>>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>>>> event_types.h if it is not meant to be used. >>>>>>>> >>>>>>>> ... and thanks for taking the time to look at it. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Crypto completion event type is certainly meant to be used. If >>>>>>>> implementation misuses the event type, the implementation needs to be >>>>>>>> fixed. When an application receives an event for crypto completion >>>>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Christophe >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>>>> --- >>>>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>>>> 3 files changed, 21 deletions(-) >>>>>>>> >>>>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> index e7b7568..2595b2d 100644 >>>>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>>>> size_t size); >>>>>>>> */ >>>>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>>>> >>>>>>>> -/* >>>>>>>> - * Buffer type set >>>>>>>> - * >>>>>>>> - * @param buf Buffer handle >>>>>>>> - * @param type New type value >>>>>>>> - * >>>>>>>> - */ >>>>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>>>> - >>>>>>>> #ifdef __cplusplus >>>>>>>> } >>>>>>>> #endif >>>>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>>>> b/platform/linux-generic/odp_buffer.c >>>>>>>> index 0803805..19b9dbd 100644 >>>>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>>>> return hdr->type; >>>>>>>> } >>>>>>>> >>>>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>>>> -{ >>>>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>>>> - >>>>>>>> - hdr->type = type; >>>>>>>> -} >>>>>>>> - >>>>>>>> >>>>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>>>> { >>>>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>>>> b/platform/linux-generic/odp_crypto.c >>>>>>>> index b16316c..e103066 100644 >>>>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>>>> *params, >>>>>>>> >>>>>>>> /* Linux generic will always use packet for >>>>>>>> completion event */ >>>>>>>> completion_event = >>>>>>>> odp_packet_to_event(params->out_pkt); >>>>>>>> - >>>>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>>>> >>>>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>>>> and send it*/ >>>>>>>> op_result = >>>>>>>> get_op_result_from_event(completion_event); >>>>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>>>> >>>>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>>>> { >>>>>>>> - /* This check not mandated by the API specification */ >>>>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> This check should be there in the reference implementation to catch >>>>>>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -Petri >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> return (odp_crypto_compl_t)ev; >>>>>>>> } >>>>>>>> >>>>>>>> -- >>>>>>>> 2.1.0 >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> 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 >>>> >>>> >>> >> > > On 12 June 2015 at 17:11, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > >> But are you OK with the architecture (API) Alex? >> Isn't it good from an implementation point of view that it is the >> implementation that is responsible for the crypto completion event? If this >> is the input packet reused or a separate event is determined by the >> implementation according to the design and requirements of the HW. >> If the application specified the crypto completion event, it is likely >> that this event would have to be allocated and freed for every crypto >> operation. This seems wasteful. >> >> On 12 June 2015 at 10:22, Alexandru Badicioiu < >> alexandru.badicioiu@linaro.org> wrote: >> >>> Completion events are now implementation's responsibility. Application >>> does not provide any pool for completion events, only for output packet in >>> case the crypto HW engine is able to allocate output packets. >>> I think the whole completion event thing was misunderstood as a HW >>> provided thing. I'm not aware of (async) crypto engines which in addition >>> to input/output packets/buffers allocate a separate "thing" which we could >>> abstract as a completion event, at least in the current ODP implementations. >>> Initially the completion event was specified by the application and its >>> meaning was a schedulable/queue-able entity which should return the >>> aggregate output from and the context of a crypto operation. A plain buffer >>> and some accessor functions seemed to be fit for this purpose. The problem >>> was the allocation of this buffer so the input packet buffer was reused for >>> this, initially explicitly in the application and now hidden into the >>> implementation. This was/is regarded as a hack but I would like to >>> understand why is considered so. Moving a hack from application to >>> implementation still is a hack. >>> >>> Alex >>> >>> On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> >>> wrote: >>> >>>> >>>> >>>> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >>>> wrote: >>>> >>>>> The proposal I described during today's ARCH meeting is to add an >>>>> explicit event type to buffers so that when buffers (of whatever flavor) >>>>> are being used as events this field can be interrogated without needing to >>>>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>>>> this later today for review. >>>>> >>>> So the original "buffer" type field is there to information the ODP >>>> implementation while the new event type field is there to information the >>>> application? This allows e.g. the crypto code to allocate a buffer (with a >>>> suitable data size) from a buffer pool and pretend it is a crypto >>>> completion event so that the application treats it like a crypto completion >>>> event and not as a buffer. Also we don't need to create any explicit crypto >>>> completion event pool, they are all allocated from some suitable buffer >>>> pool. >>>> >>>> Did I understand this correctly? >>>> >>>> >>>> >>>>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl < >>>>> ola.liljedahl@linaro.org> wrote: >>>>> >>>>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>>>> wrote: >>>>>> >>>>>>> As Petri writes, the type of a crypto completion event must be >>>>>>> visible to the user (who is receiving the event and responsible for >>>>>>> figuring out what to do with those events). Different types of events may >>>>>>> be received from the same queue and require different action. >>>>>>> >>>>>>> I think it is useful for an implementation to allow buffer pool and >>>>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>>>> completion events and timeout events. The timer patch I did on Taras' >>>>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>>>> the buffer data so no need for a timeout-specific header. This design >>>>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>>>> is fixed and there might be no room for additional event-type specific >>>>>>> headers. >>>>>>> >>>>>> Just to clearify. An implementation doing the above would still have >>>>>> specific pools for crypto completion and timeout events. No need to >>>>>> dynamically change the type of an event. >>>>>> >>>>>> >>>>>>> >>>>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>>>> petri.savolainen@nokia.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On >>>>>>>> Behalf Of *ext Christophe Milard >>>>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>>>> *To:* Bill Fischofer >>>>>>>> *Cc:* LNG ODP Mailman List >>>>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>>>> buffer type hack for completions >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 10 June 2015 at 04:50, Bill Fischofer <bill.fischofer@linaro.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>> odp_crypto_operation() should not change underlying buffer types for >>>>>>>> completions as these are purely internal and not needed, and doing >>>>>>>> so is confusing and error-prome. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack >>>>>>>> to correct for another (hence the RFC). >>>>>>>> >>>>>>>> However, this patch looks incomplete to me: >>>>>>>> >>>>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>>>> between events. >>>>>>>> >>>>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>>>> really sure why...) >>>>>>>> >>>>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>>>> event_types.h if it is not meant to be used. >>>>>>>> >>>>>>>> ... and thanks for taking the time to look at it. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Crypto completion event type is certainly meant to be used. If >>>>>>>> implementation misuses the event type, the implementation needs to be >>>>>>>> fixed. When an application receives an event for crypto completion >>>>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Christophe >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>>>> --- >>>>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>>>> 3 files changed, 21 deletions(-) >>>>>>>> >>>>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> index e7b7568..2595b2d 100644 >>>>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>>>> size_t size); >>>>>>>> */ >>>>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>>>> >>>>>>>> -/* >>>>>>>> - * Buffer type set >>>>>>>> - * >>>>>>>> - * @param buf Buffer handle >>>>>>>> - * @param type New type value >>>>>>>> - * >>>>>>>> - */ >>>>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>>>> - >>>>>>>> #ifdef __cplusplus >>>>>>>> } >>>>>>>> #endif >>>>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>>>> b/platform/linux-generic/odp_buffer.c >>>>>>>> index 0803805..19b9dbd 100644 >>>>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>>>> return hdr->type; >>>>>>>> } >>>>>>>> >>>>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>>>> -{ >>>>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>>>> - >>>>>>>> - hdr->type = type; >>>>>>>> -} >>>>>>>> - >>>>>>>> >>>>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>>>> { >>>>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>>>> b/platform/linux-generic/odp_crypto.c >>>>>>>> index b16316c..e103066 100644 >>>>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>>>> *params, >>>>>>>> >>>>>>>> /* Linux generic will always use packet for >>>>>>>> completion event */ >>>>>>>> completion_event = >>>>>>>> odp_packet_to_event(params->out_pkt); >>>>>>>> - >>>>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>>>> >>>>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>>>> and send it*/ >>>>>>>> op_result = >>>>>>>> get_op_result_from_event(completion_event); >>>>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>>>> >>>>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>>>> { >>>>>>>> - /* This check not mandated by the API specification */ >>>>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> This check should be there in the reference implementation to catch >>>>>>>> bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -Petri >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> return (odp_crypto_compl_t)ev; >>>>>>>> } >>>>>>>> >>>>>>>> -- >>>>>>>> 2.1.0 >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> 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 >>>> >>>> >>> >> >
On 14 June 2015 at 20:07, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 14 June 2015 at 14:28, Alexandru Badicioiu < > alexandru.badicioiu@linaro.org> wrote: > >> I'm fine with the completion event being implementation responsibility, >> but I don't see much difference from being application responsibility, at >> least with current implementations, from the HW or performance point of >> view. Using the input packet or other thing explicitly in the application >> or hidden in the implementation is the same. As I mentioned, I'm not aware >> of crypto support for HW provided "completion events" - in fact HW sends >> events only by generating interrupts. >> > The HW you are working with perhaps but not necessarily all HW crypto > engines, current of future. We want to keep the door open for future HW > designs which are more streamlined and require less (perp-packet) overhead. > The overhead of the crypto (ciphering) processing itself is going down so > per-packet overheads may become bottlenecks (especially if they require > update of shared data structures, e.g. when allocating/freeing buffers). > > >> >> Async crypto operation is essentially an async I/O read operation - >> regular async I/O uses user buffers to return the results. >> Initial design followed this approach. Crypto operation results would fit >> more the packet context rather than the metadata. Not all packets are >> likely to be subject to crypto operations - for example ARP packets. >> odp_ipsec application already allocates and frees a context per packet >> input. >> > This does not seem optimal. I hope this is just the way it happens to be > done and not required design. I need to take a closer look. > I remember there was a discussion if free preserves or not the packet content (and/or metadata). and the agreement was that it doesn't. Perhaps this is the reason for per-packet context alloc/.free. > -- Ola > >> >> >> >> >> >> >> >> On 12 June 2015 at 17:11, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> >>> But are you OK with the architecture (API) Alex? >>> Isn't it good from an implementation point of view that it is the >>> implementation that is responsible for the crypto completion event? If this >>> is the input packet reused or a separate event is determined by the >>> implementation according to the design and requirements of the HW. >>> If the application specified the crypto completion event, it is likely >>> that this event would have to be allocated and freed for every crypto >>> operation. This seems wasteful. >>> >>> On 12 June 2015 at 10:22, Alexandru Badicioiu < >>> alexandru.badicioiu@linaro.org> wrote: >>> >>>> Completion events are now implementation's responsibility. Application >>>> does not provide any pool for completion events, only for output packet in >>>> case the crypto HW engine is able to allocate output packets. >>>> I think the whole completion event thing was misunderstood as a HW >>>> provided thing. I'm not aware of (async) crypto engines which in addition >>>> to input/output packets/buffers allocate a separate "thing" which we could >>>> abstract as a completion event, at least in the current ODP implementations. >>>> Initially the completion event was specified by the application and its >>>> meaning was a schedulable/queue-able entity which should return the >>>> aggregate output from and the context of a crypto operation. A plain buffer >>>> and some accessor functions seemed to be fit for this purpose. The problem >>>> was the allocation of this buffer so the input packet buffer was reused for >>>> this, initially explicitly in the application and now hidden into the >>>> implementation. This was/is regarded as a hack but I would like to >>>> understand why is considered so. Moving a hack from application to >>>> implementation still is a hack. >>>> >>>> Alex >>>> >>>> On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> >>>> wrote: >>>> >>>>> >>>>> >>>>> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >>>>> wrote: >>>>> >>>>>> The proposal I described during today's ARCH meeting is to add an >>>>>> explicit event type to buffers so that when buffers (of whatever flavor) >>>>>> are being used as events this field can be interrogated without needing to >>>>>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>>>>> this later today for review. >>>>>> >>>>> So the original "buffer" type field is there to information the ODP >>>>> implementation while the new event type field is there to information the >>>>> application? This allows e.g. the crypto code to allocate a buffer (with a >>>>> suitable data size) from a buffer pool and pretend it is a crypto >>>>> completion event so that the application treats it like a crypto completion >>>>> event and not as a buffer. Also we don't need to create any explicit crypto >>>>> completion event pool, they are all allocated from some suitable buffer >>>>> pool. >>>>> >>>>> Did I understand this correctly? >>>>> >>>>> >>>>> >>>>>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl < >>>>>> ola.liljedahl@linaro.org> wrote: >>>>>> >>>>>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>>>>> wrote: >>>>>>> >>>>>>>> As Petri writes, the type of a crypto completion event must be >>>>>>>> visible to the user (who is receiving the event and responsible for >>>>>>>> figuring out what to do with those events). Different types of events may >>>>>>>> be received from the same queue and require different action. >>>>>>>> >>>>>>>> I think it is useful for an implementation to allow buffer pool and >>>>>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>>>>> completion events and timeout events. The timer patch I did on Taras' >>>>>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>>>>> the buffer data so no need for a timeout-specific header. This design >>>>>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>>>>> is fixed and there might be no room for additional event-type specific >>>>>>>> headers. >>>>>>>> >>>>>>> Just to clearify. An implementation doing the above would still have >>>>>>> specific pools for crypto completion and timeout events. No need to >>>>>>> dynamically change the type of an event. >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>>>>> petri.savolainen@nokia.com> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On >>>>>>>>> Behalf Of *ext Christophe Milard >>>>>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>>>>> *To:* Bill Fischofer >>>>>>>>> *Cc:* LNG ODP Mailman List >>>>>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>>>>> buffer type hack for completions >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 10 June 2015 at 04:50, Bill Fischofer < >>>>>>>>> bill.fischofer@linaro.org> wrote: >>>>>>>>> >>>>>>>>> odp_crypto_operation() should not change underlying buffer types >>>>>>>>> for >>>>>>>>> completions as these are purely internal and not needed, and doing >>>>>>>>> so is confusing and error-prome. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack >>>>>>>>> to correct for another (hence the RFC). >>>>>>>>> >>>>>>>>> However, this patch looks incomplete to me: >>>>>>>>> >>>>>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>>>>> between events. >>>>>>>>> >>>>>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>>>>> really sure why...) >>>>>>>>> >>>>>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>>>>> event_types.h if it is not meant to be used. >>>>>>>>> >>>>>>>>> ... and thanks for taking the time to look at it. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Crypto completion event type is certainly meant to be used. If >>>>>>>>> implementation misuses the event type, the implementation needs to be >>>>>>>>> fixed. When an application receives an event for crypto completion >>>>>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Christophe >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>>>>> --- >>>>>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>>>>> 3 files changed, 21 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> index e7b7568..2595b2d 100644 >>>>>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>>>>> size_t size); >>>>>>>>> */ >>>>>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>>>>> >>>>>>>>> -/* >>>>>>>>> - * Buffer type set >>>>>>>>> - * >>>>>>>>> - * @param buf Buffer handle >>>>>>>>> - * @param type New type value >>>>>>>>> - * >>>>>>>>> - */ >>>>>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>>>>> - >>>>>>>>> #ifdef __cplusplus >>>>>>>>> } >>>>>>>>> #endif >>>>>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>>>>> b/platform/linux-generic/odp_buffer.c >>>>>>>>> index 0803805..19b9dbd 100644 >>>>>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>>>>> return hdr->type; >>>>>>>>> } >>>>>>>>> >>>>>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>>>>> -{ >>>>>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>>>>> - >>>>>>>>> - hdr->type = type; >>>>>>>>> -} >>>>>>>>> - >>>>>>>>> >>>>>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>>>>> { >>>>>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>>>>> b/platform/linux-generic/odp_crypto.c >>>>>>>>> index b16316c..e103066 100644 >>>>>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>>>>> *params, >>>>>>>>> >>>>>>>>> /* Linux generic will always use packet for >>>>>>>>> completion event */ >>>>>>>>> completion_event = >>>>>>>>> odp_packet_to_event(params->out_pkt); >>>>>>>>> - >>>>>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>>>>> >>>>>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>>>>> and send it*/ >>>>>>>>> op_result = >>>>>>>>> get_op_result_from_event(completion_event); >>>>>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>>>>> >>>>>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>>>>> { >>>>>>>>> - /* This check not mandated by the API specification */ >>>>>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> This check should be there in the reference implementation to >>>>>>>>> catch bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -Petri >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> return (odp_crypto_compl_t)ev; >>>>>>>>> } >>>>>>>>> >>>>>>>>> -- >>>>>>>>> 2.1.0 >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> 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 >>>>> >>>>> >>>> >>> >> >> On 12 June 2015 at 17:11, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> >>> But are you OK with the architecture (API) Alex? >>> Isn't it good from an implementation point of view that it is the >>> implementation that is responsible for the crypto completion event? If this >>> is the input packet reused or a separate event is determined by the >>> implementation according to the design and requirements of the HW. >>> If the application specified the crypto completion event, it is likely >>> that this event would have to be allocated and freed for every crypto >>> operation. This seems wasteful. >>> >>> On 12 June 2015 at 10:22, Alexandru Badicioiu < >>> alexandru.badicioiu@linaro.org> wrote: >>> >>>> Completion events are now implementation's responsibility. Application >>>> does not provide any pool for completion events, only for output packet in >>>> case the crypto HW engine is able to allocate output packets. >>>> I think the whole completion event thing was misunderstood as a HW >>>> provided thing. I'm not aware of (async) crypto engines which in addition >>>> to input/output packets/buffers allocate a separate "thing" which we could >>>> abstract as a completion event, at least in the current ODP implementations. >>>> Initially the completion event was specified by the application and its >>>> meaning was a schedulable/queue-able entity which should return the >>>> aggregate output from and the context of a crypto operation. A plain buffer >>>> and some accessor functions seemed to be fit for this purpose. The problem >>>> was the allocation of this buffer so the input packet buffer was reused for >>>> this, initially explicitly in the application and now hidden into the >>>> implementation. This was/is regarded as a hack but I would like to >>>> understand why is considered so. Moving a hack from application to >>>> implementation still is a hack. >>>> >>>> Alex >>>> >>>> On 12 June 2015 at 10:56, Ola Liljedahl <ola.liljedahl@linaro.org> >>>> wrote: >>>> >>>>> >>>>> >>>>> On 10 June 2015 at 18:32, Bill Fischofer <bill.fischofer@linaro.org> >>>>> wrote: >>>>> >>>>>> The proposal I described during today's ARCH meeting is to add an >>>>>> explicit event type to buffers so that when buffers (of whatever flavor) >>>>>> are being used as events this field can be interrogated without needing to >>>>>> overwrite the buffer type field. As I mentioned, I'll post a patch for >>>>>> this later today for review. >>>>>> >>>>> So the original "buffer" type field is there to information the ODP >>>>> implementation while the new event type field is there to information the >>>>> application? This allows e.g. the crypto code to allocate a buffer (with a >>>>> suitable data size) from a buffer pool and pretend it is a crypto >>>>> completion event so that the application treats it like a crypto completion >>>>> event and not as a buffer. Also we don't need to create any explicit crypto >>>>> completion event pool, they are all allocated from some suitable buffer >>>>> pool. >>>>> >>>>> Did I understand this correctly? >>>>> >>>>> >>>>> >>>>>> On Wed, Jun 10, 2015 at 7:15 AM, Ola Liljedahl < >>>>>> ola.liljedahl@linaro.org> wrote: >>>>>> >>>>>>> On 10 June 2015 at 13:57, Ola Liljedahl <ola.liljedahl@linaro.org> >>>>>>> wrote: >>>>>>> >>>>>>>> As Petri writes, the type of a crypto completion event must be >>>>>>>> visible to the user (who is receiving the event and responsible for >>>>>>>> figuring out what to do with those events). Different types of events may >>>>>>>> be received from the same queue and require different action. >>>>>>>> >>>>>>>> I think it is useful for an implementation to allow buffer pool and >>>>>>>> buffer event code to be used for other types of events as well, e.g. crypto >>>>>>>> completion events and timeout events. The timer patch I did on Taras' >>>>>>>> request based timeouts on buffers with the timeout-specific data stored in >>>>>>>> the buffer data so no need for a timeout-specific header. This design >>>>>>>> simplifies reuse on other platforms where the event (HW buffer) definition >>>>>>>> is fixed and there might be no room for additional event-type specific >>>>>>>> headers. >>>>>>>> >>>>>>> Just to clearify. An implementation doing the above would still have >>>>>>> specific pools for crypto completion and timeout events. No need to >>>>>>> dynamically change the type of an event. >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> On 10 June 2015 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) < >>>>>>>> petri.savolainen@nokia.com> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On >>>>>>>>> Behalf Of *ext Christophe Milard >>>>>>>>> *Sent:* Wednesday, June 10, 2015 10:18 AM >>>>>>>>> *To:* Bill Fischofer >>>>>>>>> *Cc:* LNG ODP Mailman List >>>>>>>>> *Subject:* Re: [lng-odp] [PATCH] linux-generic: crypto: eliminate >>>>>>>>> buffer type hack for completions >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 10 June 2015 at 04:50, Bill Fischofer < >>>>>>>>> bill.fischofer@linaro.org> wrote: >>>>>>>>> >>>>>>>>> odp_crypto_operation() should not change underlying buffer types >>>>>>>>> for >>>>>>>>> completions as these are purely internal and not needed, and doing >>>>>>>>> so is confusing and error-prome. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Yes, Bill, I would prefer your approach: my patch felt like a hack >>>>>>>>> to correct for another (hence the RFC). >>>>>>>>> >>>>>>>>> However, this patch looks incomplete to me: >>>>>>>>> >>>>>>>>> -example/ipsec/odp_ipsec.c is using the buffer type to distinguish >>>>>>>>> between events. >>>>>>>>> >>>>>>>>> -example/timer/odp_timer_test.c also uses it. (I am honestely not >>>>>>>>> really sure why...) >>>>>>>>> >>>>>>>>> -the definition of ODP_EVENT_CRYPTO_COMPL should be removed from >>>>>>>>> event_types.h if it is not meant to be used. >>>>>>>>> >>>>>>>>> ... and thanks for taking the time to look at it. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Crypto completion event type is certainly meant to be used. If >>>>>>>>> implementation misuses the event type, the implementation needs to be >>>>>>>>> fixed. When an application receives an event for crypto completion >>>>>>>>> odp_event_type() must return ODP_EVENT_CRYPTO_COMPL. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Christophe >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>>>>> --- >>>>>>>>> platform/linux-generic/include/odp_buffer_internal.h | 9 --------- >>>>>>>>> platform/linux-generic/odp_buffer.c | 7 ------- >>>>>>>>> platform/linux-generic/odp_crypto.c | 5 ----- >>>>>>>>> 3 files changed, 21 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> index e7b7568..2595b2d 100644 >>>>>>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h >>>>>>>>> @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, >>>>>>>>> size_t size); >>>>>>>>> */ >>>>>>>>> int _odp_buffer_type(odp_buffer_t buf); >>>>>>>>> >>>>>>>>> -/* >>>>>>>>> - * Buffer type set >>>>>>>>> - * >>>>>>>>> - * @param buf Buffer handle >>>>>>>>> - * @param type New type value >>>>>>>>> - * >>>>>>>>> - */ >>>>>>>>> - void _odp_buffer_type_set(odp_buffer_t buf, int type); >>>>>>>>> - >>>>>>>>> #ifdef __cplusplus >>>>>>>>> } >>>>>>>>> #endif >>>>>>>>> diff --git a/platform/linux-generic/odp_buffer.c >>>>>>>>> b/platform/linux-generic/odp_buffer.c >>>>>>>>> index 0803805..19b9dbd 100644 >>>>>>>>> --- a/platform/linux-generic/odp_buffer.c >>>>>>>>> +++ b/platform/linux-generic/odp_buffer.c >>>>>>>>> @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) >>>>>>>>> return hdr->type; >>>>>>>>> } >>>>>>>>> >>>>>>>>> -void _odp_buffer_type_set(odp_buffer_t buf, int type) >>>>>>>>> -{ >>>>>>>>> - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >>>>>>>>> - >>>>>>>>> - hdr->type = type; >>>>>>>>> -} >>>>>>>>> - >>>>>>>>> >>>>>>>>> int odp_buffer_is_valid(odp_buffer_t buf) >>>>>>>>> { >>>>>>>>> diff --git a/platform/linux-generic/odp_crypto.c >>>>>>>>> b/platform/linux-generic/odp_crypto.c >>>>>>>>> index b16316c..e103066 100644 >>>>>>>>> --- a/platform/linux-generic/odp_crypto.c >>>>>>>>> +++ b/platform/linux-generic/odp_crypto.c >>>>>>>>> @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t >>>>>>>>> *params, >>>>>>>>> >>>>>>>>> /* Linux generic will always use packet for >>>>>>>>> completion event */ >>>>>>>>> completion_event = >>>>>>>>> odp_packet_to_event(params->out_pkt); >>>>>>>>> - >>>>>>>>> _odp_buffer_type_set(odp_buffer_from_event(completion_event), >>>>>>>>> - ODP_EVENT_CRYPTO_COMPL); >>>>>>>>> >>>>>>>>> /* Asynchronous, build result (no HW so no errors) >>>>>>>>> and send it*/ >>>>>>>>> op_result = >>>>>>>>> get_op_result_from_event(completion_event); >>>>>>>>> @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, >>>>>>>>> odp_bool_t use_entropy ODP_UNUSED) >>>>>>>>> >>>>>>>>> odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) >>>>>>>>> { >>>>>>>>> - /* This check not mandated by the API specification */ >>>>>>>>> - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) >>>>>>>>> - ODP_ABORT("Event not a crypto completion"); >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> This check should be there in the reference implementation to >>>>>>>>> catch bugs. Event type must be CRYPTO_COMPL when entering this function. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -Petri >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> return (odp_crypto_compl_t)ev; >>>>>>>>> } >>>>>>>>> >>>>>>>>> -- >>>>>>>>> 2.1.0 >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> 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 >>>>> >>>>> >>>> >>> >> >
diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h index e7b7568..2595b2d 100644 --- a/platform/linux-generic/include/odp_buffer_internal.h +++ b/platform/linux-generic/include/odp_buffer_internal.h @@ -162,15 +162,6 @@ odp_buffer_t buffer_alloc(odp_pool_t pool, size_t size); */ int _odp_buffer_type(odp_buffer_t buf); -/* - * Buffer type set - * - * @param buf Buffer handle - * @param type New type value - * - */ - void _odp_buffer_type_set(odp_buffer_t buf, int type); - #ifdef __cplusplus } #endif diff --git a/platform/linux-generic/odp_buffer.c b/platform/linux-generic/odp_buffer.c index 0803805..19b9dbd 100644 --- a/platform/linux-generic/odp_buffer.c +++ b/platform/linux-generic/odp_buffer.c @@ -47,13 +47,6 @@ int _odp_buffer_type(odp_buffer_t buf) return hdr->type; } -void _odp_buffer_type_set(odp_buffer_t buf, int type) -{ - odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); - - hdr->type = type; -} - int odp_buffer_is_valid(odp_buffer_t buf) { diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c index b16316c..e103066 100644 --- a/platform/linux-generic/odp_crypto.c +++ b/platform/linux-generic/odp_crypto.c @@ -424,8 +424,6 @@ odp_crypto_operation(odp_crypto_op_params_t *params, /* Linux generic will always use packet for completion event */ completion_event = odp_packet_to_event(params->out_pkt); - _odp_buffer_type_set(odp_buffer_from_event(completion_event), - ODP_EVENT_CRYPTO_COMPL); /* Asynchronous, build result (no HW so no errors) and send it*/ op_result = get_op_result_from_event(completion_event); @@ -510,9 +508,6 @@ odp_random_data(uint8_t *buf, int32_t len, odp_bool_t use_entropy ODP_UNUSED) odp_crypto_compl_t odp_crypto_compl_from_event(odp_event_t ev) { - /* This check not mandated by the API specification */ - if (odp_event_type(ev) != ODP_EVENT_CRYPTO_COMPL) - ODP_ABORT("Event not a crypto completion"); return (odp_crypto_compl_t)ev; }
odp_crypto_operation() should not change underlying buffer types for completions as these are purely internal and not needed, and doing so is confusing and error-prome. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/include/odp_buffer_internal.h | 9 --------- platform/linux-generic/odp_buffer.c | 7 ------- platform/linux-generic/odp_crypto.c | 5 ----- 3 files changed, 21 deletions(-)