[EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata negotiation

[Ferruh Yigit]

On 1/25/2023 1:17 PM, Ferruh Yigit wrote:
> On 1/25/2023 9:30 AM, Hanumanth Reddy Pothula wrote:
>> ++ Ivan Malov and Andrew Rybchenko
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit at amd.com>
>>> Sent: Tuesday, January 24, 2023 11:34 PM
>>> To: Hanumanth Reddy Pothula <hpothula at marvell.com>; Aman Singh
>>> <aman.deep.singh at intel.com>; Yuying Zhang <yuying.zhang at intel.com>
>>> Cc: dev at dpdk.org; andrew.rybchenko at oktetlabs.ru;
>>> viacheslavo at nvidia.com; Jerin Jacob Kollanukkaran <jerinj at marvell.com>;
>>> Nithin Kumar Dabilpuram <ndabilpuram at marvell.com>
>>> Subject: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process
>>> Rx metadata negotiation
>>>
>>> External Email
>>>
>>> ----------------------------------------------------------------------
>>> On 12/21/2022 2:07 AM, Hanumanth Pothula wrote:
>>>> Presently, Rx metadata is sent to PMD by default, leading to a
>>>> performance drop as processing for the same in Rx path takes extra
>>>> cycles.
>>>>
>>>> Hence, add new testpmd command,
>>>>   'enable port <port_id> nic_to_pmd_rx_metadata'
>>>>
>>>> This command helps in sending Rx metadata to PMD and thereby Rx
>>>> metadata flow command requests are processed.
>>>>
>>>> Signed-off-by: Hanumanth Pothula <hpothula at marvell.com>
>>>
>>> Hi Hanumanth,
>>>
>>> I agree with Thomas for the patch.
>>>
>>> 'eth_rx_metadata_negotiate_mp()' requests all Rx metadata offloads to be
>>> enabled, but at this stage if there is no flow rule for Rx metadata why it is
>>> consuming extra cycles?
>>>
>>> Can you update driver code to process Rx metadata when it is enabled by
>>> application (via 'rte_eth_rx_metadata_negotiate()') AND there is at least
>>> one flow rule for it?
>>
>> #1 What is the purpose of rte_eth_rx_metadata_negotiate() API if it is always called by testpmd.
>> We thought it was added so that when that metadata is not needed, application need not call this
>> thereby saving cycles/bandwidth.
>>
> 
> Purpose looks like to 'negotiate', so it is both ways:
> a) To learn Rx Meta capability of HW
> b) To configure Rx Meta feature for HW
> 
> So it is both to help application to learn what flow rule is supported
> and to help driver to improve performance.
> 
> Before this API application assumed all Rx metadata flow rules are
> supported, so why enabling all causing performance drop, that was the
> default without this API.
> 
> But other-way around can be true, to disable some offloads can improve
> driver performance.
> 
> Looking again,
> 

please ignore this email, it has unfinished draft reply by mistake, I
will send a new one.

> 
>> #2 We use this API similar to Rx/Tx offload flags so that we can set things up before device is
>> configured. We thought that is the purpose of having this negotiate API and avoid depleting offload flags.
>>
>> #3 Generally any new offloads added to DPDK would be in disabled state in testpmd and we would have
>> an option to enable it. In this case, testpmd is by default calling this negotiation.
>>
>> We can update the driver if the purpose of this API is clear.
> 
> 
> Hi Hanumanth,
> 
> After looking the history of the API again, you may be right.
> 
> One of the previous version commit log describes the intention better
> [1], because of negative performance impact of enabling Rx metadata
> offload by default and difficulty to switch configuration dynamically,
> there is a desire to learn application intention before configuration.
> 
> Although I have some concerns with this API [2] it is already there as
> stable API.
> 
> So it sounds reasonable to make this configurable for a test
> application, indeed intention of the API is to get this configuration
> from application and operate based on it.
> 
> Next question is what should be the default value, I am not sure about
> it, there are only a few drivers impacted from this overall.
> For the Thomas' point, it helps to test the feature of its impact if it
> is enabled by default.
> 
> Will it work to enable them all by default and add capability to disable
> it in testpmd, which helps to run performance tests also to verify the
> impact of the API?
> 
> Thanks,
> ferruh
> 
> 
> 
> [1]
> https://inbox.dpdk.org/dev/20210902142359.28138-2-ivan.malov@oktetlabs.ru/
> 
> 
> [2]
> API does two things:
> a) Learn Rx Meta capability of HW
> b) Configure Rx Meta feature for HW
> 
> Functionality (a) conflicts with rest of the flow rules that capability
> checked via `rte_flow_validate()` API.
> 
> Functionality (b) conflicts with configuring flow actions, this
> configuration should be controlled by flow rule not with a specific API,
> although I understand the reasoning behind the API.
> 
> RSS_HASH offload seems given example in the discussions but it is still
> controlled via offload flag, there is no specific API to configure RSS
> hash functionality, same could be done here.
> [/2]