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

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

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?

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.

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?

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