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

[Van Haaren, Harry]

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>
> Sent: Thursday, July 14, 2022 7:33 AM
> To: mattias.ronnblom <mattias.ronnblom at ericsson.com>; Thomas Monjalon
> <thomas at monjalon.net>
> Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Ray Kinsella <mdr at ashroe.eu>;
> dev at dpdk.org; McDaniel, Timothy <timothy.mcdaniel at intel.com>; Hemant
> Agrawal <hemant.agrawal at nxp.com>; sachin.saxena at oss.nxp.com;
> liangma at liangbit.com; Mccarthy, Peter <peter.mccarthy at intel.com>; Van Haaren,
> Harry <harry.van.haaren at intel.com>; Carrillo, Erik G <erik.g.carrillo at intel.com>;
> Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>; Jayatheerthan, Jay
> <jay.jayatheerthan at intel.com>; Burakov, Anatoly <anatoly.burakov at 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 at ericsson.com>
> > Sent: Wednesday, July 13, 2022 5:45 PM
> > To: Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>; Thomas
> > Monjalon <thomas at monjalon.net>
> > Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Ray Kinsella
> > <mdr at ashroe.eu>; dev at dpdk.org; timothy.mcdaniel at intel.com; Hemant
> > Agrawal <hemant.agrawal at nxp.com>; sachin.saxena at oss.nxp.com;
> > liangma at liangbit.com; peter.mccarthy at intel.com;
> > harry.van.haaren at intel.com; erik.g.carrillo at intel.com;
> > abhinandan.gujjar at intel.com; jay.jayatheerthan at intel.com;
> > anatoly.burakov at 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 at ericsson.com>
> > >> Sent: Wednesday, July 13, 2022 2:39 PM
> > >> To: Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>; Thomas
> > >> Monjalon <thomas at monjalon.net>
> > >> Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Ray Kinsella
> > >> <mdr at ashroe.eu>; dev at dpdk.org; timothy.mcdaniel at intel.com;
> > Hemant
> > >> Agrawal <hemant.agrawal at nxp.com>; sachin.saxena at oss.nxp.com;
> > >> liangma at liangbit.com; peter.mccarthy at intel.com;
> > >> harry.van.haaren at intel.com; erik.g.carrillo at intel.com;
> > >> abhinandan.gujjar at intel.com; jay.jayatheerthan at intel.com;
> > >> anatoly.burakov at 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 at intel.com;
> > >>> hemant.agrawal at nxp.com;
> > >>> sachin.saxena at oss.nxp.com;
> > >>> mattias.ronnblom at ericsson.com;
> > >>> jerinj at marvell.com;
> > >>> liangma at liangbit.com;
> > >>> peter.mccarthy at intel.com;
> > >>> harry.van.haaren at intel.com;
> > >>> erik.g.carrillo at intel.com;
> > >>> abhinandan.gujjar at intel.com;
> > >>> jay.jayatheerthan at intel.com;
> > >>> mdr at ashroe.eu;
> > >>> anatoly.burakov at intel.com;
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Thomas Monjalon <thomas at monjalon.net>
> > >>>> Sent: Tuesday, July 12, 2022 8:31 PM
> > >>>> To: Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>
> > >>>> Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Ray Kinsella
> > >>>> <mdr at ashroe.eu>; dev at dpdk.org; Pavan Nikhilesh Bhagavatula
> > >>>> <pbhagavatula at 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 at marvell.com:
> > >>>>> From: Pavan Nikhilesh <pbhagavatula at 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 at 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://patches.dpdk.org/project/dpdk/patch/20220627095702.8047-2-
> pbhagavatula at 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()?

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.
> > >