Re: Eventdev dequeue-enqueue event correlation

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Oct 25, 2023 at 09:40:54AM +0200, Mattias Rönnblom wrote:
> On 2023-10-24 11:10, Bruce Richardson wrote:
> > On Tue, Oct 24, 2023 at 09:10:30AM +0100, Bruce Richardson wrote:
> > > On Mon, Oct 23, 2023 at 06:10:54PM +0200, Mattias Rönnblom wrote:
> > > > Hi.
> > > > 
> > > > Consider an Eventdev app using atomic-type scheduling doing something like:
> > > > 
> > > >      struct rte_event events[3];
> > > > 
> > > >      rte_event_dequeue_burst(dev_id, port_id, events, 3, 0);
> > > > 
> > > >      /* Assume three events were dequeued, and the application decides
> > > >       * it's best off to processing event 0 and 2 consecutively */
> > > > 
> > > >      process(&events[0]);
> > > >      process(&events[2]);
> > > > 
> > > >      events[0].queue_id++;
> > > >      events[0].op = RTE_EVENT_OP_FORWARD;
> > > >      events[2].queue_id++;
> > > >      events[2].op = RTE_EVENT_OP_FORWARD;
> > > > 
> > > >      rte_event_enqueue_burst(dev_id, port_id, &events[0], 1);
> > > >      rte_event_enqueue_burst(dev_id, port_id, &events[2], 1);
> > > > 
> > > >      process(&events[1]);
> > > >      events[1].queue_id++;
> > > >      events[1].op = RTE_EVENT_OP_FORWARD;
> > > > 
> > > >      rte_event_enqueue_burst(dev_id, port_id, &events[1], 1);
> > > > 
> > > > If one would just read the Eventdev API spec, they might expect this to work
> > > > (especially since impl_opaque hints as potentially be useful for the purpose
> > > > of identifying events).
> > > > 
> > > > However, on certain event devices, it doesn't (and maybe rightly so). If
> > > > event 0 and 2 belongs to the same flow (queue id + flow id pair), and event
> > > > 1 belongs to some other, then this other flow would be "unlocked" at the
> > > > point of the second enqueue operation (and thus be processed on some other
> > > > core, in parallel). The first flow would still be needlessly "locked".
> > > > 
> > > > Such event devices require the order of the enqueued events to be the same
> > > > as the dequeued events, using RTE_EVENT_OP_RELEASE type events as "fillers"
> > > > for dropped events.
> > > > 
> > > > Am I missing something in the Eventdev API documentation?
> > > > 
> > > 
> > > Much more likely is that the documentation is missing something. We should
> > > explicitly clarify this behaviour, as it's required by a number of drivers.
> > > 
> > > > Could an event device use the impl_opaque field to track the identity of an
> > > > event (and thus relax ordering requirements) and still be complaint toward
> > > > the API?
> > > > 
> > > 
> > > Possibly, but the documentation also doesn't report that the impl_opaque
> > > field must be preserved between dequeue and enqueue. When forwarding a
> > > packet it's well possible for an app to extract an mbuf from a dequeued
> > > event and create a new event for sending it back in to the eventdev. For
> 
> Such a behavior would be in violation of a part of the Eventdev API contract
> actually specified. The rte_event struct documentation says about
> impl_opaque that "An implementation may use this field to hold
> implementation specific value to share between dequeue and enqueue
> operation. The application should not modify this field. "
> 
> I see no other way to read this than that "an implementation" here is
> referring to an event device PMD. The requirement that the application can't
> modify this field only make sense in the context of "from dequeue to
> enqueue".
> 

Yep, you are completely correct. For some reason, I had this in my head the
other way round, that it was for internal use between the enqueue and
dequeue. My mistake! :-(

> > > example, if the first stage post-RX is doing classify, it's entirely
> > > possible for every single field in the event header to be different for the
> > > event returned compared to dequeue (flow_id recomputed, event type/source
> > > adjusted, target queue_id and priority updated, op type changed to forward
> > > from new, etc. etc.).
> > > 
> > > > What happens if a RTE_EVENT_OP_NEW event is inserted into the mix of
> > > > OP_FORWARD and OP_RELEASE type events being enqueued? Again I'm not clear on
> > > > what the API says, if anything.
> > > > 
> > > OP_NEW should have no effect on the "history-list" of events previousl
> > > dequeued. Again, our docs should clarify that explicitly. Thanks for
> > > calling all this out.
> > > 
> > Looking at the docs we have, I would propose adding a new subsection "Event
> > Operations", as section 49.1.6 to [1]. There we could explain "New",
> > "Forward" and "Release" events - what they mean for the different queue
> > types and how to use them. That section could also cover the enqueue
> > ordering rules, as the use of event "history" is necessary to explain
> > releases and forwards.
> > 
> > This seem reasonable? If nobody else has already started on updating docs
> > for this, I'm happy enough to give it a stab.
> > 
> 
> Batch dequeues not only provides an opportunity to amortize per-interaction
> overhead with the event device, it also allows the application to reshuffle
> the order in which it decides to process the events.
> 
> Such reshuffling may have a very significant impact on performance. At a
> minimum, cache locality improves, and in case the app is able to "vector
> processing" (e.g., something akin to what fd.io VPP does), the gains may be
> further increased.
> 
> One may argue the app/core should just "do what it's told" by the event
> device. After all, an event device is a work scheduler, and reshuffling
> items of work certainly counts as (micro-)scheduling work.
> 
> However it's much to hope for to expect a fairly generic function,
> especially if it comes in the form of hardware, with a design frozen years
> ago, to be able to arrange the work in whatever is currently optimal order
> for one particular application.
> 
> What such an app can do (or must do, if it has efficiency constraints) is to
> buffer the events on the output side, rearranging them in accordance to the
> yet-seemingly-undocumented Eventdev API contract. That's certainly possible,
> and not very difficult, but it seems to me that this really is the job
> something in the platform (e.g., in Eventdev or the event device PMD).
> 
> One way out of this could be to add an "implicit release-*only*" mode of
> operation for eventdev.
> 
> In such a mode, the RTE_SCHED_TYPE_ATOMIC per-flow "lock" (and its ORDERED
> equivalent, if there is one) would be held until the next dequeue. In such a
> mode, the difference between OP_FORWARD and OP_NEW events would just be the
> back-pressure watermark (new_event_threshold).
> 
> That pre-rte_event_enqueue_burst() buffering would prevent the event device
> from releasing "locks" that could otherwise be released, but the typical
> cost of event device interaction is so high so I have my doubts about how
> useful that feature is. If you are worried about "locks" held for a long
> time, one may need to use short bursts anyway (since worst-case critical
> section length is not reduced by such RELEASEs).
> 
> Another option would be to have the current RTE_EVENT_DEV_CAP_BURST_MODE
> capable PMDs start using the "impl_opaque" field for the purpose of matching
> in and out events. It would require applications to actually start adhering
> to the "don't touch impl_opaque" requirement of the Eventdev API.
> 
> Those "fixes" are not mutually exclusive.
> 
> A side note: it's unfortunate there are no bits in the rte_event struct that
> can be used for "event id"/"event SN"/"event dequeue idx" type information,
> if an app would like to work around this issue with current PMDs.
> 
Lots of good points here. We'll take a look and see what we can do in our
drivers and any other ideas or suggestions.

/Bruce