On 2022-07-14 11:45, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Pavan Nikhilesh Bhagavatula <[email protected]> >> Sent: Thursday, July 14, 2022 7:33 AM >> To: mattias.ronnblom <[email protected]>; Thomas Monjalon >> <[email protected]> >> Cc: Jerin Jacob Kollanukkaran <[email protected]>; Ray Kinsella >> <[email protected]>; >> [email protected]; McDaniel, Timothy <[email protected]>; Hemant >> Agrawal <[email protected]>; [email protected]; >> [email protected]; Mccarthy, Peter <[email protected]>; Van Haaren, >> Harry <[email protected]>; Carrillo, Erik G >> <[email protected]>; >> Gujjar, Abhinandan S <[email protected]>; Jayatheerthan, Jay >> <[email protected]>; Burakov, Anatoly <[email protected]> >> Subject: RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event type >> >> >> >>> -----Original Message----- >>> From: Mattias Rönnblom <[email protected]> >>> Sent: Wednesday, July 13, 2022 5:45 PM >>> To: Pavan Nikhilesh Bhagavatula <[email protected]>; Thomas >>> Monjalon <[email protected]> >>> Cc: Jerin Jacob Kollanukkaran <[email protected]>; Ray Kinsella >>> <[email protected]>; [email protected]; [email protected]; Hemant >>> Agrawal <[email protected]>; [email protected]; >>> [email protected]; [email protected]; >>> [email protected]; [email protected]; >>> [email protected]; [email protected]; >>> [email protected] >>> Subject: Re: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event >>> type >>> >>> On 2022-07-13 12:40, Pavan Nikhilesh Bhagavatula wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Mattias Rönnblom <[email protected]> >>>>> Sent: Wednesday, July 13, 2022 2:39 PM >>>>> To: Pavan Nikhilesh Bhagavatula <[email protected]>; Thomas >>>>> Monjalon <[email protected]> >>>>> Cc: Jerin Jacob Kollanukkaran <[email protected]>; Ray Kinsella >>>>> <[email protected]>; [email protected]; [email protected]; >>> Hemant >>>>> Agrawal <[email protected]>; [email protected]; >>>>> [email protected]; [email protected]; >>>>> [email protected]; [email protected]; >>>>> [email protected]; [email protected]; >>>>> [email protected] >>>>> Subject: Re: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new >>> event >>>>> type >>>>> >>>>> On 2022-07-12 20:11, Pavan Nikhilesh Bhagavatula wrote: >>>>>> +Cc >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> [email protected]; >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Thomas Monjalon <[email protected]> >>>>>>> Sent: Tuesday, July 12, 2022 8:31 PM >>>>>>> To: Pavan Nikhilesh Bhagavatula <[email protected]> >>>>>>> Cc: Jerin Jacob Kollanukkaran <[email protected]>; Ray Kinsella >>>>>>> <[email protected]>; [email protected]; Pavan Nikhilesh Bhagavatula >>>>>>> <[email protected]> >>>>>>> Subject: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event >>>>> type >>>>>>> >>>>>>> External Email >>>>>>> >>>>>>> ---------------------------------------------------------------------- >>>>>>> I'm not your teacher, but please consider Cc'ing people outside of your >>>>>>> company. >>>>>>> >>>>>> >>>>>> I generally add --to-cmd=./devtools/get-maintainer.sh but looks like it's >>>>> useless for >>>>>> sending deprecation notices. >>>>>> >>>>>> Maybe we should update it to include lib/driver maintainers when diff >>> sees >>>>> deprecation.rst >>>>>> >>>>>>> >>>>>>> 27/06/2022 11:57, [email protected]: >>>>>>>> From: Pavan Nikhilesh <[email protected]> >>>>>>>> >>>>>>>> A new field ``max_event_port_enqueue_new_burst`` will be added >>> to >>>>> the >>>>>>>> structure ``rte_event_dev_info``. The field defines the max enqueue >>>>>>>> burst size of new events (OP_NEW) supported by the underlying >>> event >>>>>>>> device. >>>>>>>> >>>>>>>> Signed-off-by: Pavan Nikhilesh <[email protected]> >>>>>>>> --- >>>>>>>> doc/guides/rel_notes/deprecation.rst | 5 +++++ >>>>>>>> 1 file changed, 5 insertions(+) >>>>>>>> >>>>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst >>>>>>> b/doc/guides/rel_notes/deprecation.rst >>>>>>>> index 4e5b23c53d..071317e8e3 100644 >>>>>>>> --- a/doc/guides/rel_notes/deprecation.rst >>>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst >>>>>>>> @@ -125,3 +125,8 @@ Deprecation Notices >>>>>>>> applications should be updated to use the ``dmadev`` library >>> instead, >>>>>>>> with the underlying HW-functionality being provided by the >>>>>>>> ``ioat`` >>> or >>>>>>>> ``idxd`` dma drivers >>>>>>>> + >>>>>>>> +* eventdev: The structure ``rte_event_dev_info`` will be extended >>> to >>>>>>> include the >>>>>>>> + max enqueue burst size of new events supported by the >>> underlying >>>>>>> event device. >>>>>>>> + A new field ``max_event_port_enqueue_new_burst`` will be >>> added >>>>> to >>>>>>> the structure >>>>>>>> + ``rte_event_dev_info`` in DPDK 22.11. >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> Why is this needed, or useful? Why isn't called something with >>>>> "enqueue_depth" in it, like the already-present field? >>>>> >>>> >>>> This is for a case where enqueues with OP_FORWARD/OP_RELEASE only >>> supports >>>> enqueue depth of 1. >>> >>> I assume it's for other cases as well? Any case when the event device >>> has a max forward enqueue depth != max new enqueue depth? >>> >> >> Yes, generally new events have much more flexibility than forwards event. >> >>> I don't see why an event device would have such low max limit on the >>> number events enqueued. >> >> It depends on the number of scheduling contexts a given event port can track. >> Anyway this would align to the current existing feature definitions. The >> existing >> API only defines the enqueue size of fwd and release events i.e. scheduling >> contexts >> already tracked by event device. >> NEW is always a special case as we are adding new scheduling contexts to the >> event >> device, the idea of this patch is to specify that NEW events wouldn’t have >> the same >> restrictions of fwd/release events. >> >> This would also allow us to craft optimized APIs such as >> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-6bd5fd62af0f8992&q=1&e=c4f1686f-725e-48bf-aa62-069489e74009&u=https%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20220627095702.8047-2- >> [email protected]/ > > I've reviewed the above; I'm not in favour of adding even more APIs to the > Eventdev level. > Adding even more enq APIs just overloads applications with options; today we > already have > multiple APIs; > rte_event_enqueue_burst() > rte_event_enqueue_new_burst() > rte_event_enqueue_forward_burst() > > Now the suggestion is to add another one, > rte_event_enqueue_new_queue_burst()? >
Why not three new ones? And why not also rte_event_queue(_new_|_forward_|)(_queue_|)priority_burst()? Plus maybe you want functions that enqueue to the same flow id as well? It's like you can almost hear the combinatorial explosion go off. :) Btw, isn't this the problem that the event vector functionality is supposed to solve? Enqueue many similar events to the same destination. > For an Ethdev analogy: we have rx_burst() and tx_burst(). There is no > tx_to_same_dest_ip_burst(). > > The driver already has all knowledge required if "all events going to same > destination", > so it can optimize for that case already internally. I think Mattias was > asking similar questions, > around PMD having enough info today already. > > Pushing more APIs and complexity to Application level doesn't seem a good > direction to me. > > >>> If the underlying hardware has some limitations, >>> why not let the driver loop until back pressure occurs? Then you can >>> amortize the function call overhead and potentially other software >>> operations (credit system management etc) over multiple events. Plus, >>> the application doesn't need a special case for new events versus >>> forward type events, or this-versus-that event device. >>> >>>> Where as OP_NEW supports higher burst size. >>>> >>>> This is already tied into capability description : >>>> >>>> #define RTE_EVENT_DEV_CAP_BURST_MODE (1ULL << 4) >>>> /**< Event device is capable of operating in burst mode for >>> enqueue(forward, >>>> * release) and dequeue operation. If this capability is not set, >>>> application >>>> * still uses the rte_event_dequeue_burst() and >>> rte_event_enqueue_burst() but >>>> * PMD accepts only one event at a time. >>>> * >>>> * @see rte_event_dequeue_burst() rte_event_enqueue_burst() >>>> */ >>>> >>>>> I think I would rather remove all fields related to the max >>>>> enqueue/dequeue burst sizes. They serve no purpose, as far as I see. If >>>>> you have some HW limit on the maximum number of new events it can >>>>> accept, have the driver retry until backpressure occurs. >>>> >
