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

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2022-07-15 15:16, Pavan Nikhilesh Bhagavatula wrote:
> 
> 
>> -----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.
> 

OK.

Can't this be communicate to the application by setting the max dequeue 
depth to 1?

> 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.
>
Yeah, but why? How is this information useful?

The only scenario I can think of is an application employing a fd.io VPP 
style SIMD-heavy "vectorized" design, with the per-burst processing 
progressing as a series of loops, one per layer (or "node"). If the 
event device can only hand you a single event at a time, then such an 
application would suffer, performance wise. In such a case, what is 
relevant is the maximum *dequeue* depth.

For one-event-at-a-time type applications, it will just be a case of an 
unnecessary loop, which cost will be trivial.

>>
>> 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:
> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-afc8d38dd7668c15&q=1&e=8100a5ca-5a51-46b2-91e1-1b6a49519b24&u=http%3A%2F%2Fgit.dpdk.org%2Fdpdk%2Ftree%2Fapp%2Ftest-eventdev%2Ftest_perf_common.c%23n545
> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-bb3f76042845f72f&q=1&e=8100a5ca-5a51-46b2-91e1-1b6a49519b24&u=http%3A%2F%2Fgit.dpdk.org%2Fdpdk%2Ftree%2Fapp%2Ftest-eventdev%2Fevt_common.h%23n99
> 
>>
>>> 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.
>>>>>>>
>>>>>
>>>
>