On 2021-10-29 18:02, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Jerin Jacob <jerinjac...@gmail.com> >> Sent: Friday, October 29, 2021 4:18 PM >> To: mattias.ronnblom <mattias.ronnb...@ericsson.com>; Van Haaren, Harry >> <harry.van.haa...@intel.com>; McDaniel, Timothy >> <timothy.mcdan...@intel.com>; Pavan Nikhilesh <pbhagavat...@marvell.com>; >> Hemant Agrawal <hemant.agra...@nxp.com>; Liang Ma >> <lian...@liangbit.com> >> Cc: Richardson, Bruce <bruce.richard...@intel.com>; Jerin Jacob >> <jer...@marvell.com>; Gujjar, Abhinandan S <abhinandan.guj...@intel.com>; >> Carrillo, Erik G <erik.g.carri...@intel.com>; Jayatheerthan, Jay >> <jay.jayatheert...@intel.com>; dpdk-dev <dev@dpdk.org> >> Subject: Re: [dpdk-dev] [PATCH 1/3] eventdev: allow for event devices >> requiring >> maintenance >> >> On Fri, Oct 29, 2021 at 8:33 PM Mattias Rönnblom >> <mattias.ronnb...@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.ronnb...@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.ronnb...@ericsson.com> >>>>> Tested-by: Richard Eklycke <richard.ekly...@ericsson.com> >>>>> Tested-by: Liron Himi <lir...@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 can identify if this "maintain()" call is required by > the RTE_EVENT_DEV_CAP_REQUIRES_MAINT flag correct?
Yes. > So the call through the Eventdev function pointer could be branched over at > application level if required. Yes, you could use the constant propagation trick to eliminate all overhead, if so would be preferred. > Or if the PMD doesn't actually set the ".maintain" function pointer to a > valid value, then it will just return? That's another alternative, but a slightly more costly one. I fail to see the benefit of taking that approach. > Is it easy/possible to test/benchmark this with test-eventdev to measure > perf-impact? > I've benchmarked it in a different test bed, and as I've mentioned, for event devices which doesn't set RTE_EVENT_DEV_CAP_REQUIRES_MAINT and thus have NULL pointer as the maintain function in the ops struct, I was unable to measure any increase at all in overhead, even if they called rte_event_maintain() for every iteration in their "dequeue+process+enqueue" loop. Obviously, there are a couple of more instructions in the code path, so surely there is some cost, but ISP managed to hide everything on the system I was using. >> 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. > The "port hints" that was recently added could be used to communicate such a > concept. > https://protect2.fireeye.com/v1/url?k=131d62df-4c865bdd-131d2244-869a14f4b08c-e995dd419beebf7c&q=1&e=75c16b2e-15e8-4a1a-ab9f-6f80ec7138e6&u=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211014145141.679372-1-harry.van.haaren%40intel.com%2F > > Note that today the "port hints" are *hints*, and must not be *relied on* to > do anything. > This may be useful as (one of) the tools/concepts we could use to try > abstract the "maintain" requirement. > > >> In the current adapters code, you are calling maintain() when enqueue >> returns zero. >> In such a case, if the port is configured as producer and then >> internally it can call maintain. >> >> Thoughts from other eventdev maintainers? >> Cc+ @Van Haaren, Harry @Richardson, Bruce @Gujjar, Abhinandan S >> @Jayatheerthan, Jay @Erik Gabriel Carrillo @McDaniel, Timothy @Pavan >> Nikhilesh @Hemant Agrawal @Liang Ma > Above, thanks for the +CC. > > <snip remaining patch> Sorry, I should have included all from the original (RFC) discussion.
