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

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2021-10-29 18:02, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Jerin Jacob <jerinjacobk@gmail.com>
>> Sent: Friday, October 29, 2021 4:18 PM
>> To: mattias.ronnblom <mattias.ronnblom@ericsson.com>; Van Haaren, Harry
>> <harry.van.haaren@intel.com>; McDaniel, Timothy
>> <timothy.mcdaniel@intel.com>; Pavan Nikhilesh <pbhagavatula@marvell.com>;
>> Hemant Agrawal <hemant.agrawal@nxp.com>; Liang Ma
>> <liangma@liangbit.com>
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Jerin Jacob
>> <jerinj@marvell.com>; Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
>> Carrillo, Erik G <erik.g.carrillo@intel.com>; Jayatheerthan, Jay
>> <jay.jayatheerthan@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.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 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.