RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event type

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

> -----Original Message-----
> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Sent: Thursday, July 14, 2022 4:24 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella
> <mdr@ashroe.eu>; dev@dpdk.org; McDaniel, Timothy
> <timothy.mcdaniel@intel.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; sachin.saxena@oss.nxp.com;
> liangma@liangbit.com; Mccarthy, Peter <peter.mccarthy@intel.com>;
> Carrillo, Erik G <erik.g.carrillo@intel.com>; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>; Jayatheerthan, Jay
> <jay.jayatheerthan@intel.com>; Burakov, Anatoly
> <anatoly.burakov@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 <pbhagavatula@marvell.com>
> >> Sent: Thursday, July 14, 2022 7:33 AM
> >> To: mattias.ronnblom <mattias.ronnblom@ericsson.com>; Thomas
> Monjalon
> >> <thomas@monjalon.net>
> >> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella
> <mdr@ashroe.eu>;
> >> dev@dpdk.org; McDaniel, Timothy <timothy.mcdaniel@intel.com>;
> Hemant
> >> Agrawal <hemant.agrawal@nxp.com>; sachin.saxena@oss.nxp.com;
> >> liangma@liangbit.com; Mccarthy, Peter <peter.mccarthy@intel.com>;
> Van Haaren,
> >> Harry <harry.van.haaren@intel.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>;
> >> Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Jayatheerthan, Jay
> >> <jay.jayatheerthan@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> >> Subject: RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event
> type
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> >>> Sent: Wednesday, July 13, 2022 5:45 PM
> >>> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Thomas
> >>> Monjalon <thomas@monjalon.net>
> >>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella
> >>> <mdr@ashroe.eu>; dev@dpdk.org; timothy.mcdaniel@intel.com;
> Hemant
> >>> Agrawal <hemant.agrawal@nxp.com>; sachin.saxena@oss.nxp.com;
> >>> liangma@liangbit.com; peter.mccarthy@intel.com;
> >>> harry.van.haaren@intel.com; erik.g.carrillo@intel.com;
> >>> abhinandan.gujjar@intel.com; jay.jayatheerthan@intel.com;
> >>> anatoly.burakov@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.ronnblom@ericsson.com>
> >>>>> Sent: Wednesday, July 13, 2022 2:39 PM
> >>>>> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>;
> Thomas
> >>>>> Monjalon <thomas@monjalon.net>
> >>>>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella
> >>>>> <mdr@ashroe.eu>; dev@dpdk.org; timothy.mcdaniel@intel.com;
> >>> Hemant
> >>>>> Agrawal <hemant.agrawal@nxp.com>; sachin.saxena@oss.nxp.com;
> >>>>> liangma@liangbit.com; peter.mccarthy@intel.com;
> >>>>> harry.van.haaren@intel.com; erik.g.carrillo@intel.com;
> >>>>> abhinandan.gujjar@intel.com; jay.jayatheerthan@intel.com;
> >>>>> anatoly.burakov@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.mcdaniel@intel.com;
> >>>>>> hemant.agrawal@nxp.com;
> >>>>>> sachin.saxena@oss.nxp.com;
> >>>>>> mattias.ronnblom@ericsson.com;
> >>>>>> jerinj@marvell.com;
> >>>>>> liangma@liangbit.com;
> >>>>>> peter.mccarthy@intel.com;
> >>>>>> harry.van.haaren@intel.com;
> >>>>>> erik.g.carrillo@intel.com;
> >>>>>> abhinandan.gujjar@intel.com;
> >>>>>> jay.jayatheerthan@intel.com;
> >>>>>> mdr@ashroe.eu;
> >>>>>> anatoly.burakov@intel.com;
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>> Sent: Tuesday, July 12, 2022 8:31 PM
> >>>>>>> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> >>>>>>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella
> >>>>>>> <mdr@ashroe.eu>; dev@dpdk.org; Pavan Nikhilesh Bhagavatula
> >>>>>>> <pbhagavatula@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, pbhagavatula@marvell.com:
> >>>>>>>> From: Pavan Nikhilesh <pbhagavatula@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 <pbhagavatula@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=
> >> pbhagavatula@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 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.
> >>>>
> >