> -----Original Message----- > From: shally verma [mailto:[email protected]] > Sent: Tuesday, June 20, 2017 8:55 AM > To: Bill Fischofer <[email protected]> > Cc: Savolainen, Petri (Nokia - FI/Espoo) <[email protected]>; > lng-odp-forward <[email protected]> > 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 > <[email protected]> wrote: > > On Fri, Jun 16, 2017 at 8:33 AM, shally verma > > <[email protected]> wrote: > >> On Fri, Jun 16, 2017 at 6:42 PM, Bill Fischofer > >> <[email protected]> wrote: > >>> On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo) > >>> <[email protected]> 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
