| Message ID | 20170616104940.20166-2-petri.savolainen@linaro.org |
|---|---|
| State | Accepted |
| Commit | 483de1f3cd4a7f0197e17c3c2387696a742274cd |
| Headers | show |
| Series | IPSEC packet event | expand |
On Fri, Jun 16, 2017 at 5:49 AM, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Event subtype gives more detailed information about the event. > Two subtypes (basic and IPSEC packet) are introduced initially. > Later on, other packet producing APIs (crypto, comp, etc) may > also produce packet events with additional subtypes. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > include/odp/api/spec/event.h | 80 ++++++++++++++++++++-- > include/odp/arch/default/api/abi/event.h | 9 ++- > .../include/odp/api/plat/event_types.h | 8 ++- > 3 files changed, 89 insertions(+), 8 deletions(-) > > diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h > index f22efce5..2ad3ce84 100644 > --- a/include/odp/api/spec/event.h > +++ b/include/odp/api/spec/event.h > @@ -37,21 +37,91 @@ extern "C" { > > /** > * @typedef odp_event_type_t > - * ODP event types: > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, ODP_EVENT_IPSEC_STATUS > + * Event type > + * > + * Event type specifies purpose and general format of an event. It can be > + * checked with odp_event_type() or odp_event_types(). Each event type has > + * functions (e.g. odp_buffer_from_event()) to convert between the generic event > + * handle (odp_event_t) and the type specific handle (e.g. odp_buffer_t). > + * Results are undefined, if conversion function of a wrong event type is used. > + * Application cannot change event type by chaining conversion functions. > + * > + * List of event types: > + * - ODP_EVENT_BUFFER > + * - Buffer event (odp_buffer_t) for simple data storage and message passing > + * - ODP_EVENT_PACKET > + * - Packet event (odp_packet_t) containing packet data and plenty of Why "plenty of"? "containing packet data and associated metadata" seems sufficient. > + * packet processing related metadata > + * - ODP_EVENT_TIMEOUT > + * - Timeout event (odp_timeout_t) from a timer > + * - ODP_EVENT_CRYPTO_COMPL > + * - Crypto completion event (odp_crypto_compl_t) > + * - ODP_EVENT_IPSEC_STATUS > + * - IPSEC status update event (odp_ipsec_status_t) > */ > > /** > - * Get event type > + * @typedef odp_event_subtype_t > + * Event subtype > * > - * @param event Event handle > + * Event subtype expands event type specification by providing more detailed > + * purpose and format of an event. It can be checked with odp_event_subtype() or > + * odp_event_types(). Each event subtype may define specific functions > + * (e.g. odp_ipsec_packet_from_event()) to convert between the generic event > + * handle (odp_event_t) and event type specific handle (e.g. odp_packet_t). When > + * subtype is known, these subtype specific functions should be preferred over > + * the event type general function (e.g. odp_packet_from_event()). Results are > + * undefined, if conversion function of a wrong event subtype is used. > + * Application cannot change event subtype by chaining conversion functions. > + * > + * List of event subtypes: > + * - ODP_EVENT_PACKET_BASIC > + * - Packet event (odp_packet_t) with basic packet metadata > + * - ODP_EVENT_PACKET_IPSEC > + * - Packet event (odp_packet_t) generated as a result of an IPsec > + * operation. It contains IPSEC specific metadata in addition to the basic > + * packet metadata. > + * - ODP_EVENT_NO_SUBTYPE > + * - An event type does not have any subtypes defined > + */ > + > +/** > + * Event type of an event > + * > + * Event type specifies purpose and general format of an event. > + * > + * @param event Event handle > * > * @return Event type > */ > odp_event_type_t odp_event_type(odp_event_t event); > > /** > + * Event subtype of an event > + * > + * Event subtype expands event type specification by providing more detailed > + * purpose and format of an event. > + * > + * @param event Event handle > + * > + * @return Event subtype > + */ > +odp_event_subtype_t odp_event_subtype(odp_event_t event); > + > +/** > + * Event type and subtype of an event > + * > + * Returns event type and outputs event subtype. > + * > + * @param event Event handle > + * @param[out] subtype Pointer to event subtype for output > + * > + * @return Event type > + */ > +odp_event_type_t odp_event_types(odp_event_t event, > + odp_event_subtype_t *subtype); > + > +/** > * Get printable value for an odp_event_t > * > * @param hdl odp_event_t handle to be printed > diff --git a/include/odp/arch/default/api/abi/event.h b/include/odp/arch/default/api/abi/event.h > index 87220d63..ab3c0f75 100644 > --- a/include/odp/arch/default/api/abi/event.h > +++ b/include/odp/arch/default/api/abi/event.h > @@ -29,10 +29,15 @@ typedef enum odp_event_type_t { > ODP_EVENT_PACKET = 2, > ODP_EVENT_TIMEOUT = 3, > ODP_EVENT_CRYPTO_COMPL = 4, > - ODP_EVENT_IPSEC_RESULT = 5, > - ODP_EVENT_IPSEC_STATUS = 6 > + ODP_EVENT_IPSEC_STATUS = 5 > } odp_event_type_t; > > +typedef enum odp_event_subtype_t { > + ODP_EVENT_NO_SUBTYPE = 0, > + ODP_EVENT_PACKET_BASIC = 1, > + ODP_EVENT_PACKET_IPSEC = 2 > +} odp_event_subtype_t; > + > /** > * @} > */ > diff --git a/platform/linux-generic/include/odp/api/plat/event_types.h b/platform/linux-generic/include/odp/api/plat/event_types.h > index 0f517834..5b3a07e3 100644 > --- a/platform/linux-generic/include/odp/api/plat/event_types.h > +++ b/platform/linux-generic/include/odp/api/plat/event_types.h > @@ -39,9 +39,15 @@ typedef enum odp_event_type_t { > ODP_EVENT_PACKET = 2, > ODP_EVENT_TIMEOUT = 3, > ODP_EVENT_CRYPTO_COMPL = 4, > - ODP_EVENT_IPSEC_RESULT = 5 > + ODP_EVENT_IPSEC_STATUS = 5 > } odp_event_type_t; > > +typedef enum odp_event_subtype_t { > + ODP_EVENT_NO_SUBTYPE = 0, > + ODP_EVENT_PACKET_BASIC = 1, > + ODP_EVENT_PACKET_IPSEC = 2 > +} odp_event_subtype_t; > + > /** > * @} > */ > -- > 2.13.0 >
> > diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h > > index f22efce5..2ad3ce84 100644 > > --- a/include/odp/api/spec/event.h > > +++ b/include/odp/api/spec/event.h > > @@ -37,21 +37,91 @@ extern "C" { > > > > /** > > * @typedef odp_event_type_t > > - * ODP event types: > > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, > > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, > ODP_EVENT_IPSEC_STATUS > > + * Event type > > + * > > + * Event type specifies purpose and general format of an event. It can > be > > + * checked with odp_event_type() or odp_event_types(). Each event type > has > > + * functions (e.g. odp_buffer_from_event()) to convert between the > generic event > > + * handle (odp_event_t) and the type specific handle (e.g. > odp_buffer_t). > > + * Results are undefined, if conversion function of a wrong event type > is used. > > + * Application cannot change event type by chaining conversion > functions. > > + * > > + * List of event types: > > + * - ODP_EVENT_BUFFER > > + * - Buffer event (odp_buffer_t) for simple data storage and > message passing > > + * - ODP_EVENT_PACKET > > + * - Packet event (odp_packet_t) containing packet data and plenty > of > > Why "plenty of"? "containing packet data and associated metadata" > seems sufficient. It highlights that packets should be used only for packets, not as random buffer space (buffers are simple vs. packets are complex). BTW. It would have been nice to get all review comments in one go. I did sent v3 as there were no more comments. I cannot do v4 before our lab is back online from a maintenance break... Hopefully v4 is not necessary. -Petri
On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: >> > diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h >> > index f22efce5..2ad3ce84 100644 >> > --- a/include/odp/api/spec/event.h >> > +++ b/include/odp/api/spec/event.h >> > @@ -37,21 +37,91 @@ extern "C" { >> > >> > /** >> > * @typedef odp_event_type_t >> > - * ODP event types: >> > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, >> > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, >> ODP_EVENT_IPSEC_STATUS >> > + * Event type >> > + * >> > + * Event type specifies purpose and general format of an event. It can >> be >> > + * checked with odp_event_type() or odp_event_types(). Each event type >> has >> > + * functions (e.g. odp_buffer_from_event()) to convert between the >> generic event >> > + * handle (odp_event_t) and the type specific handle (e.g. >> odp_buffer_t). >> > + * Results are undefined, if conversion function of a wrong event type >> is used. >> > + * Application cannot change event type by chaining conversion >> functions. >> > + * >> > + * List of event types: >> > + * - ODP_EVENT_BUFFER >> > + * - Buffer event (odp_buffer_t) for simple data storage and >> message passing >> > + * - ODP_EVENT_PACKET >> > + * - Packet event (odp_packet_t) containing packet data and plenty >> of >> >> Why "plenty of"? "containing packet data and associated metadata" >> seems sufficient. > > It highlights that packets should be used only for packets, not as random buffer space (buffers are simple vs. packets are complex). Ok, it just looked a bit odd. Not a biggie. > > BTW. It would have been nice to get all review comments in one go. I did sent v3 as there were no more comments. I cannot do v4 before our lab is back online from a maintenance break... Hopefully v4 is not necessary. Still working on it. This series needs Bala and Nikhil's reviews as a check against their HW. But so far this looks very reasonable. > > -Petri
On Fri, Jun 16, 2017 at 6:42 PM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia.com> wrote: >>> > diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h >>> > index f22efce5..2ad3ce84 100644 >>> > --- a/include/odp/api/spec/event.h >>> > +++ b/include/odp/api/spec/event.h >>> > @@ -37,21 +37,91 @@ extern "C" { >>> > >>> > /** >>> > * @typedef odp_event_type_t >>> > - * ODP event types: >>> > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, >>> > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, >>> ODP_EVENT_IPSEC_STATUS >>> > + * Event type >>> > + * >>> > + * Event type specifies purpose and general format of an event. It can >>> be >>> > + * checked with odp_event_type() or odp_event_types(). Each event type >>> has >>> > + * functions (e.g. odp_buffer_from_event()) to convert between the >>> generic event >>> > + * handle (odp_event_t) and the type specific handle (e.g. >>> odp_buffer_t). >>> > + * Results are undefined, if conversion function of a wrong event type >>> is used. >>> > + * Application cannot change event type by chaining conversion >>> functions. >>> > + * >>> > + * List of event types: >>> > + * - ODP_EVENT_BUFFER >>> > + * - Buffer event (odp_buffer_t) for simple data storage and >>> message passing >>> > + * - ODP_EVENT_PACKET >>> > + * - Packet event (odp_packet_t) containing packet data and plenty >>> of >>> >>> Why "plenty of"? "containing packet data and associated metadata" >>> seems sufficient. >> >> It highlights that packets should be used only for packets, not as random buffer space (buffers are simple vs. packets are complex). > > Ok, it just looked a bit odd. Not a biggie. > >> >> BTW. It would have been nice to get all review comments in one go. I did sent v3 as there were no more comments. I cannot do v4 before our lab is back online from a maintenance break... Hopefully v4 is not necessary. > > Still working on it. This series needs Bala and Nikhil's reviews as a > check against their HW. But so far this looks very reasonable. > >> >> -Petri So, how does this affect event notification mechanism now? Currently we had ODP_EVENT_COMP_COMPL set on a packet to mark compression/decompression complete status on requested input. So that still remain valid Or we need to introduce it like: event ODP_EVENT_PACKET subtype ODP_EVENT_PACKET_COMP_COMPL? Thanks Shally
On Fri, Jun 16, 2017 at 8:33 AM, shally verma <shallyvermacavium@gmail.com> wrote: > On Fri, Jun 16, 2017 at 6:42 PM, Bill Fischofer > <bill.fischofer@linaro.org> wrote: >> On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo) >> <petri.savolainen@nokia.com> wrote: >>>> > diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h >>>> > index f22efce5..2ad3ce84 100644 >>>> > --- a/include/odp/api/spec/event.h >>>> > +++ b/include/odp/api/spec/event.h >>>> > @@ -37,21 +37,91 @@ extern "C" { >>>> > >>>> > /** >>>> > * @typedef odp_event_type_t >>>> > - * ODP event types: >>>> > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, >>>> > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, >>>> ODP_EVENT_IPSEC_STATUS >>>> > + * Event type >>>> > + * >>>> > + * Event type specifies purpose and general format of an event. It can >>>> be >>>> > + * checked with odp_event_type() or odp_event_types(). Each event type >>>> has >>>> > + * functions (e.g. odp_buffer_from_event()) to convert between the >>>> generic event >>>> > + * handle (odp_event_t) and the type specific handle (e.g. >>>> odp_buffer_t). >>>> > + * Results are undefined, if conversion function of a wrong event type >>>> is used. >>>> > + * Application cannot change event type by chaining conversion >>>> functions. >>>> > + * >>>> > + * List of event types: >>>> > + * - ODP_EVENT_BUFFER >>>> > + * - Buffer event (odp_buffer_t) for simple data storage and >>>> message passing >>>> > + * - ODP_EVENT_PACKET >>>> > + * - Packet event (odp_packet_t) containing packet data and plenty >>>> of >>>> >>>> Why "plenty of"? "containing packet data and associated metadata" >>>> seems sufficient. >>> >>> It highlights that packets should be used only for packets, not as random buffer space (buffers are simple vs. packets are complex). >> >> Ok, it just looked a bit odd. Not a biggie. >> >>> >>> BTW. It would have been nice to get all review comments in one go. I did sent v3 as there were no more comments. I cannot do v4 before our lab is back online from a maintenance break... Hopefully v4 is not necessary. >> >> Still working on it. This series needs Bala and Nikhil's reviews as a >> check against their HW. But so far this looks very reasonable. >> >>> >>> -Petri > > So, how does this affect event notification mechanism now? > Currently we had ODP_EVENT_COMP_COMPL set on a packet to mark > compression/decompression complete status on requested input. So that > still remain valid Or we need to introduce it like: > > event ODP_EVENT_PACKET > subtype ODP_EVENT_PACKET_COMP_COMPL? For consistency with the subtype pattern defined here I'd guess ODP_EVENT_PACKET_COMP would be the name to use. > > Thanks > Shally
On Fri, Jun 16, 2017 at 7:15 PM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On Fri, Jun 16, 2017 at 8:33 AM, shally verma > <shallyvermacavium@gmail.com> wrote: >> On Fri, Jun 16, 2017 at 6:42 PM, Bill Fischofer >> <bill.fischofer@linaro.org> wrote: >>> On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo) >>> <petri.savolainen@nokia.com> wrote: >>>>> > diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h >>>>> > index f22efce5..2ad3ce84 100644 >>>>> > --- a/include/odp/api/spec/event.h >>>>> > +++ b/include/odp/api/spec/event.h >>>>> > @@ -37,21 +37,91 @@ extern "C" { >>>>> > >>>>> > /** >>>>> > * @typedef odp_event_type_t >>>>> > - * ODP event types: >>>>> > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, >>>>> > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, >>>>> ODP_EVENT_IPSEC_STATUS >>>>> > + * Event type >>>>> > + * >>>>> > + * Event type specifies purpose and general format of an event. It can >>>>> be >>>>> > + * checked with odp_event_type() or odp_event_types(). Each event type >>>>> has >>>>> > + * functions (e.g. odp_buffer_from_event()) to convert between the >>>>> generic event >>>>> > + * handle (odp_event_t) and the type specific handle (e.g. >>>>> odp_buffer_t). >>>>> > + * Results are undefined, if conversion function of a wrong event type >>>>> is used. >>>>> > + * Application cannot change event type by chaining conversion >>>>> functions. >>>>> > + * >>>>> > + * List of event types: >>>>> > + * - ODP_EVENT_BUFFER >>>>> > + * - Buffer event (odp_buffer_t) for simple data storage and >>>>> message passing >>>>> > + * - ODP_EVENT_PACKET >>>>> > + * - Packet event (odp_packet_t) containing packet data and plenty >>>>> of >>>>> >>>>> Why "plenty of"? "containing packet data and associated metadata" >>>>> seems sufficient. >>>> >>>> It highlights that packets should be used only for packets, not as random buffer space (buffers are simple vs. packets are complex). >>> >>> Ok, it just looked a bit odd. Not a biggie. >>> >>>> >>>> BTW. It would have been nice to get all review comments in one go. I did sent v3 as there were no more comments. I cannot do v4 before our lab is back online from a maintenance break... Hopefully v4 is not necessary. >>> >>> Still working on it. This series needs Bala and Nikhil's reviews as a >>> check against their HW. But so far this looks very reasonable. >>> >>>> >>>> -Petri >> >> So, how does this affect event notification mechanism now? >> Currently we had ODP_EVENT_COMP_COMPL set on a packet to mark >> compression/decompression complete status on requested input. So that >> still remain valid Or we need to introduce it like: >> >> event ODP_EVENT_PACKET >> subtype ODP_EVENT_PACKET_COMP_COMPL? > > For consistency with the subtype pattern defined here I'd guess > ODP_EVENT_PACKET_COMP would be the name to use. > >> Ok. but it still leave question open for me. Should existing Compression/Crypto completion event is going to be there or do we need to change as per current proposal? Or current proposal is for newer event notifications. When an compression/encryption event should be considered as sub-event to packet? What event should be if user send a buffer for compression/decompression than packet? May be I am missing broader perspective to this proposal. Also, I have bit of confusion with event type enums placement. ODO_EVENT_PACKET/ODP_EVENT_CRYPTO_COMPL .. are these standard event types and public? Because I see their definition inside platform/linux-generic/include/odp/api/plat/event_types.h instead of standard spec file at include/odp/api/spec/event.h giving me an impression they are platform specific enum defines. So far I was under impression because they are platform specific so we are introducing conversion APIs like odp_xxx_event_from_event() so that user don't need to know specific event enumeration. As example (referring to crypto as base design) , we have: odp_comp_compl_t odp_comp_compl_from_event(odp_event_t); odp_comp_compl_result(odp_comp_compl_t comp_handle, odp_comp_op_result_t *result); Am I missing any base assumption here? >> Thanks >> Shally
> -----Original Message----- > From: shally verma [mailto:shallyvermacavium@gmail.com] > Sent: Tuesday, June 20, 2017 8:55 AM > To: Bill Fischofer <bill.fischofer@linaro.org> > Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com>; > lng-odp-forward <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [API-NEXT PATCH v3 1/3] api: event: add subtype to > expand event type > > On Fri, Jun 16, 2017 at 7:15 PM, Bill Fischofer > <bill.fischofer@linaro.org> wrote: > > On Fri, Jun 16, 2017 at 8:33 AM, shally verma > > <shallyvermacavium@gmail.com> wrote: > >> On Fri, Jun 16, 2017 at 6:42 PM, Bill Fischofer > >> <bill.fischofer@linaro.org> wrote: > >>> On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo) > >>> <petri.savolainen@nokia.com> wrote: > >>>>> > diff --git a/include/odp/api/spec/event.h > b/include/odp/api/spec/event.h > >>>>> > index f22efce5..2ad3ce84 100644 > >>>>> > --- a/include/odp/api/spec/event.h > >>>>> > +++ b/include/odp/api/spec/event.h > >>>>> > @@ -37,21 +37,91 @@ extern "C" { > >>>>> > > >>>>> > /** > >>>>> > * @typedef odp_event_type_t > >>>>> > - * ODP event types: > >>>>> > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, > >>>>> > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, > >>>>> ODP_EVENT_IPSEC_STATUS > >>>>> > + * Event type > >>>>> > + * > >>>>> > + * Event type specifies purpose and general format of an event. > It can > >>>>> be > >>>>> > + * checked with odp_event_type() or odp_event_types(). Each event > type > >>>>> has > >>>>> > + * functions (e.g. odp_buffer_from_event()) to convert between > the > >>>>> generic event > >>>>> > + * handle (odp_event_t) and the type specific handle (e.g. > >>>>> odp_buffer_t). > >>>>> > + * Results are undefined, if conversion function of a wrong event > type > >>>>> is used. > >>>>> > + * Application cannot change event type by chaining conversion > >>>>> functions. > >>>>> > + * > >>>>> > + * List of event types: > >>>>> > + * - ODP_EVENT_BUFFER > >>>>> > + * - Buffer event (odp_buffer_t) for simple data storage and > >>>>> message passing > >>>>> > + * - ODP_EVENT_PACKET > >>>>> > + * - Packet event (odp_packet_t) containing packet data and > plenty > >>>>> of > >>>>> > >>>>> Why "plenty of"? "containing packet data and associated metadata" > >>>>> seems sufficient. > >>>> > >>>> It highlights that packets should be used only for packets, not as > random buffer space (buffers are simple vs. packets are complex). > >>> > >>> Ok, it just looked a bit odd. Not a biggie. > >>> > >>>> > >>>> BTW. It would have been nice to get all review comments in one go. I > did sent v3 as there were no more comments. I cannot do v4 before our lab > is back online from a maintenance break... Hopefully v4 is not necessary. > >>> > >>> Still working on it. This series needs Bala and Nikhil's reviews as a > >>> check against their HW. But so far this looks very reasonable. > >>> > >>>> > >>>> -Petri > >> > >> So, how does this affect event notification mechanism now? > >> Currently we had ODP_EVENT_COMP_COMPL set on a packet to mark > >> compression/decompression complete status on requested input. So that > >> still remain valid Or we need to introduce it like: > >> > >> event ODP_EVENT_PACKET > >> subtype ODP_EVENT_PACKET_COMP_COMPL? > > > > For consistency with the subtype pattern defined here I'd guess > > ODP_EVENT_PACKET_COMP would be the name to use. > > > >> > Ok. but it still leave question open for me. Should existing > Compression/Crypto completion event is going to be there or do we need > to change as per current proposal? Or current proposal is for newer > event notifications. When an compression/encryption event should be > considered as sub-event to packet? What event should be if user send a > buffer for compression/decompression than packet? May be I am missing > broader perspective to this proposal. If/when this patch is merged, new work (like comp) should be done against it. So, comp packet would be another subtype. Also, crypto completion APIs will be changed to this (with deprecation). By buffer, do you mean odp_buffer_t event, or void *ptr ? Maybe ptr/len is more relevant addition than odp_buffer_t. Sync version of that (ptr/len) is easy, async version would need another event type. > > Also, I have bit of confusion with event type enums placement. > ODO_EVENT_PACKET/ODP_EVENT_CRYPTO_COMPL .. are these standard event > types and public? Because I see their definition inside > platform/linux-generic/include/odp/api/plat/event_types.h instead of > standard spec file at include/odp/api/spec/event.h giving me an > impression they are platform specific enum defines. API spec defines the enum names, but not the values. Application must not rely on the values. Depending on implementation, values may be HW defined. /** * @typedef odp_event_type_t * ODP event types: * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, > > So far I was under impression because they are platform specific so we > are introducing conversion APIs like > odp_xxx_event_from_event() so that user don't need to know specific > event enumeration. As example (referring to crypto as base design) , > we have: > > odp_comp_compl_t odp_comp_compl_from_event(odp_event_t); > odp_comp_compl_result(odp_comp_compl_t comp_handle, > odp_comp_op_result_t *result); > > Am I missing any base assumption here? odp_event_t is the base class of actual events (e.g. odp_packet_t). Odp_event_t offers minimal operations such as check types or free event without checking the type. Odp_event_t handle needs to be converted to the type specific handle, before application is able to access more features/metadata. The result call is one example of getting metadata from an event. With the subtype specification these would turn into: odp_packet_t odp_comp_packet_from_event(odp_event_t ev); int odp_comp_result(odp_comp_result_t *result, odp_packet_t pkt); ... or int odp_comp_pkt_result(odp_comp_pkt_result_t *result, odp_packet_t pkt); int odp_comp_xxx_result(odp_comp_xxx_result_t *result, odp_comp_xxx_t xxx); ... if you speculate with another non-packet result type. -Petri
On Tue, Jun 20, 2017 at 3:12 PM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: > > >> -----Original Message----- >> From: shally verma [mailto:shallyvermacavium@gmail.com] >> Sent: Tuesday, June 20, 2017 8:55 AM >> To: Bill Fischofer <bill.fischofer@linaro.org> >> Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com>; >> lng-odp-forward <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [API-NEXT PATCH v3 1/3] api: event: add subtype to >> expand event type >> >> On Fri, Jun 16, 2017 at 7:15 PM, Bill Fischofer >> <bill.fischofer@linaro.org> wrote: >> > On Fri, Jun 16, 2017 at 8:33 AM, shally verma >> > <shallyvermacavium@gmail.com> wrote: >> >> On Fri, Jun 16, 2017 at 6:42 PM, Bill Fischofer >> >> <bill.fischofer@linaro.org> wrote: >> >>> On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo) >> >>> <petri.savolainen@nokia.com> wrote: >> >>>>> > diff --git a/include/odp/api/spec/event.h >> b/include/odp/api/spec/event.h >> >>>>> > index f22efce5..2ad3ce84 100644 >> >>>>> > --- a/include/odp/api/spec/event.h >> >>>>> > +++ b/include/odp/api/spec/event.h >> >>>>> > @@ -37,21 +37,91 @@ extern "C" { >> >>>>> > >> >>>>> > /** >> >>>>> > * @typedef odp_event_type_t >> >>>>> > - * ODP event types: >> >>>>> > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, >> >>>>> > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, >> >>>>> ODP_EVENT_IPSEC_STATUS >> >>>>> > + * Event type >> >>>>> > + * >> >>>>> > + * Event type specifies purpose and general format of an event. >> It can >> >>>>> be >> >>>>> > + * checked with odp_event_type() or odp_event_types(). Each event >> type >> >>>>> has >> >>>>> > + * functions (e.g. odp_buffer_from_event()) to convert between >> the >> >>>>> generic event >> >>>>> > + * handle (odp_event_t) and the type specific handle (e.g. >> >>>>> odp_buffer_t). >> >>>>> > + * Results are undefined, if conversion function of a wrong event >> type >> >>>>> is used. >> >>>>> > + * Application cannot change event type by chaining conversion >> >>>>> functions. >> >>>>> > + * >> >>>>> > + * List of event types: >> >>>>> > + * - ODP_EVENT_BUFFER >> >>>>> > + * - Buffer event (odp_buffer_t) for simple data storage and >> >>>>> message passing >> >>>>> > + * - ODP_EVENT_PACKET >> >>>>> > + * - Packet event (odp_packet_t) containing packet data and >> plenty >> >>>>> of >> >>>>> >> >>>>> Why "plenty of"? "containing packet data and associated metadata" >> >>>>> seems sufficient. >> >>>> >> >>>> It highlights that packets should be used only for packets, not as >> random buffer space (buffers are simple vs. packets are complex). >> >>> >> >>> Ok, it just looked a bit odd. Not a biggie. >> >>> >> >>>> >> >>>> BTW. It would have been nice to get all review comments in one go. I >> did sent v3 as there were no more comments. I cannot do v4 before our lab >> is back online from a maintenance break... Hopefully v4 is not necessary. >> >>> >> >>> Still working on it. This series needs Bala and Nikhil's reviews as a >> >>> check against their HW. But so far this looks very reasonable. >> >>> >> >>>> >> >>>> -Petri >> >> >> >> So, how does this affect event notification mechanism now? >> >> Currently we had ODP_EVENT_COMP_COMPL set on a packet to mark >> >> compression/decompression complete status on requested input. So that >> >> still remain valid Or we need to introduce it like: >> >> >> >> event ODP_EVENT_PACKET >> >> subtype ODP_EVENT_PACKET_COMP_COMPL? >> > >> > For consistency with the subtype pattern defined here I'd guess >> > ODP_EVENT_PACKET_COMP would be the name to use. >> > >> >> >> Ok. but it still leave question open for me. Should existing >> Compression/Crypto completion event is going to be there or do we need >> to change as per current proposal? Or current proposal is for newer >> event notifications. When an compression/encryption event should be >> considered as sub-event to packet? What event should be if user send a >> buffer for compression/decompression than packet? May be I am missing >> broader perspective to this proposal. > > If/when this patch is merged, new work (like comp) should be done against it. So, comp packet would be another subtype. Also, crypto completion APIs will be changed to this (with deprecation). > Ok. > By buffer, do you mean odp_buffer_t event, or void *ptr ? Maybe ptr/len is more relevant addition than odp_buffer_t. Though not fully decide yet on it but most likely it will ptr/len struct. > Sync version of that (ptr/len) is easy, async version would need another event type. if so, then I probably retain ODP_EVENT_COMP_COMPL for event notification on buffer data. >> >> Also, I have bit of confusion with event type enums placement. >> ODO_EVENT_PACKET/ODP_EVENT_CRYPTO_COMPL .. are these standard event >> types and public? Because I see their definition inside >> platform/linux-generic/include/odp/api/plat/event_types.h instead of >> standard spec file at include/odp/api/spec/event.h giving me an >> impression they are platform specific enum defines. > > API spec defines the enum names, but not the values. Application must not rely on the values. Depending on implementation, values may be HW defined. > Ok. > /** > * @typedef odp_event_type_t > * ODP event types: > * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, > > > >> >> So far I was under impression because they are platform specific so we >> are introducing conversion APIs like >> odp_xxx_event_from_event() so that user don't need to know specific >> event enumeration. As example (referring to crypto as base design) , >> we have: >> >> odp_comp_compl_t odp_comp_compl_from_event(odp_event_t); >> odp_comp_compl_result(odp_comp_compl_t comp_handle, >> odp_comp_op_result_t *result); >> >> Am I missing any base assumption here? > > odp_event_t is the base class of actual events (e.g. odp_packet_t). Odp_event_t offers minimal operations such as check types or free event without checking the type. Odp_event_t handle needs to be converted to the type specific handle, before application is able to access more features/metadata. The result call is one example of getting metadata from an event. > > With the subtype specification these would turn into: > > odp_packet_t odp_comp_packet_from_event(odp_event_t ev); > int odp_comp_result(odp_comp_result_t *result, odp_packet_t pkt); > > ... or > > int odp_comp_pkt_result(odp_comp_pkt_result_t *result, odp_packet_t pkt); > int odp_comp_xxx_result(odp_comp_xxx_result_t *result, odp_comp_xxx_t xxx); > > ... if you speculate with another non-packet result type. > Can it simply be translated to one single API like odp_xxx_get_event_data() . For ex. for compression, it translates to: odp_event_type_t event_t = odp_event_get_type_subtype(event, &subtype); if(event_t == ODP_EVENT_PACKET) { switch(subtype) { case ODP_EVENT_PACKET_COMP: odp_comp_op_result_t result; odp_comp_get_event_data(event, (void *)&result); /* Or since event is originating from packet so it can be odp_packet_get_event_data() as well. */ break; } } this way odp_xxx_get_event_data() can further scale to handle more event types or sub-types to it. Otherwise I see we will need to add conversion API for each new type and sub-type specific data. What is more preferred approach here? Thanks Shally > > -Petri > >
diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h index f22efce5..2ad3ce84 100644 --- a/include/odp/api/spec/event.h +++ b/include/odp/api/spec/event.h @@ -37,21 +37,91 @@ extern "C" { /** * @typedef odp_event_type_t - * ODP event types: - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT, - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT, ODP_EVENT_IPSEC_STATUS + * Event type + * + * Event type specifies purpose and general format of an event. It can be + * checked with odp_event_type() or odp_event_types(). Each event type has + * functions (e.g. odp_buffer_from_event()) to convert between the generic event + * handle (odp_event_t) and the type specific handle (e.g. odp_buffer_t). + * Results are undefined, if conversion function of a wrong event type is used. + * Application cannot change event type by chaining conversion functions. + * + * List of event types: + * - ODP_EVENT_BUFFER + * - Buffer event (odp_buffer_t) for simple data storage and message passing + * - ODP_EVENT_PACKET + * - Packet event (odp_packet_t) containing packet data and plenty of + * packet processing related metadata + * - ODP_EVENT_TIMEOUT + * - Timeout event (odp_timeout_t) from a timer + * - ODP_EVENT_CRYPTO_COMPL + * - Crypto completion event (odp_crypto_compl_t) + * - ODP_EVENT_IPSEC_STATUS + * - IPSEC status update event (odp_ipsec_status_t) */ /** - * Get event type + * @typedef odp_event_subtype_t + * Event subtype * - * @param event Event handle + * Event subtype expands event type specification by providing more detailed + * purpose and format of an event. It can be checked with odp_event_subtype() or + * odp_event_types(). Each event subtype may define specific functions + * (e.g. odp_ipsec_packet_from_event()) to convert between the generic event + * handle (odp_event_t) and event type specific handle (e.g. odp_packet_t). When + * subtype is known, these subtype specific functions should be preferred over + * the event type general function (e.g. odp_packet_from_event()). Results are + * undefined, if conversion function of a wrong event subtype is used. + * Application cannot change event subtype by chaining conversion functions. + * + * List of event subtypes: + * - ODP_EVENT_PACKET_BASIC + * - Packet event (odp_packet_t) with basic packet metadata + * - ODP_EVENT_PACKET_IPSEC + * - Packet event (odp_packet_t) generated as a result of an IPsec + * operation. It contains IPSEC specific metadata in addition to the basic + * packet metadata. + * - ODP_EVENT_NO_SUBTYPE + * - An event type does not have any subtypes defined + */ + +/** + * Event type of an event + * + * Event type specifies purpose and general format of an event. + * + * @param event Event handle * * @return Event type */ odp_event_type_t odp_event_type(odp_event_t event); /** + * Event subtype of an event + * + * Event subtype expands event type specification by providing more detailed + * purpose and format of an event. + * + * @param event Event handle + * + * @return Event subtype + */ +odp_event_subtype_t odp_event_subtype(odp_event_t event); + +/** + * Event type and subtype of an event + * + * Returns event type and outputs event subtype. + * + * @param event Event handle + * @param[out] subtype Pointer to event subtype for output + * + * @return Event type + */ +odp_event_type_t odp_event_types(odp_event_t event, + odp_event_subtype_t *subtype); + +/** * Get printable value for an odp_event_t * * @param hdl odp_event_t handle to be printed diff --git a/include/odp/arch/default/api/abi/event.h b/include/odp/arch/default/api/abi/event.h index 87220d63..ab3c0f75 100644 --- a/include/odp/arch/default/api/abi/event.h +++ b/include/odp/arch/default/api/abi/event.h @@ -29,10 +29,15 @@ typedef enum odp_event_type_t { ODP_EVENT_PACKET = 2, ODP_EVENT_TIMEOUT = 3, ODP_EVENT_CRYPTO_COMPL = 4, - ODP_EVENT_IPSEC_RESULT = 5, - ODP_EVENT_IPSEC_STATUS = 6 + ODP_EVENT_IPSEC_STATUS = 5 } odp_event_type_t; +typedef enum odp_event_subtype_t { + ODP_EVENT_NO_SUBTYPE = 0, + ODP_EVENT_PACKET_BASIC = 1, + ODP_EVENT_PACKET_IPSEC = 2 +} odp_event_subtype_t; + /** * @} */ diff --git a/platform/linux-generic/include/odp/api/plat/event_types.h b/platform/linux-generic/include/odp/api/plat/event_types.h index 0f517834..5b3a07e3 100644 --- a/platform/linux-generic/include/odp/api/plat/event_types.h +++ b/platform/linux-generic/include/odp/api/plat/event_types.h @@ -39,9 +39,15 @@ typedef enum odp_event_type_t { ODP_EVENT_PACKET = 2, ODP_EVENT_TIMEOUT = 3, ODP_EVENT_CRYPTO_COMPL = 4, - ODP_EVENT_IPSEC_RESULT = 5 + ODP_EVENT_IPSEC_STATUS = 5 } odp_event_type_t; +typedef enum odp_event_subtype_t { + ODP_EVENT_NO_SUBTYPE = 0, + ODP_EVENT_PACKET_BASIC = 1, + ODP_EVENT_PACKET_IPSEC = 2 +} odp_event_subtype_t; + /** * @} */
Event subtype gives more detailed information about the event. Two subtypes (basic and IPSEC packet) are introduced initially. Later on, other packet producing APIs (crypto, comp, etc) may also produce packet events with additional subtypes. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- include/odp/api/spec/event.h | 80 ++++++++++++++++++++-- include/odp/arch/default/api/abi/event.h | 9 ++- .../include/odp/api/plat/event_types.h | 8 ++- 3 files changed, 89 insertions(+), 8 deletions(-) -- 2.13.0