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 16:44, Pavan Nikhilesh Bhagavatula wrote:
> 
> 
>> -----Original Message-----
>> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>> Sent: Thursday, July 14, 2022 4:24 PM
>> To: Van Haaren, Harry <harry.van.haaren@intel.com>; 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; 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>;
>> 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
>>
>> 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://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__protect2.fireeye.com_v1_url-3Fk-3D31323334-2D501d5122-2D313273af-
>> 2D454445555731-2D6bd5fd62af0f8992-26q-3D1-26e-3Dc4f1686f-2D725e-
>> 2D48bf-2Daa62-2D069489e74009-26u-3Dhttps-253A-252F-
>> 252Fpatches.dpdk.org-252Fproject-252Fdpdk-252Fpatch-
>> 252F20220627095702.8047-2D2-
>> 2D&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB-
>> fmvgGV3o-g_fjLhk5Pupi9ijohpc&m=-Mr6gUdwMnOZbXSlWeHhYpTVt7UdA-
>> VHDmIC_MuUjne3U5nqwFeoOatsEX10pzow&s=Khz6GF4271RAiO7-
>> 5BY1J2u0BvT8CwWvfwvRSnzeeeM&e=
>>>> 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.
> 
> Maybe we could move this to rte_event_enqueue_new_burst() by adding an
> additional flags param?
> 

This sounds better. Extending this idea, you could add a 
rte_event_enqueue_burst_ex(), which would include a flags parameters, 
specifying which fields were invariant across the burst, including the 
op type (and also sub type, priority, queue id, flow id, and/or sched 
type). Mark the op-specific functions obsolete.

That would move the explosion to the driver instead, where it could be 
better confined.

> This is already done for an existing API rte_event_eth_tx_adapter_enqueue
> where flags is used to signify same Tx queue destination.
> 
> * @param flags
>   *  RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_ flags.
>   *  #RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_SAME_DEST signifies that all the packets
>   *  which are enqueued are destined for the same Ethernet port & Tx queue.
> static inline uint16_t
> rte_event_eth_tx_adapter_enqueue(uint8_t dev_id,
> 				uint8_t port_id,
> 				struct rte_event ev[],
> 				uint16_t nb_events,
> 				const uint8_t flags)
> 
> 
> 
>>
>>> 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.
>>>>>>
>>>
>