[dpdk-dev] [PATCH 1/3] eventdev: allow for event devices requiring maintenance

[Jerin Jacob]

On Sat, Oct 30, 2021 at 10:49 PM Mattias Rönnblom
<mattias.ronnblom at ericsson.com> wrote:
>
> On 2021-10-29 17:17, Jerin Jacob wrote:
> > On Fri, Oct 29, 2021 at 8:33 PM Mattias Rönnblom
> > <mattias.ronnblom at ericsson.com> wrote:
> >> On 2021-10-29 16:38, Jerin Jacob wrote:
> >>> On Tue, Oct 26, 2021 at 11:02 PM Mattias Rönnblom
> >>> <mattias.ronnblom at ericsson.com> wrote:
> >>>> Extend Eventdev API to allow for event devices which require various
> >>>> forms of internal processing to happen, even when events are not
> >>>> enqueued to or dequeued from a port.
> >>>>
> >>>> PATCH v1:
> >>>>     - Adapt to the move of fastpath function pointers out of
> >>>>       rte_eventdev struct
> >>>>     - Attempt to clarify how often the application is expected to
> >>>>       call rte_event_maintain()
> >>>>     - Add trace point
> >>>> RFC v2:
> >>>>     - Change rte_event_maintain() return type to be consistent
> >>>>       with the documentation.
> >>>>     - Remove unused typedef from eventdev_pmd.h.
> >>>>
> >>>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom at ericsson.com>
> >>>> Tested-by: Richard Eklycke <richard.eklycke at ericsson.com>
> >>>> Tested-by: Liron Himi <lironh at marvell.com>
> >>>> ---
> >>>>
> >>>> +/**
> >>>> + * Maintain an event device.
> >>>> + *
> >>>> + * This function is only relevant for event devices which has the
> >>>> + * RTE_EVENT_DEV_CAP_REQUIRES_MAINT flag set. Such devices require the
> >>>> + * application to call rte_event_maintain() on a port during periods
> >>>> + * which it is neither enqueuing nor dequeuing events from that
> >>>> + * port.
> >>> # We need to add  "by the same core". Right? As other core such as
> >>> service core can not call rte_event_maintain()
> >>
> >> Do you mean by the same lcore thread that "owns" (dequeues and enqueues
> >> to) the port? Yes. I thought that was implicit, since eventdev port are
> >> not MT safe. I'll try to figure out some wording that makes that more clear.
> > OK.
> >
> >>
> >>> # Also, Incase of Adapters enqueue() happens, right? If so, either
> >>> above text is not correct.
> >>> # @Erik Gabriel Carrillo  @Jayatheerthan, Jay @Gujjar, Abhinandan S
> >>> Please review 3/3 patch on adapter change.
> >>> Let me know you folks are OK with change or not or need more time to analyze.
> >>>
> >>> If it need only for the adapter subsystem then can we make it an
> >>> internal API between DSW and adapters?
> >>
> >> No, it's needed for any producer-only eventdev ports, including any such
> >> ports used by the application.
> >
> > In that case, the code path in testeventdev, eventdev_pipeline, etc needs
> > to be updated. I am worried about the performance impact for the drivers they
> > don't have such limitations.
>
>
> Applications that are using some other event device today, and don't
> care about DSW or potential future event devices
> requiringRTE_EVENT_DEV_CAP_REQUIRES_MAINT, won't be affected at all,
> except the ops struct will be 8 bytes larger.

Ack. That's not an issue.

>
>
> A rte_event_maintain() call on a device which doesn't need maintenance
> is just an inlined NULL compare on the ops struct field, which is
> frequently used and should be in a cache close to the core. In my
> benchmarks, I've been unable to measure any additional cost at all.
>
>
> I reviewed the test and example applications last time I attempted to
> upstream this patch set, and from what I remember there was nothing to
> update. Things might have changed and I might misremember, so I'll have
> a look again.


OK.

>
>
> What's important to keep in mind is that applications (DPDK tests,
> examples, user applications etc.) that have producer-only ports or
> otherwise potentially leave eventdev ports "unattended" don't work with
> DSW today, unless the take the measures described in the DSW
> documentation (which for example the eventdev adapters do not). So
> rte_event_maintain() will not break anything that's not already broken.


OK.

>
>
> > Why not have an additional config option in port_config which says
> > it is a producer-only port by an application and takes care of the driver.
> >
> > In the current adapters code, you are calling maintain() when enqueue
> > returns zero.
>
>
> rte_event_maintain() is called when no interaction with the event device
> has been done, during that service function call. That's the overall
> intention.
>
> In the RX adapter, zero flushed events can also mean that the RX adapter
> had buffered events it wanted to flush, but the event device didn't
> accept new events (i.e, back pressure). In that case, the
> rte_event_maintain() call is redundant, but harmless (both because it's
> very low overhead on DSW, and near-zero overhead on any other current
> event device). Plus, if you are back-pressured by the pipeline, RX is
> not the bottleneck so a tiny bit of extra overhead is not an issue.

OK.

Looks good to me. Since DSW has better performance than other SW
drivers, I think, it is OK
to take some limitations to the application.

Please send the next version with the suggested documentation change.

If there is no objection, I will merge it on Wednesday. (3rd  Nov)