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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Sun, Oct 31, 2021 at 6:47 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Sat, Oct 30, 2021 at 10:49 PM Mattias Rönnblom
> <mattias.ronnblom@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@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@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@ericsson.com>
> > >>>> Tested-by: Richard Eklycke <richard.eklycke@ericsson.com>
> > >>>> Tested-by: Liron Himi <lironh@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)


Applied v3 version  to dpdk-next-net-eventdev/for-main. Thanks