On 2022-07-14 16:44, Pavan Nikhilesh Bhagavatula wrote: > > >> -----Original Message----- >> From: Mattias Rönnblom <[email protected]> >> Sent: Thursday, July 14, 2022 4:24 PM >> To: Van Haaren, Harry <[email protected]>; Pavan Nikhilesh >> Bhagavatula <[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]>; >> 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 >> >> 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://urldefense.proofpoint.com/v2/url?u=https- >> 3A__protect2.fireeye.com_v1_url-3Fk-3D31323334-2D501d5122-2D313273af- >> 2D454445555731-2D6bd5fd62af0f8992-26q-3D1-26e-3Dc4f1686f-2D725e- >> 2D48bf-2Daa62-2D069489e74009-26u-3Dhttps-253A-252F- >> 252Fpatches.dpdk.org-252Fproject-252Fdpdk-252Fpatch- >> 252F20220627095702.8047-2D2- >> 2D&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB- >> fmvgGV3o-g_fjLhk5Pupi9ijohpc&m=-Mr6gUdwMnOZbXSlWeHhYpTVt7UdA- >> VHDmIC_MuUjne3U5nqwFeoOatsEX10pzow&s=Khz6GF4271RAiO7- >> 5BY1J2u0BvT8CwWvfwvRSnzeeeM&e= >>>> [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. > > Maybe we could move this to rte_event_enqueue_new_burst() by adding an > additional flags param? >
This sounds better. Extending this idea, you could add a rte_event_enqueue_burst_ex(), which would include a flags parameters, specifying which fields were invariant across the burst, including the op type (and also sub type, priority, queue id, flow id, and/or sched type). Mark the op-specific functions obsolete. That would move the explosion to the driver instead, where it could be better confined. > This is already done for an existing API rte_event_eth_tx_adapter_enqueue > where flags is used to signify same Tx queue destination. > > * @param flags > * RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_ flags. > * #RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_SAME_DEST signifies that all the > packets > * which are enqueued are destined for the same Ethernet port & Tx queue. > static inline uint16_t > rte_event_eth_tx_adapter_enqueue(uint8_t dev_id, > uint8_t port_id, > struct rte_event ev[], > uint16_t nb_events, > const uint8_t flags) > > > >> >>> 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. >>>>>> >>> >
