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: Friday, July 15, 2022 1:26 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-14 18:42, Pavan Nikhilesh Bhagavatula wrote:
> >
> >
> >> -----Original Message-----
> >> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> >> Sent: Thursday, July 14, 2022 4:13 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-14 08:32, Pavan Nikhilesh Bhagavatula wrote:
> >>>
> >>>
> >>>> -----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.
> >>
> >> I have no idea what a "scheduling context" is (it's not something
> >> related to the Eventdev APIs), but if you have a shortage of them (for
> >> the moment, or always), the driver would just return a short count from
> >> the enqueue burst function. What use has the application of knowing that
> >> the event device can accept at most a certain number of events?
> >>
> >>> 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.
> >>
> >> The documentation of max_event_port_enqueue_depth says:
> >> "Maximum number of events can be enqueued at a time from an event
> port
> >> by this device."
> >>
> >>   From what I can tell, it says nothing about new versus forward, or
> >> release type events.
> >>
> >>> 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-2Def12ffaf4bdb568f-26q-3D1-26e-3Df0a92ad3-2D1167-
> >> 2D448d-2Dbe14-2D0987e939f019-26u-3Dhttps-253A-252F-
> >> 252Fpatches.dpdk.org-252Fproject-252Fdpdk-252Fpatch-
> >> 252F20220627095702.8047-2D2-2Dpbhagavatula-2540marvell.com-
> >> 252F&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB-
> >> fmvgGV3o-g_fjLhk5Pupi9ijohpc&m=iazRQtbylIuGXRfj2NPpf_wwG-
> >>
> ppSwFOPKclj5zcwu2pQG9b7ovePiSBn9k4PjQZ&s=sTrEcWbst59oK_jYe6jXhya
> >> CWwELib6Yr-3d9BccQt4&e=
> >>>
> >>>
> >>>> If the underlying hardware has some limitations,
> >>>> why not let the driver loop until back pressure occurs? Then you can
> >>
> >> You didn't answer this question. Why not let the driver loop, until you
> >> for some reason or the other can't accept more events?
> >
> > CNXK event driver cannot accept forwarding(enq) more than one event
> that has
> > been dequeued. Enqueueing more than one event for
> forwarding/releasing
> > is a violation from HW perspective, this is currently announced by BURST
> capability.
> > But It can enqueue a burst if new events.
> >
> 
> OK, so the limitation we are discussing here is not really related to
> enqueue bursts, but the number of allowed events that can be in-flight
> (in-progress) on the eventdev port?

Yes that’s correct.

In CNXK each event port tracks only one scheduling context (held on dequeue), 
and OP_FWD/OP_RELEASE translate to commands to the device to operate on the 
the scheduling context. There can be only one pending command per a "scheduling context"
until the next dequeue.

> 
> So what happens if you announce some larger max enqueue depth in
> dev_info? If the application can't dequeue more than one event at a
> time, which means it can't possible enqueue more than one forward event
> at a time. Or am I missing something? Even with implicit release turned
> off, the PMD can deny the application having more than one outstanding
> event.

My intention was to cleanly differentiate between OP_FWD and OP_NEW as it might mislead 
an application writer as he  might interpret the max_enqueue_depth to be applicable for 
OP_FWD/OP_NEW unless explicitly stated.

> 
> One issue is head-of-line blocking if you mix new and forward events,
> but that you can solve on the application level by separating new and
> forward bursts. That problem already exists, but for back pressure
> scenarios, where new events are disallowed, but forward are not.
> 
> > If you see the current example implementation we pick the worker based
> on
> > BURST capability for optimizing the enqueue/dequeue by providing a hint
> > to the driver layer.
> >
> 
> What example? What does "pick the worker" mean?

Pick worker functions:
http://git.dpdk.org/dpdk/tree/app/test-eventdev/test_perf_common.c#n545
http://git.dpdk.org/dpdk/tree/app/test-eventdev/evt_common.h#n99

> 
> > Although, we could live with aggregating the events at driver layer based
> on
> > queue. We would still require announce burst capability for new events i.e.
> > changes to the info structure.
> >
> >>
> >>>> 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.
> >>>>>
> >>>
> >