On Mon, Jun 12, 2017 at 2:34 AM, Savolainen, Petri (Nokia - FI/Espoo)
<[email protected]> wrote:
>> >> > /**
>> >> > * 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.
EVENT_PACKET remains the same after the addition of packet subtypes
since every subtype is fully an odp_packet_t. The subtypes simply
qualify that by noting what additional metadata is available for this
packet. Applications (or more properly individual routines within
applications) need only be concerned with subtypes when they want to
process this additional metadata.
>
>
>>
>> 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).
>
The application enabled IPsec processing in one, likely small, section
of the application code. If I have an existing large body of code it's
best not to have to comb through it updating every reference to packet
events to now have to consider other packet events that don't present
themselves as ODP_EVENT_PACKETs.
>
>>
>> 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 ...
That's a disruptive change that again forces applications to be
updated. We can't keep adjusting fundamental concepts like this and
stringing these changes out in a series of API "bumps". The concept of
event classes seems overkill since it's packets that have "flavors".
If timeouts developed the need for subtypes then that could be added
there as well, but so far it's only packets that have this need.
The failed decrypt case is trivially handled because routines that
know nothing about IPsec would simply see that the packet had an error
bit set and treat it as any other uncategorized error (e.g., checksum
failure).
>
>
>>
>> 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.
No, applications need only verify that the packet doesn't have any
error bits set, as noted above. This is something it always needs to
do, so there's no additional work needed here.
>
>
>>
>> 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.
Yes, we can special-case specific composites, but this isn't a general
solution to the problem of dealing with multi-stage pipelines. We
don't want to have to keep coming up with one-offs as we add more
types of pipelining, so best to put a the structure for a general
solution to this problem now.
>
>
> 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);
It seems this is just renaming events to be event classes and then
using the subtypes I mentioned for packets. An odp_status_type() API
to get the specific status would be in keeping with the
odp_packet_type() API proposed earlier.
>
>
>
>>
>> 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.
When would that be? Before or after the Tiger Moth release? I'd
suggest it needs to be before because again this requires application
change at a fairly fundamental level and we can't keep doing that sort
of thing.
>
> -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
>> >
>> >
