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

[Ferruh Yigit]

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,


> #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]