On 2022-07-14 16:44, Pavan Nikhilesh Bhagavatula wrote: > > >> -----Original Message----- >> From: Mattias Rönnblom <mattias.ronnb...@ericsson.com> >> Sent: Thursday, July 14, 2022 4:24 PM >> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Pavan Nikhilesh >> Bhagavatula <pbhagavat...@marvell.com>; Thomas Monjalon >> <tho...@monjalon.net> >> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Ray Kinsella >> <m...@ashroe.eu>; dev@dpdk.org; McDaniel, Timothy >> <timothy.mcdan...@intel.com>; Hemant Agrawal >> <hemant.agra...@nxp.com>; sachin.sax...@oss.nxp.com; >> lian...@liangbit.com; Mccarthy, Peter <peter.mccar...@intel.com>; >> Carrillo, Erik G <erik.g.carri...@intel.com>; Gujjar, Abhinandan S >> <abhinandan.guj...@intel.com>; Jayatheerthan, Jay >> <jay.jayatheert...@intel.com>; Burakov, Anatoly >> <anatoly.bura...@intel.com> >> 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 <pbhagavat...@marvell.com> >>>> Sent: Thursday, July 14, 2022 7:33 AM >>>> To: mattias.ronnblom <mattias.ronnb...@ericsson.com>; Thomas >> Monjalon >>>> <tho...@monjalon.net> >>>> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Ray Kinsella >> <m...@ashroe.eu>; >>>> dev@dpdk.org; McDaniel, Timothy <timothy.mcdan...@intel.com>; >> Hemant >>>> Agrawal <hemant.agra...@nxp.com>; sachin.sax...@oss.nxp.com; >>>> lian...@liangbit.com; Mccarthy, Peter <peter.mccar...@intel.com>; >> Van Haaren, >>>> Harry <harry.van.haa...@intel.com>; Carrillo, Erik G >> <erik.g.carri...@intel.com>; >>>> Gujjar, Abhinandan S <abhinandan.guj...@intel.com>; Jayatheerthan, Jay >>>> <jay.jayatheert...@intel.com>; Burakov, Anatoly >> <anatoly.bura...@intel.com> >>>> Subject: RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event >> type >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: Mattias Rönnblom <mattias.ronnb...@ericsson.com> >>>>> Sent: Wednesday, July 13, 2022 5:45 PM >>>>> To: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com>; Thomas >>>>> Monjalon <tho...@monjalon.net> >>>>> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Ray Kinsella >>>>> <m...@ashroe.eu>; dev@dpdk.org; timothy.mcdan...@intel.com; >> Hemant >>>>> Agrawal <hemant.agra...@nxp.com>; sachin.sax...@oss.nxp.com; >>>>> lian...@liangbit.com; peter.mccar...@intel.com; >>>>> harry.van.haa...@intel.com; erik.g.carri...@intel.com; >>>>> abhinandan.guj...@intel.com; jay.jayatheert...@intel.com; >>>>> anatoly.bura...@intel.com >>>>> 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 <mattias.ronnb...@ericsson.com> >>>>>>> Sent: Wednesday, July 13, 2022 2:39 PM >>>>>>> To: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com>; >> Thomas >>>>>>> Monjalon <tho...@monjalon.net> >>>>>>> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Ray Kinsella >>>>>>> <m...@ashroe.eu>; dev@dpdk.org; timothy.mcdan...@intel.com; >>>>> Hemant >>>>>>> Agrawal <hemant.agra...@nxp.com>; sachin.sax...@oss.nxp.com; >>>>>>> lian...@liangbit.com; peter.mccar...@intel.com; >>>>>>> harry.van.haa...@intel.com; erik.g.carri...@intel.com; >>>>>>> abhinandan.guj...@intel.com; jay.jayatheert...@intel.com; >>>>>>> anatoly.bura...@intel.com >>>>>>> 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 >>>>>>>> timothy.mcdan...@intel.com; >>>>>>>> hemant.agra...@nxp.com; >>>>>>>> sachin.sax...@oss.nxp.com; >>>>>>>> mattias.ronnb...@ericsson.com; >>>>>>>> jer...@marvell.com; >>>>>>>> lian...@liangbit.com; >>>>>>>> peter.mccar...@intel.com; >>>>>>>> harry.van.haa...@intel.com; >>>>>>>> erik.g.carri...@intel.com; >>>>>>>> abhinandan.guj...@intel.com; >>>>>>>> jay.jayatheert...@intel.com; >>>>>>>> m...@ashroe.eu; >>>>>>>> anatoly.bura...@intel.com; >>>>>>>> >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Thomas Monjalon <tho...@monjalon.net> >>>>>>>>> Sent: Tuesday, July 12, 2022 8:31 PM >>>>>>>>> To: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com> >>>>>>>>> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Ray Kinsella >>>>>>>>> <m...@ashroe.eu>; dev@dpdk.org; Pavan Nikhilesh Bhagavatula >>>>>>>>> <pbhagavat...@marvell.com> >>>>>>>>> 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, pbhagavat...@marvell.com: >>>>>>>>>> From: Pavan Nikhilesh <pbhagavat...@marvell.com> >>>>>>>>>> >>>>>>>>>> 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 <pbhagavat...@marvell.com> >>>>>>>>>> --- >>>>>>>>>> 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= >>>> pbhagavat...@marvell.com/ >>> >>> 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. >>>>>> >>> >
