> >> >  /**
> >> >   * Get IPSEC status information from an ODP_EVENT_IPSEC_STATUS event
> >> > diff --git a/include/odp/arch/default/api/abi/event.h
> >> b/include/odp/arch/default/api/abi/event.h
> >> > index 87220d63..d807425b 100644
> >> > --- a/include/odp/arch/default/api/abi/event.h
> >> > +++ b/include/odp/arch/default/api/abi/event.h
> >> > @@ -27,9 +27,9 @@ typedef _odp_abi_event_t *odp_event_t;
> >> >  typedef enum odp_event_type_t {
> >> >     ODP_EVENT_BUFFER       = 1,
> >> >     ODP_EVENT_PACKET       = 2,
> >> > -   ODP_EVENT_TIMEOUT      = 3,
> >> > -   ODP_EVENT_CRYPTO_COMPL = 4,
> >> > -   ODP_EVENT_IPSEC_RESULT = 5,
> >> > +   ODP_EVENT_PACKET_IPSEC = 3,
> >> > +   ODP_EVENT_TIMEOUT      = 4,
> >> > +   ODP_EVENT_CRYPTO_COMPL = 5,
> >> >     ODP_EVENT_IPSEC_STATUS = 6
> >> >  } odp_event_type_t;
> >>
> >> Any reason for rearrangement of existing values?
> >
> > Yes, it's now
> >
> >         ODP_EVENT_BUFFER       = 1,
> >         ODP_EVENT_PACKET       = 2,
> >         ODP_EVENT_PACKET_IPSEC = 3,
> >
> > which may turn in couple of weeks into
> >
> >         ODP_EVENT_BUFFER       = 1,
> >         ODP_EVENT_PACKET       = 2,
> >         ODP_EVENT_PACKET_IPSEC = 3,
> >         ODP_EVENT_PACKET_CRYPTO = 4,
> >         ODP_EVENT_PACKET_COMP = 5,
> >
> > ... normal packet and special ones.
> 
> The problem with trying to to this as an ever-growing list of event
> types, is that every time we introduce a new one every application
> needs to change. ODP_EVENT_PACKET_IPSEC is simply an ODP_EVENT_PACKET
> with some additional metadata that may or may not be of interest to a
> given application processing stage. So the basic architecture should
> reflect this fact.

Actually, the idea is that existing application *does not* have to change. 
EVENT_PACKET is still what it is today. So, current applications do not need to 
change anything: e.g. those do not need to check if a packet is coming from 
IPSEC and if the IPSEC operation has failed, etc.


> 
> The typical event loop for a worker thread would look like:
> 
> while (1) {
>         odp_event_t ev = odp_schedule(&queue, ODP_SCHED_WAIT);
>         switch (odp_event_type(ev)) {
>                 case ODP_EVENT_BUFFER:
>                         do_buffer_processing(odp_buffer_from_event(ev),
> queue);
>                         break;
>                 case ODP_EVENT_TIMEOUT:
> 
> do_timeout_processing(odp_timeout_from_event(ev), queue);
>                         break;
>                 case ODP_EVENT_PACKET:
>                         do_packet_processing(odp_packet_from_event(ev),
> queue);
>                         break;
>                 ...
>                 default:
>                         // Unknown event type, log and discard
>         }
> }
> 
> So now that processing loop needs to be updated even if this worker
> thread doesn't care about that optional IPsec metadata.


No, by default nothing changes. Only if application itself *enabled* IPSEC 
processing, it would see PACKET_IPSEC events and would need to handle those... 
but then application was already changed a lot (decided to use IPSEC offload).


> 
> A more flexible approach would be to define a new odp_packet_type_t
> and associated odp_packet_type() API that identifies these subtypes.
> That way the worker packet processing routine can be written as:
> 
> void do_packet_processing(odp_packet_t pkt, odp_queue_t queue)
> {
>         ....
>         switch (odp_packet_type(pkt)) {
>                 case ODP_PACKET_IPSEC:
>                           // do processing of interest with associated
> IPsec metadata
>                           break;
>                 case ODP_PACKET_COMP:
>                           // do processing of interest with associated
> COMP metadata
>                           break;
>                 default:
>         }
> 
>         ... process the basic packet itself
> }


Subtype is not needed - event type is the subtype already.

We have a plan of adding odp_event_class_t instead (ODP_EVENT_CLASS_PACKET, 
...) as a super-type, but decided that it can wait for the actual need. The 
need is not yet there today as all packets are basic packets. After 
IPSEC/crypto/comp packet types are merged, it could be needed by an application 
pipeline stage which needs to do simple processing for all type packets. But 
even with that only successfully decrypted packets would go through that stage, 
otherwise failed decrypt would leave inner packet as garbage ...


> 
> Note that this means that anytime we add a new packet subtype nothing
> has to change at the application layer. It only needs to add code to
> process that optional metadata in stages that specifically want to do
> something with it. Otherwise they are treated as regular packets. So
> the fact that this packet was IPsec decrypted before it got to this
> stage is something the stage need not be aware of or care about unless
> it wants to take special action because the packet arrived via that
> offload path.

Event class would enable this later on. As explained above, when IPSEC 
(/crypto/comp) is enabled application needs to have at least one stage which 
checks the results, otherwise other stage may end up processing garbage.


> 
> As we add more pipeline capabilities this sort of structure will
> become increasingly important as the whole idea behind pipelining is
> that worker threads don't need to be aware of where the packets came
> from or their offload history in order to do what that stage needs to
> do. Some stages will want to inspect that history and do something
> different based on it, but most will not.
> 
> This becomes even more important when we consider pipeline chains. For
> example, what happens when we support full offloading of packet that
> is IPsec decrypted, then decompressed, and then handed to a worker?
> Trying to encode these composites in a single event type quickly
> becomes completely unmanageable. However, we can easily extend
> odp_packet_type() to be:

Pipelining in API would change the type and connection between stages would be 
(mostly) invisible to the application. So, e.g. pktin -> IPSEC decrypt -> 
decomp would result PACKET_COMP event for successfully decrypted and 
decompressed packets. Some small amount of IPSEC metadata could be needed still 
to be visible (e.g. "packet was ipsec", "ipsec sa was X"). Failed IPSEC step 
would result PACKET_IPSEC with error flags.


This is roughly the idea of event classes.

typedef enum odp_event_class_t {
     ODP_EVENT_BUFFER,
     ODP_EVENT_PACKET, // basic, ipsec, crypto, comp, ... all share the same 
basic packet metadata
     ODP_EVENT_TIMER,
     ODP_EVENT_STATUS, // all status events from ODP offloads 
} odp_event_class_t

odp_event_class_t odp_event_class(odp_event_t ev);



> 
> odp_packet_type_t odp_packet_type(odp_packet_t pkt, int level);
> 
> So that odp_packet_type(pkt, 0) gives the current packet subtype,
> odp_packet_type(pkt, 1) gives the previous, etc. That way processing
> stages that need this sort of "forensic" information can get it if
> needed, but other stages can ignore it. We may want to add the API:


Most application code is written against a specific event type (e.g. basic 
packet vs. ipsec packet). The class spec can be added later when we have 
gathered more use cases for it.

-Petri



> 
> int odp_packet_history(odp_packet_t pkt);
> 
> to return the number of such levels available, but this sort of
> extension may be premature for now.
> 
> >
> > -Petri
> >
> >

Reply via email to