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 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://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-6bd5fd62af0f8992&q=1&e=c4f1686f-725e-48bf-aa62-069489e74009&u=https%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20220627095702.8047-2-
>> 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.

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