[v5,2/2] app/testpmd: add command to process Rx metadata negotiation
Checks
Commit Message
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@marvell.com>
---
app/test-pmd/cmdline.c | 58 +++++++++++++++++++++
app/test-pmd/config.c | 9 ++++
app/test-pmd/testpmd.c | 5 +-
app/test-pmd/testpmd.h | 1 +
doc/guides/testpmd_app_ug/testpmd_funcs.rst | 6 +++
5 files changed, 77 insertions(+), 2 deletions(-)
Comments
21/12/2022 03:07, Hanumanth Pothula:
> 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@marvell.com>
As said in previous versions, please don't add such option in testpmd.
The problem is somewhere else, probably in your driver.
You should make sure your driver is not sending metadata if not needed.
By the way you didn't reply to the last comments on v3 in December.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, January 18, 2023 4:03 PM
> To: Hanumanth Reddy Pothula <hpothula@marvell.com>
> Cc: Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; dev@dpdk.org;
> andrew.rybchenko@oktetlabs.ru; viacheslavo@nvidia.com; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; david.marchand@redhat.com
> Subject: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process
> Rx metadata negotiation
>
> External Email
>
> ----------------------------------------------------------------------
> 21/12/2022 03:07, Hanumanth Pothula:
> > 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@marvell.com>
>
> As said in previous versions, please don't add such option in testpmd.
> The problem is somewhere else, probably in your driver.
> You should make sure your driver is not sending metadata if not needed.
> By the way you didn't reply to the last comments on v3 in December.
>
>
I added following comment on V4,
- As per spec rte_eth_rx_metadata_negotiate() is processed only when dev_configured is set. Hence can't enable(Rx metadata negotiation) automatically when a flow command requests metadata.
- Add new testpmd command to allow NIC to PMD Rx metadata negotiation.
As we can't enable Rx metadata negotiation on fly, introduced new testpmd command to enable Rx metadata negotiation by resetting the ports.
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@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?
++ Ivan Malov and Andrew Rybchenko
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, January 24, 2023 11:34 PM
> To: Hanumanth Reddy Pothula <hpothula@marvell.com>; Aman Singh
> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; andrew.rybchenko@oktetlabs.ru;
> viacheslavo@nvidia.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Nithin Kumar Dabilpuram <ndabilpuram@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@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.
#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.
19/01/2023 11:33, Hanumanth Reddy Pothula:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 21/12/2022 03:07, Hanumanth Pothula:
> > > 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@marvell.com>
> >
> > As said in previous versions, please don't add such option in testpmd.
> > The problem is somewhere else, probably in your driver.
> > You should make sure your driver is not sending metadata if not needed.
> > By the way you didn't reply to the last comments on v3 in December.
> >
> I added following comment on V4,
> - As per spec rte_eth_rx_metadata_negotiate() is processed only
> when dev_configured is set.
Not exactly. It is said
"
Invoke this API before the first rte_eth_dev_configure() invocation
to let the PMD make preparations that are inconvenient to do later.
"
> Hence can't enable(Rx metadata negotiation)
> automatically when a flow command requests metadata.
This sentence does not make any sense.
First you negotiate to prepare the feature.
Later you enable per flow.
> - Add new testpmd command to allow NIC to PMD Rx metadata negotiation.
>
> As we can't enable Rx metadata negotiation on fly,
> introduced new testpmd command to enable Rx metadata negotiation
> by resetting the ports.
You can enable on the fly. Fix your driver.
25/01/2023 10:30, Hanumanth Reddy Pothula:
> ++ Ivan Malov and Andrew Rybchenko
>
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> > 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@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.
testpmd is for testing all features. That's why all is negotiated.
Cycles should be saved if you don't enable it until a flow rule requires it.
> #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.
It is just a configuration negotiation specific to metadata.
> #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.
Negotiating is not enabling.
> We can update the driver if the purpose of this API is clear.
Please do.
On 1/25/2023 9:30 AM, Hanumanth Reddy Pothula wrote:
> ++ Ivan Malov and Andrew Rybchenko
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Tuesday, January 24, 2023 11:34 PM
>> To: Hanumanth Reddy Pothula <hpothula@marvell.com>; Aman Singh
>> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
>> Cc: dev@dpdk.org; andrew.rybchenko@oktetlabs.ru;
>> viacheslavo@nvidia.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>> Nithin Kumar Dabilpuram <ndabilpuram@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@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]
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@amd.com>
>>> Sent: Tuesday, January 24, 2023 11:34 PM
>>> To: Hanumanth Reddy Pothula <hpothula@marvell.com>; Aman Singh
>>> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
>>> Cc: dev@dpdk.org; andrew.rybchenko@oktetlabs.ru;
>>> viacheslavo@nvidia.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>>> Nithin Kumar Dabilpuram <ndabilpuram@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@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]
On 1/25/2023 9:30 AM, Hanumanth Reddy Pothula wrote:
> ++ Ivan Malov and Andrew Rybchenko
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Tuesday, January 24, 2023 11:34 PM
>> To: Hanumanth Reddy Pothula <hpothula@marvell.com>; Aman Singh
>> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
>> Cc: dev@dpdk.org; andrew.rybchenko@oktetlabs.ru;
>> viacheslavo@nvidia.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>> Nithin Kumar Dabilpuram <ndabilpuram@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@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.
>
> #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]
On 1/25/2023 12:55 PM, Thomas Monjalon wrote:
> 25/01/2023 10:30, Hanumanth Reddy Pothula:
>> ++ Ivan Malov and Andrew Rybchenko
>>
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>> 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@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.
>
> testpmd is for testing all features. That's why all is negotiated.
> Cycles should be saved if you don't enable it until a flow rule requires it.
>
Hi Thomas,
Not just for saving cycles, but from testing perspective too, do you
think does it work if a way to disable these Rx metadata added by
keeping default behavior as it is?
And new command can be in a consistent command syntax like:
"port config <port_id> ..."
>> #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.
>
> It is just a configuration negotiation specific to metadata.
>
>> #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.
>
> Negotiating is not enabling.
>
>> We can update the driver if the purpose of this API is clear.
>
> Please do.
>
Is following understanding correct?
API Flow Rule Result
----- ------------ --------
Enable No Rule Feature Disabled
Enable Rule exist Feature Enabled
Disable X Feature Disabled
25/01/2023 14:55, Ferruh Yigit:
> On 1/25/2023 12:55 PM, Thomas Monjalon wrote:
> > 25/01/2023 10:30, Hanumanth Reddy Pothula:
> >> ++ Ivan Malov and Andrew Rybchenko
> >>
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >>> 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@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.
> >
> > testpmd is for testing all features. That's why all is negotiated.
> > Cycles should be saved if you don't enable it until a flow rule requires it.
> >
>
> Hi Thomas,
>
> Not just for saving cycles, but from testing perspective too, do you
> think does it work if a way to disable these Rx metadata added by
> keeping default behavior as it is?
>
> And new command can be in a consistent command syntax like:
> "port config <port_id> ..."
Yes I agree it would be good to have a way to test different values.
And it would allow to completely disable metadata I suppose.
Note: I don't understand why we don't have
RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
negotiated in this function. Probably something to add.
> >> #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.
> >
> > It is just a configuration negotiation specific to metadata.
> >
> >> #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.
> >
> > Negotiating is not enabling.
> >
> >> We can update the driver if the purpose of this API is clear.
> >
> > Please do.
>
> Is following understanding correct?
>
> API Flow Rule Result
> ----- ------------ --------
> Enable No Rule Feature Disabled
> Enable Rule exist Feature Enabled
> Disable X Feature Disabled
In the API column, you should say "negotiated" instead of "Enable".
> >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?
The spirit of the negotiating features/Rx/Tx offloads upfront is to have it disabled by default and enable the feature only when needed. Having the features enabled by default is probably against that spirit.
We understand the concerns with drivers that didn't not implement that API.
Why not disable it by default(like other offloads) and modify rte_flow action creation in testpmd to check for if !ENOSUP and feature disabled and add print to enable. So for the PMD's that won't support rte_eth_rx_metadata_negotiate(), there won't be any difference and for very few PMD's that support this API, they need to enable it before using RTE_FLOW with MARK/FLAG.
Behavior change would be seen only with two PMD's(cnxk, sfc).
> Note: I don't understand why we don't have
> RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> negotiated in this function. Probably something to add.
The purpose of negotiate is to tell the PMD upfront so that PMD can prepare
HW appropriately. Having these new actions would be very late to inform PMD and
I think won't solve the purpose.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, January 25, 2023 7:30 PM
> To: Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>;
> Ivan Malov <ivan.malov@oktetlabs.ru>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Hanumanth Reddy Pothula
> <hpothula@marvell.com>; Ferruh Yigit <ferruh.yigit@amd.com>
> Cc: viacheslavo@nvidia.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Nithin Kumar
> Dabilpuram <ndabilpuram@marvell.com>; david.marchand@redhat.com
> Subject: Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata
> negotiation
>
> 25/01/2023 14:55, Ferruh Yigit:
> > On 1/25/2023 12:55 PM, Thomas Monjalon wrote:
> > > 25/01/2023 10:30, Hanumanth Reddy Pothula:
> > >> ++ Ivan Malov and Andrew Rybchenko
> > >>
> > >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> > >>> 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@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.
> > >
> > > testpmd is for testing all features. That's why all is negotiated.
> > > Cycles should be saved if you don't enable it until a flow rule requires it.
> > >
> >
> > Hi Thomas,
> >
> > Not just for saving cycles, but from testing perspective too, do you
> > think does it work if a way to disable these Rx metadata added by
> > keeping default behavior as it is?
> >
> > And new command can be in a consistent command syntax like:
> > "port config <port_id> ..."
>
> Yes I agree it would be good to have a way to test different values.
> And it would allow to completely disable metadata I suppose.
>
> Note: I don't understand why we don't have
> RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> negotiated in this function. Probably something to add.
>
>
> > >> #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.
> > >
> > > It is just a configuration negotiation specific to metadata.
> > >
> > >> #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.
> > >
> > > Negotiating is not enabling.
> > >
> > >> We can update the driver if the purpose of this API is clear.
> > >
> > > Please do.
> >
> > Is following understanding correct?
> >
> > API Flow Rule Result
> > ----- ------------ --------
> > Enable No Rule Feature Disabled
> > Enable Rule exist Feature Enabled
> > Disable X Feature Disabled
>
> In the API column, you should say "negotiated" instead of "Enable".
>
>
Please reply inline below instead of doing an incomplete copy
of the replies on top.
25/01/2023 15:42, Nithin Kumar Dabilpuram:
> > >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?
>
> The spirit of the negotiating features/Rx/Tx offloads upfront is to have it disabled by default and enable the feature only when needed. Having the features enabled by default is probably against that spirit.
>
> We understand the concerns with drivers that didn't not implement that API.
There is no such concern I think.
> Why not disable it by default(like other offloads) and modify rte_flow action creation in testpmd to check for if !ENOSUP and feature disabled and add print to enable. So for the PMD's that won't support rte_eth_rx_metadata_negotiate(), there won't be any difference and for very few PMD's that support this API, they need to enable it before using RTE_FLOW with MARK/FLAG.
> Behavior change would be seen only with two PMD's(cnxk, sfc).
I think you missed the whole point.
Ferruh is proposing to have a command "port config <port_id> ..."
to configure the flags to negotiate.
Are you OK with this approach?
> > Note: I don't understand why we don't have
> > RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> > negotiated in this function. Probably something to add.
>
> The purpose of negotiate is to tell the PMD upfront so that PMD can prepare
> HW appropriately. Having these new actions would be very late to inform PMD and
> I think won't solve the purpose.
I am not talking about your problem here.
I am just saying that TAG and META should be negotiated as well
in rte_eth_rx_metadata_negotiate().
> From: Thomas Monjalon <thomas@monjalon.net>
> > 25/01/2023 14:55, Ferruh Yigit:
> > > On 1/25/2023 12:55 PM, Thomas Monjalon wrote:
> > > > 25/01/2023 10:30, Hanumanth Reddy Pothula:
> > > >> ++ Ivan Malov and Andrew Rybchenko
> > > >>
> > > >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> > > >>> 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@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.
> > > >
> > > > testpmd is for testing all features. That's why all is negotiated.
> > > > Cycles should be saved if you don't enable it until a flow rule requires it.
> > > >
> > >
> > > Hi Thomas,
> > >
> > > Not just for saving cycles, but from testing perspective too, do you
> > > think does it work if a way to disable these Rx metadata added by
> > > keeping default behavior as it is?
> > >
> > > And new command can be in a consistent command syntax like:
> > > "port config <port_id> ..."
> >
> > Yes I agree it would be good to have a way to test different values.
> > And it would allow to completely disable metadata I suppose.
> >
> > Note: I don't understand why we don't have
> > RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> > negotiated in this function. Probably something to add.
> >
> >
> > > >> #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.
> > > >
> > > > It is just a configuration negotiation specific to metadata.
> > > >
> > > >> #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.
> > > >
> > > > Negotiating is not enabling.
> > > >
> > > >> We can update the driver if the purpose of this API is clear.
> > > >
> > > > Please do.
> > >
> > > Is following understanding correct?
> > >
> > > API Flow Rule Result
> > > ----- ------------ --------
> > > Enable No Rule Feature Disabled
> > > Enable Rule exist Feature Enabled
> > > Disable X Feature Disabled
> >
> > In the API column, you should say "negotiated" instead of "Enable".
> >
> >
>
>
Please see inline.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, January 26, 2023 4:33 PM
> To: Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>
> Cc: Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>;
> Ivan Malov <ivan.malov@oktetlabs.ru>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Hanumanth Reddy Pothula
> <hpothula@marvell.com>; Ferruh Yigit <ferruh.yigit@amd.com>; viacheslavo@nvidia.com;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>; david.marchand@redhat.com
> Subject: Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata
> negotiation
>
> Please reply inline below instead of doing an incomplete copy
> of the replies on top.
Ack, was just trying converge threads.
>
>
> 25/01/2023 15:42, Nithin Kumar Dabilpuram:
> > > >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?
> >
> > The spirit of the negotiating features/Rx/Tx offloads upfront is to have it disabled by
> default and enable the feature only when needed. Having the features enabled by default
> is probably against that spirit.
> >
> > We understand the concerns with drivers that didn't not implement that API.
>
> There is no such concern I think.
>
> > Why not disable it by default(like other offloads) and modify rte_flow action creation in
> testpmd to check for if !ENOSUP and feature disabled and add print to enable. So for the
> PMD's that won't support rte_eth_rx_metadata_negotiate(), there won't be any
> difference and for very few PMD's that support this API, they need to enable it before
> using RTE_FLOW with MARK/FLAG.
> > Behavior change would be seen only with two PMD's(cnxk, sfc).
>
> I think you missed the whole point.
> Ferruh is proposing to have a command "port config <port_id> ..."
> to configure the flags to negotiate.
> Are you OK with this approach?
>
Yes, we are fine to have such command to enable and disable the feature with default being it disabled if supported by PMD.
Is default being disabled fine if the feature is supported by a PMD ?
> > > Note: I don't understand why we don't have
> > > RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> > > negotiated in this function. Probably something to add.
> >
> > The purpose of negotiate is to tell the PMD upfront so that PMD can prepare
> > HW appropriately. Having these new actions would be very late to inform PMD and
> > I think won't solve the purpose.
>
> I am not talking about your problem here.
> I am just saying that TAG and META should be negotiated as well
> in rte_eth_rx_metadata_negotiate().
>
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 25/01/2023 14:55, Ferruh Yigit:
> > > > On 1/25/2023 12:55 PM, Thomas Monjalon wrote:
> > > > > 25/01/2023 10:30, Hanumanth Reddy Pothula:
> > > > >> ++ Ivan Malov and Andrew Rybchenko
> > > > >>
> > > > >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> > > > >>> 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@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.
> > > > >
> > > > > testpmd is for testing all features. That's why all is negotiated.
> > > > > Cycles should be saved if you don't enable it until a flow rule requires it.
> > > > >
> > > >
> > > > Hi Thomas,
> > > >
> > > > Not just for saving cycles, but from testing perspective too, do you
> > > > think does it work if a way to disable these Rx metadata added by
> > > > keeping default behavior as it is?
> > > >
> > > > And new command can be in a consistent command syntax like:
> > > > "port config <port_id> ..."
> > >
> > > Yes I agree it would be good to have a way to test different values.
> > > And it would allow to completely disable metadata I suppose.
> > >
> > > Note: I don't understand why we don't have
> > > RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> > > negotiated in this function. Probably something to add.
> > >
> > >
> > > > >> #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.
> > > > >
> > > > > It is just a configuration negotiation specific to metadata.
> > > > >
> > > > >> #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.
> > > > >
> > > > > Negotiating is not enabling.
> > > > >
> > > > >> We can update the driver if the purpose of this API is clear.
> > > > >
> > > > > Please do.
> > > >
> > > > Is following understanding correct?
> > > >
> > > > API Flow Rule Result
> > > > ----- ------------ --------
> > > > Enable No Rule Feature Disabled
> > > > Enable Rule exist Feature Enabled
> > > > Disable X Feature Disabled
> > >
> > > In the API column, you should say "negotiated" instead of "Enable".
> > >
> > >
> >
> >
>
>
>
>
27/01/2023 06:02, Nithin Kumar Dabilpuram:
> From: Thomas Monjalon <thomas@monjalon.net>
> > Ferruh is proposing to have a command "port config <port_id> ..."
> > to configure the flags to negotiate.
> > Are you OK with this approach?
>
> Yes, we are fine to have such command to enable and disable the feature
> with default being it disabled if supported by PMD.
> Is default being disabled fine if the feature is supported by a PMD ?
I think the default should be enabled for ease of use.
Do we have similar features disables by default?
I mean do we know features in testpmd which require a "double enablement"
like one configuration command + one rte_flow rule?
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 27, 2023 2:25 PM
> To: Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>
> Cc: Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>;
> Ivan Malov <ivan.malov@oktetlabs.ru>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Hanumanth Reddy Pothula
> <hpothula@marvell.com>; Ferruh Yigit <ferruh.yigit@amd.com>; viacheslavo@nvidia.com;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>; david.marchand@redhat.com
> Subject: Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata
> negotiation
>
> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > Ferruh is proposing to have a command "port config <port_id> ..."
> > > to configure the flags to negotiate.
> > > Are you OK with this approach?
> >
> > Yes, we are fine to have such command to enable and disable the feature
> > with default being it disabled if supported by PMD.
> > Is default being disabled fine if the feature is supported by a PMD ?
>
> I think the default should be enabled for ease of use.
Since testpmd is used extensively for benchmarking purposes, we thought it should have minimum features
enabled by default. The default testpmd doesn't have any Rx/Tx offloads enabled(except for FAST FREE), default
fwd mode being "iofwd" and the Rx metadata is only referenced when dumping packets.
> Do we have similar features disables by default?
> I mean do we know features in testpmd which require a "double enablement"
> like one configuration command + one rte_flow rule?
Spec itself is that way i.e "RTE_FLOW_RULE + RX_METADATA_NEGOTIATE(once)"
Isn't it enough if
#1 We have enough print when rte_flow is being create without negotiation done and ask user to enable rx metadata using
"port config <port_id>..."
#2 Provide testpmd app arg to enable Rx metadata(for example " --rx-metadata") like other features to avoid calling another
command before rte flow create.
>
>
27/01/2023 11:42, Nithin Kumar Dabilpuram:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > Ferruh is proposing to have a command "port config <port_id> ..."
> > > > to configure the flags to negotiate.
> > > > Are you OK with this approach?
> > >
> > > Yes, we are fine to have such command to enable and disable the feature
> > > with default being it disabled if supported by PMD.
> > > Is default being disabled fine if the feature is supported by a PMD ?
> >
> > I think the default should be enabled for ease of use.
>
> Since testpmd is used extensively for benchmarking purposes, we thought it should have minimum features
> enabled by default. The default testpmd doesn't have any Rx/Tx offloads enabled(except for FAST FREE), default
> fwd mode being "iofwd" and the Rx metadata is only referenced when dumping packets.
>
>
> > Do we have similar features disables by default?
> > I mean do we know features in testpmd which require a "double enablement"
> > like one configuration command + one rte_flow rule?
>
> Spec itself is that way i.e "RTE_FLOW_RULE + RX_METADATA_NEGOTIATE(once)"
>
> Isn't it enough if
>
> #1 We have enough print when rte_flow is being create without negotiation done and ask user to enable rx metadata using
> "port config <port_id>..."
> #2 Provide testpmd app arg to enable Rx metadata(for example " --rx-metadata") like other features to avoid calling another
> command before rte flow create.
I'm not sure what is best.
I will let others discuss this part.
On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > Ferruh is proposing to have a command "port config <port_id> ..."
> > > > > to configure the flags to negotiate.
> > > > > Are you OK with this approach?
> > > >
> > > > Yes, we are fine to have such command to enable and disable the feature
> > > > with default being it disabled if supported by PMD.
> > > > Is default being disabled fine if the feature is supported by a PMD ?
> > >
> > > I think the default should be enabled for ease of use.
> >
> > Since testpmd is used extensively for benchmarking purposes, we thought it should have minimum features
> > enabled by default. The default testpmd doesn't have any Rx/Tx offloads enabled(except for FAST FREE), default
> > fwd mode being "iofwd" and the Rx metadata is only referenced when dumping packets.
> >
> >
> > > Do we have similar features disables by default?
> > > I mean do we know features in testpmd which require a "double enablement"
> > > like one configuration command + one rte_flow rule?
> >
> > Spec itself is that way i.e "RTE_FLOW_RULE + RX_METADATA_NEGOTIATE(once)"
> >
> > Isn't it enough if
> >
> > #1 We have enough print when rte_flow is being create without negotiation done and ask user to enable rx metadata using
> > "port config <port_id>..."
> > #2 Provide testpmd app arg to enable Rx metadata(for example " --rx-metadata") like other features to avoid calling another
> > command before rte flow create.
>
> I'm not sure what is best.
> I will let others discuss this part.
IMO, enabling something always defeat the purpose to negotiate. IMO,
someone needs to negotiate
for a feature if the feature is needed. I think, the double enablement
is part of the spec itself. In case, The PMD
drivers won't like double enablement, no need to implement the PMD
callback. That way, there is no change in the existing flow.
The reason why cnxk driver thought of leveraging negotiate() feature
so that it helps for optimization. e.s.p
function template for multiprocess case as we know what features
needed in fastpath upfront.
If there still concerns with patch we can take up this to TB decide
one way or another to make forward progress. Let us know.
>
>
31/01/2023 17:17, Jerin Jacob:
> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > Ferruh is proposing to have a command "port config <port_id> ..."
> > > > > > to configure the flags to negotiate.
> > > > > > Are you OK with this approach?
> > > > >
> > > > > Yes, we are fine to have such command to enable and disable the feature
> > > > > with default being it disabled if supported by PMD.
> > > > > Is default being disabled fine if the feature is supported by a PMD ?
> > > >
> > > > I think the default should be enabled for ease of use.
> > >
> > > Since testpmd is used extensively for benchmarking purposes, we thought it should have minimum features
> > > enabled by default. The default testpmd doesn't have any Rx/Tx offloads enabled(except for FAST FREE), default
> > > fwd mode being "iofwd" and the Rx metadata is only referenced when dumping packets.
> > >
> > >
> > > > Do we have similar features disables by default?
> > > > I mean do we know features in testpmd which require a "double enablement"
> > > > like one configuration command + one rte_flow rule?
> > >
> > > Spec itself is that way i.e "RTE_FLOW_RULE + RX_METADATA_NEGOTIATE(once)"
> > >
> > > Isn't it enough if
> > >
> > > #1 We have enough print when rte_flow is being create without negotiation done and ask user to enable rx metadata using
> > > "port config <port_id>..."
> > > #2 Provide testpmd app arg to enable Rx metadata(for example " --rx-metadata") like other features to avoid calling another
> > > command before rte flow create.
> >
> > I'm not sure what is best.
> > I will let others discuss this part.
>
> IMO, enabling something always defeat the purpose to negotiate. IMO,
> someone needs to negotiate
> for a feature if the feature is needed. I think, the double enablement
> is part of the spec itself. In case, The PMD
> drivers won't like double enablement, no need to implement the PMD
> callback. That way, there is no change in the existing flow.
>
> The reason why cnxk driver thought of leveraging negotiate() feature
> so that it helps for optimization. e.s.p
> function template for multiprocess case as we know what features
> needed in fastpath upfront.
>
> If there still concerns with patch we can take up this to TB decide
> one way or another to make forward progress. Let us know.
Ferruh, Andrew, Ori, Ivan, what is your opinion?
Let's progress with this patch to make it in -rc1.
Hello everyone,
Since making automatic, or implicit, offload decisions does
not belong in testpmd responsibility domain, it should be
safer to avoid calling the "negotiate metadata delivery"
API with some default selection unless the user asks to
do so explicitly, via internal CLI or app options.
With that in mind, port config <port_id> ... sounds OK.
PMDs that support flow primitives which can generate metadata
but, if in started state, can't enable its delivery may pass
appropriate rte_error messages to the user suggesting
they enable delivery of such metadata from NIC to PMD
first. This way, if the person operating testpmd
enters a flow create command and that fails,
they can figure out the inconsistency, stop
the port, negotiate, start and try again.
As for non-debug applications, their developers shall
be properly informed about the problem of enabling
delivery of metadata from NIC to PMD. This way,
they will invoke the negotiate API by default
in their apps, with the feature selection (eg.
MARK) as per nature of the app's business.
This API should indeed be helpful to some PMDs with
regard to collecting upfront knowledge like this.
At the same time, should be benign to those PMDs
who do not need this knowledge and can enable
delivery of metadata right when inserting the
flow rules. So I hope the API does not create
too much discomfort to vendors not needing it.
Thank you.
On Wed, 1 Feb 2023, Thomas Monjalon wrote:
> 31/01/2023 17:17, Jerin Jacob:
>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>
>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>> Ferruh is proposing to have a command "port config <port_id> ..."
>>>>>>> to configure the flags to negotiate.
>>>>>>> Are you OK with this approach?
>>>>>>
>>>>>> Yes, we are fine to have such command to enable and disable the feature
>>>>>> with default being it disabled if supported by PMD.
>>>>>> Is default being disabled fine if the feature is supported by a PMD ?
>>>>>
>>>>> I think the default should be enabled for ease of use.
>>>>
>>>> Since testpmd is used extensively for benchmarking purposes, we thought it should have minimum features
>>>> enabled by default. The default testpmd doesn't have any Rx/Tx offloads enabled(except for FAST FREE), default
>>>> fwd mode being "iofwd" and the Rx metadata is only referenced when dumping packets.
>>>>
>>>>
>>>>> Do we have similar features disables by default?
>>>>> I mean do we know features in testpmd which require a "double enablement"
>>>>> like one configuration command + one rte_flow rule?
>>>>
>>>> Spec itself is that way i.e "RTE_FLOW_RULE + RX_METADATA_NEGOTIATE(once)"
>>>>
>>>> Isn't it enough if
>>>>
>>>> #1 We have enough print when rte_flow is being create without negotiation done and ask user to enable rx metadata using
>>>> "port config <port_id>..."
>>>> #2 Provide testpmd app arg to enable Rx metadata(for example " --rx-metadata") like other features to avoid calling another
>>>> command before rte flow create.
>>>
>>> I'm not sure what is best.
>>> I will let others discuss this part.
>>
>> IMO, enabling something always defeat the purpose to negotiate. IMO,
>> someone needs to negotiate
>> for a feature if the feature is needed. I think, the double enablement
>> is part of the spec itself. In case, The PMD
>> drivers won't like double enablement, no need to implement the PMD
>> callback. That way, there is no change in the existing flow.
>>
>> The reason why cnxk driver thought of leveraging negotiate() feature
>> so that it helps for optimization. e.s.p
>> function template for multiprocess case as we know what features
>> needed in fastpath upfront.
>>
>> If there still concerns with patch we can take up this to TB decide
>> one way or another to make forward progress. Let us know.
>
> Ferruh, Andrew, Ori, Ivan, what is your opinion?
> Let's progress with this patch to make it in -rc1.
>
>
>
On 2/1/23 09:10, Ivan Malov wrote:
> Hello everyone,
>
> Since making automatic, or implicit, offload decisions does
> not belong in testpmd responsibility domain, it should be
> safer to avoid calling the "negotiate metadata delivery"
> API with some default selection unless the user asks to
> do so explicitly, via internal CLI or app options.
>
> With that in mind, port config <port_id> ... sounds OK.
>
> PMDs that support flow primitives which can generate metadata
> but, if in started state, can't enable its delivery may pass
> appropriate rte_error messages to the user suggesting
> they enable delivery of such metadata from NIC to PMD
> first. This way, if the person operating testpmd
> enters a flow create command and that fails,
> they can figure out the inconsistency, stop
> the port, negotiate, start and try again.
>
> As for non-debug applications, their developers shall
> be properly informed about the problem of enabling
> delivery of metadata from NIC to PMD. This way,
> they will invoke the negotiate API by default
> in their apps, with the feature selection (eg.
> MARK) as per nature of the app's business.
>
> This API should indeed be helpful to some PMDs with
> regard to collecting upfront knowledge like this.
> At the same time, should be benign to those PMDs
> who do not need this knowledge and can enable
> delivery of metadata right when inserting the
> flow rules. So I hope the API does not create
> too much discomfort to vendors not needing it.
>
> Thank you.
>
> On Wed, 1 Feb 2023, Thomas Monjalon wrote:
>
>> 31/01/2023 17:17, Jerin Jacob:
>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon <thomas@monjalon.net>
>>> wrote:
>>>>
>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>> Ferruh is proposing to have a command "port config <port_id> ..."
>>>>>>>> to configure the flags to negotiate.
>>>>>>>> Are you OK with this approach?
>>>>>>>
>>>>>>> Yes, we are fine to have such command to enable and disable the
>>>>>>> feature
>>>>>>> with default being it disabled if supported by PMD.
>>>>>>> Is default being disabled fine if the feature is supported by a
>>>>>>> PMD ?
>>>>>>
>>>>>> I think the default should be enabled for ease of use.
>>>>>
>>>>> Since testpmd is used extensively for benchmarking purposes, we
>>>>> thought it should have minimum features
>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
>>>>> offloads enabled(except for FAST FREE), default
>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced when
>>>>> dumping packets.
>>>>>
>>>>>
>>>>>> Do we have similar features disables by default?
>>>>>> I mean do we know features in testpmd which require a "double
>>>>>> enablement"
>>>>>> like one configuration command + one rte_flow rule?
>>>>>
>>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
>>>>> RX_METADATA_NEGOTIATE(once)"
>>>>>
>>>>> Isn't it enough if
>>>>>
>>>>> #1 We have enough print when rte_flow is being create without
>>>>> negotiation done and ask user to enable rx metadata using
>>>>> "port config <port_id>..."
>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
>>>>> --rx-metadata") like other features to avoid calling another
>>>>> command before rte flow create.
>>>>
>>>> I'm not sure what is best.
>>>> I will let others discuss this part.
>>>
>>> IMO, enabling something always defeat the purpose to negotiate. IMO,
>>> someone needs to negotiate
>>> for a feature if the feature is needed. I think, the double enablement
>>> is part of the spec itself. In case, The PMD
>>> drivers won't like double enablement, no need to implement the PMD
>>> callback. That way, there is no change in the existing flow.
>>>
>>> The reason why cnxk driver thought of leveraging negotiate() feature
>>> so that it helps for optimization. e.s.p
>>> function template for multiprocess case as we know what features
>>> needed in fastpath upfront.
>>>
>>> If there still concerns with patch we can take up this to TB decide
>>> one way or another to make forward progress. Let us know.
>>
>> Ferruh, Andrew, Ori, Ivan, what is your opinion?
>> Let's progress with this patch to make it in -rc1.
As I understand all agreed that we need testpmd command to
control negotiated Rx metadata. May be even command-line
option would be useful.
So, remaining question is what should be the default value in
testpmd. Note that it is just testpmd question since default
value in an abstract application is nothing negotiated
(if I'm not mistaken).
The key advantage of the current behaviour is to avoid
"double-enabling" in testpmd. It preserves behaviour which
we had before before the API addition. It is a strong
argument. Basically it puts the feature into the same
basket as FAST_FREE - need an action to run faster.
I see no problem in such approach.
The key disadvantage is the difference in testpmd and
other applications default behaviour.
I'd look at the feature in the following way:
if an application theoretically wants to use
USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
corresponding Rx metadata to ensure that the feature is
available and HW is informed that application may need it.
Since testpmd supports corresponding flow API actions and
flow tunnels, it tries to negotiate it by default, but do
not fail if the negotiation fails.
So, I'd would vote to keeping the default value as is.
On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
>
> On 2/1/23 09:10, Ivan Malov wrote:
> > Hello everyone,
> >
> > Since making automatic, or implicit, offload decisions does
> > not belong in testpmd responsibility domain, it should be
> > safer to avoid calling the "negotiate metadata delivery"
> > API with some default selection unless the user asks to
> > do so explicitly, via internal CLI or app options.
> >
> > With that in mind, port config <port_id> ... sounds OK.
> >
> > PMDs that support flow primitives which can generate metadata
> > but, if in started state, can't enable its delivery may pass
> > appropriate rte_error messages to the user suggesting
> > they enable delivery of such metadata from NIC to PMD
> > first. This way, if the person operating testpmd
> > enters a flow create command and that fails,
> > they can figure out the inconsistency, stop
> > the port, negotiate, start and try again.
> >
> > As for non-debug applications, their developers shall
> > be properly informed about the problem of enabling
> > delivery of metadata from NIC to PMD. This way,
> > they will invoke the negotiate API by default
> > in their apps, with the feature selection (eg.
> > MARK) as per nature of the app's business.
> >
> > This API should indeed be helpful to some PMDs with
> > regard to collecting upfront knowledge like this.
> > At the same time, should be benign to those PMDs
> > who do not need this knowledge and can enable
> > delivery of metadata right when inserting the
> > flow rules. So I hope the API does not create
> > too much discomfort to vendors not needing it.
> >
> > Thank you.
> >
> > On Wed, 1 Feb 2023, Thomas Monjalon wrote:
> >
> >> 31/01/2023 17:17, Jerin Jacob:
> >>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon <thomas@monjalon.net>
> >>> wrote:
> >>>>
> >>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> >>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> >>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>> Ferruh is proposing to have a command "port config <port_id> ..."
> >>>>>>>> to configure the flags to negotiate.
> >>>>>>>> Are you OK with this approach?
> >>>>>>>
> >>>>>>> Yes, we are fine to have such command to enable and disable the
> >>>>>>> feature
> >>>>>>> with default being it disabled if supported by PMD.
> >>>>>>> Is default being disabled fine if the feature is supported by a
> >>>>>>> PMD ?
> >>>>>>
> >>>>>> I think the default should be enabled for ease of use.
> >>>>>
> >>>>> Since testpmd is used extensively for benchmarking purposes, we
> >>>>> thought it should have minimum features
> >>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
> >>>>> offloads enabled(except for FAST FREE), default
> >>>>> fwd mode being "iofwd" and the Rx metadata is only referenced when
> >>>>> dumping packets.
> >>>>>
> >>>>>
> >>>>>> Do we have similar features disables by default?
> >>>>>> I mean do we know features in testpmd which require a "double
> >>>>>> enablement"
> >>>>>> like one configuration command + one rte_flow rule?
> >>>>>
> >>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
> >>>>> RX_METADATA_NEGOTIATE(once)"
> >>>>>
> >>>>> Isn't it enough if
> >>>>>
> >>>>> #1 We have enough print when rte_flow is being create without
> >>>>> negotiation done and ask user to enable rx metadata using
> >>>>> "port config <port_id>..."
> >>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
> >>>>> --rx-metadata") like other features to avoid calling another
> >>>>> command before rte flow create.
> >>>>
> >>>> I'm not sure what is best.
> >>>> I will let others discuss this part.
> >>>
> >>> IMO, enabling something always defeat the purpose to negotiate. IMO,
> >>> someone needs to negotiate
> >>> for a feature if the feature is needed. I think, the double enablement
> >>> is part of the spec itself. In case, The PMD
> >>> drivers won't like double enablement, no need to implement the PMD
> >>> callback. That way, there is no change in the existing flow.
> >>>
> >>> The reason why cnxk driver thought of leveraging negotiate() feature
> >>> so that it helps for optimization. e.s.p
> >>> function template for multiprocess case as we know what features
> >>> needed in fastpath upfront.
> >>>
> >>> If there still concerns with patch we can take up this to TB decide
> >>> one way or another to make forward progress. Let us know.
> >>
> >> Ferruh, Andrew, Ori, Ivan, what is your opinion?
> >> Let's progress with this patch to make it in -rc1.
>
> As I understand all agreed that we need testpmd command to
> control negotiated Rx metadata. May be even command-line
> option would be useful.
>
> So, remaining question is what should be the default value in
> testpmd. Note that it is just testpmd question since default
> value in an abstract application is nothing negotiated
> (if I'm not mistaken).
>
> The key advantage of the current behaviour is to avoid
> "double-enabling" in testpmd. It preserves behaviour which
> we had before before the API addition. It is a strong
> argument. Basically it puts the feature into the same
> basket as FAST_FREE - need an action to run faster.
I think, there is a disconnect here. FAST_FREE is enabled by default.
i.e We don't need any specific action to run faster. To align with performance
test application, by default the configuration should be run faster. User
needs to give explicit configuration to allow more offload or the one causes
the mpps drops. IMO, That is the theme followed in testpmd.
> I see no problem in such approach.
>
> The key disadvantage is the difference in testpmd and
> other applications default behaviour.
>
> I'd look at the feature in the following way:
> if an application theoretically wants to use
> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
> corresponding Rx metadata to ensure that the feature is
> available and HW is informed that application may need it.
> Since testpmd supports corresponding flow API actions and
> flow tunnels, it tries to negotiate it by default, but do
> not fail if the negotiation fails.
>
> So, I'd would vote to keeping the default value as is.
>
Hi all,
Sorry for jumping in late,
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, 1 February 2023 10:53
>
> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
> >
> > On 2/1/23 09:10, Ivan Malov wrote:
> > > Hello everyone,
> > >
> > > Since making automatic, or implicit, offload decisions does
> > > not belong in testpmd responsibility domain, it should be
> > > safer to avoid calling the "negotiate metadata delivery"
> > > API with some default selection unless the user asks to
> > > do so explicitly, via internal CLI or app options.
> > >
> > > With that in mind, port config <port_id> ... sounds OK.
> > >
> > > PMDs that support flow primitives which can generate metadata
> > > but, if in started state, can't enable its delivery may pass
> > > appropriate rte_error messages to the user suggesting
> > > they enable delivery of such metadata from NIC to PMD
> > > first. This way, if the person operating testpmd
> > > enters a flow create command and that fails,
> > > they can figure out the inconsistency, stop
> > > the port, negotiate, start and try again.
> > >
> > > As for non-debug applications, their developers shall
> > > be properly informed about the problem of enabling
> > > delivery of metadata from NIC to PMD. This way,
> > > they will invoke the negotiate API by default
> > > in their apps, with the feature selection (eg.
> > > MARK) as per nature of the app's business.
> > >
> > > This API should indeed be helpful to some PMDs with
> > > regard to collecting upfront knowledge like this.
> > > At the same time, should be benign to those PMDs
> > > who do not need this knowledge and can enable
> > > delivery of metadata right when inserting the
> > > flow rules. So I hope the API does not create
> > > too much discomfort to vendors not needing it.
> > >
> > > Thank you.
> > >
> > > On Wed, 1 Feb 2023, Thomas Monjalon wrote:
> > >
> > >> 31/01/2023 17:17, Jerin Jacob:
> > >>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
> <thomas@monjalon.net>
> > >>> wrote:
> > >>>>
> > >>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> > >>>>> From: Thomas Monjalon <thomas@monjalon.net>
> > >>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> > >>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> > >>>>>>>> Ferruh is proposing to have a command "port config <port_id>
> ..."
> > >>>>>>>> to configure the flags to negotiate.
> > >>>>>>>> Are you OK with this approach?
> > >>>>>>>
> > >>>>>>> Yes, we are fine to have such command to enable and disable the
> > >>>>>>> feature
> > >>>>>>> with default being it disabled if supported by PMD.
> > >>>>>>> Is default being disabled fine if the feature is supported by a
> > >>>>>>> PMD ?
> > >>>>>>
> > >>>>>> I think the default should be enabled for ease of use.
> > >>>>>
> > >>>>> Since testpmd is used extensively for benchmarking purposes, we
> > >>>>> thought it should have minimum features
> > >>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
> > >>>>> offloads enabled(except for FAST FREE), default
> > >>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
> when
> > >>>>> dumping packets.
> > >>>>>
> > >>>>>
> > >>>>>> Do we have similar features disables by default?
> > >>>>>> I mean do we know features in testpmd which require a "double
> > >>>>>> enablement"
> > >>>>>> like one configuration command + one rte_flow rule?
> > >>>>>
> > >>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
> > >>>>> RX_METADATA_NEGOTIATE(once)"
> > >>>>>
> > >>>>> Isn't it enough if
> > >>>>>
> > >>>>> #1 We have enough print when rte_flow is being create without
> > >>>>> negotiation done and ask user to enable rx metadata using
> > >>>>> "port config <port_id>..."
> > >>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
> > >>>>> --rx-metadata") like other features to avoid calling another
> > >>>>> command before rte flow create.
> > >>>>
> > >>>> I'm not sure what is best.
> > >>>> I will let others discuss this part.
> > >>>
> > >>> IMO, enabling something always defeat the purpose to negotiate. IMO,
> > >>> someone needs to negotiate
> > >>> for a feature if the feature is needed. I think, the double enablement
> > >>> is part of the spec itself. In case, The PMD
> > >>> drivers won't like double enablement, no need to implement the PMD
> > >>> callback. That way, there is no change in the existing flow.
> > >>>
> > >>> The reason why cnxk driver thought of leveraging negotiate() feature
> > >>> so that it helps for optimization. e.s.p
> > >>> function template for multiprocess case as we know what features
> > >>> needed in fastpath upfront.
> > >>>
> > >>> If there still concerns with patch we can take up this to TB decide
> > >>> one way or another to make forward progress. Let us know.
> > >>
> > >> Ferruh, Andrew, Ori, Ivan, what is your opinion?
> > >> Let's progress with this patch to make it in -rc1.
> >
> > As I understand all agreed that we need testpmd command to
> > control negotiated Rx metadata. May be even command-line
> > option would be useful.
> >
> > So, remaining question is what should be the default value in
> > testpmd. Note that it is just testpmd question since default
> > value in an abstract application is nothing negotiated
> > (if I'm not mistaken).
> >
> > The key advantaan ge of the current behaviour is to avoid
> > "double-enabling" in testpmd. It preserves behaviour which
> > we had before before the API addition. It is a strong
> > argument. Basically it puts the feature into the same
> > basket as FAST_FREE - need an action to run faster.
>
> I think, there is a disconnect here. FAST_FREE is enabled by default.
> i.e We don't need any specific action to run faster. To align with performance
> test application, by default the configuration should be run faster. User
> needs to give explicit configuration to allow more offload or the one causes
> the mpps drops. IMO, That is the theme followed in testpmd.
>
>
I agree with Andrew, the default should stay the same, as now, PMD may already implement
logic to only enable the feature if there is a flow rule.
Changing the default will result in breaking applications.
I want to suggest new approach for this feature,
maybe we can use the rte_flow_configure, and add a new bit that says if those
actions are going to be used.
What do you think?
> > I see no problem in such approach.
> >
> > The key disadvantage is the difference in testpmd and
> > other applications default behaviour.
> >
> > I'd look at the feature in the following way:
> > if an application theoretically wants to use
> > USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
> > corresponding Rx metadata to ensure that the feature is
> > available and HW is informed that application may need it.
> > Since testpmd supports corresponding flow API actions and
> > flow tunnels, it tries to negotiate it by default, but do
> > not fail if the negotiation fails.
> >
> > So, I'd would vote to keeping the default value as is.
> >
01/02/2023 10:00, Ori Kam:
> Hi all,
>
> Sorry for jumping in late,
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Wednesday, 1 February 2023 10:53
> >
> > On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru> wrote:
> > >
> > > On 2/1/23 09:10, Ivan Malov wrote:
> > > > Hello everyone,
> > > >
> > > > Since making automatic, or implicit, offload decisions does
> > > > not belong in testpmd responsibility domain, it should be
> > > > safer to avoid calling the "negotiate metadata delivery"
> > > > API with some default selection unless the user asks to
> > > > do so explicitly, via internal CLI or app options.
> > > >
> > > > With that in mind, port config <port_id> ... sounds OK.
> > > >
> > > > PMDs that support flow primitives which can generate metadata
> > > > but, if in started state, can't enable its delivery may pass
> > > > appropriate rte_error messages to the user suggesting
> > > > they enable delivery of such metadata from NIC to PMD
> > > > first. This way, if the person operating testpmd
> > > > enters a flow create command and that fails,
> > > > they can figure out the inconsistency, stop
> > > > the port, negotiate, start and try again.
> > > >
> > > > As for non-debug applications, their developers shall
> > > > be properly informed about the problem of enabling
> > > > delivery of metadata from NIC to PMD. This way,
> > > > they will invoke the negotiate API by default
> > > > in their apps, with the feature selection (eg.
> > > > MARK) as per nature of the app's business.
> > > >
> > > > This API should indeed be helpful to some PMDs with
> > > > regard to collecting upfront knowledge like this.
> > > > At the same time, should be benign to those PMDs
> > > > who do not need this knowledge and can enable
> > > > delivery of metadata right when inserting the
> > > > flow rules. So I hope the API does not create
> > > > too much discomfort to vendors not needing it.
> > > >
> > > > Thank you.
> > > >
> > > > On Wed, 1 Feb 2023, Thomas Monjalon wrote:
> > > >
> > > >> 31/01/2023 17:17, Jerin Jacob:
> > > >>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
> > <thomas@monjalon.net>
> > > >>> wrote:
> > > >>>>
> > > >>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> > > >>>>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> > > >>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>>>>>> Ferruh is proposing to have a command "port config <port_id>
> > ..."
> > > >>>>>>>> to configure the flags to negotiate.
> > > >>>>>>>> Are you OK with this approach?
> > > >>>>>>>
> > > >>>>>>> Yes, we are fine to have such command to enable and disable the
> > > >>>>>>> feature
> > > >>>>>>> with default being it disabled if supported by PMD.
> > > >>>>>>> Is default being disabled fine if the feature is supported by a
> > > >>>>>>> PMD ?
> > > >>>>>>
> > > >>>>>> I think the default should be enabled for ease of use.
> > > >>>>>
> > > >>>>> Since testpmd is used extensively for benchmarking purposes, we
> > > >>>>> thought it should have minimum features
> > > >>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
> > > >>>>> offloads enabled(except for FAST FREE), default
> > > >>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
> > when
> > > >>>>> dumping packets.
> > > >>>>>
> > > >>>>>
> > > >>>>>> Do we have similar features disables by default?
> > > >>>>>> I mean do we know features in testpmd which require a "double
> > > >>>>>> enablement"
> > > >>>>>> like one configuration command + one rte_flow rule?
> > > >>>>>
> > > >>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
> > > >>>>> RX_METADATA_NEGOTIATE(once)"
> > > >>>>>
> > > >>>>> Isn't it enough if
> > > >>>>>
> > > >>>>> #1 We have enough print when rte_flow is being create without
> > > >>>>> negotiation done and ask user to enable rx metadata using
> > > >>>>> "port config <port_id>..."
> > > >>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
> > > >>>>> --rx-metadata") like other features to avoid calling another
> > > >>>>> command before rte flow create.
> > > >>>>
> > > >>>> I'm not sure what is best.
> > > >>>> I will let others discuss this part.
> > > >>>
> > > >>> IMO, enabling something always defeat the purpose to negotiate. IMO,
> > > >>> someone needs to negotiate
> > > >>> for a feature if the feature is needed. I think, the double enablement
> > > >>> is part of the spec itself. In case, The PMD
> > > >>> drivers won't like double enablement, no need to implement the PMD
> > > >>> callback. That way, there is no change in the existing flow.
> > > >>>
> > > >>> The reason why cnxk driver thought of leveraging negotiate() feature
> > > >>> so that it helps for optimization. e.s.p
> > > >>> function template for multiprocess case as we know what features
> > > >>> needed in fastpath upfront.
> > > >>>
> > > >>> If there still concerns with patch we can take up this to TB decide
> > > >>> one way or another to make forward progress. Let us know.
> > > >>
> > > >> Ferruh, Andrew, Ori, Ivan, what is your opinion?
> > > >> Let's progress with this patch to make it in -rc1.
> > >
> > > As I understand all agreed that we need testpmd command to
> > > control negotiated Rx metadata. May be even command-line
> > > option would be useful.
> > >
> > > So, remaining question is what should be the default value in
> > > testpmd. Note that it is just testpmd question since default
> > > value in an abstract application is nothing negotiated
> > > (if I'm not mistaken).
> > >
> > > The key advantaan ge of the current behaviour is to avoid
> > > "double-enabling" in testpmd. It preserves behaviour which
> > > we had before before the API addition. It is a strong
> > > argument. Basically it puts the feature into the same
> > > basket as FAST_FREE - need an action to run faster.
> >
> > I think, there is a disconnect here. FAST_FREE is enabled by default.
> > i.e We don't need any specific action to run faster. To align with performance
> > test application, by default the configuration should be run faster. User
> > needs to give explicit configuration to allow more offload or the one causes
> > the mpps drops. IMO, That is the theme followed in testpmd.
> >
> >
> I agree with Andrew, the default should stay the same, as now, PMD may already implement
> logic to only enable the feature if there is a flow rule.
> Changing the default will result in breaking applications.
That's not what is discussed here.
We are talking only about testpmd default.
> I want to suggest new approach for this feature,
> maybe we can use the rte_flow_configure, and add a new bit that says if those
> actions are going to be used.
> What do you think?
Let's not change the API please.
> > > I see no problem in such approach.
> > >
> > > The key disadvantage is the difference in testpmd and
> > > other applications default behaviour.
> > >
> > > I'd look at the feature in the following way:
> > > if an application theoretically wants to use
> > > USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
> > > corresponding Rx metadata to ensure that the feature is
> > > available and HW is informed that application may need it.
> > > Since testpmd supports corresponding flow API actions and
> > > flow tunnels, it tries to negotiate it by default, but do
> > > not fail if the negotiation fails.
> > >
> > > So, I'd would vote to keeping the default value as is.
> > >
>
On 2/1/23 12:05, Thomas Monjalon wrote:
> 01/02/2023 10:00, Ori Kam:
>> Hi all,
>>
>> Sorry for jumping in late,
>>
>>> -----Original Message-----
>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>> Sent: Wednesday, 1 February 2023 10:53
>>>
>>> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>
>>>> On 2/1/23 09:10, Ivan Malov wrote:
>>>>> Hello everyone,
>>>>>
>>>>> Since making automatic, or implicit, offload decisions does
>>>>> not belong in testpmd responsibility domain, it should be
>>>>> safer to avoid calling the "negotiate metadata delivery"
>>>>> API with some default selection unless the user asks to
>>>>> do so explicitly, via internal CLI or app options.
>>>>>
>>>>> With that in mind, port config <port_id> ... sounds OK.
>>>>>
>>>>> PMDs that support flow primitives which can generate metadata
>>>>> but, if in started state, can't enable its delivery may pass
>>>>> appropriate rte_error messages to the user suggesting
>>>>> they enable delivery of such metadata from NIC to PMD
>>>>> first. This way, if the person operating testpmd
>>>>> enters a flow create command and that fails,
>>>>> they can figure out the inconsistency, stop
>>>>> the port, negotiate, start and try again.
>>>>>
>>>>> As for non-debug applications, their developers shall
>>>>> be properly informed about the problem of enabling
>>>>> delivery of metadata from NIC to PMD. This way,
>>>>> they will invoke the negotiate API by default
>>>>> in their apps, with the feature selection (eg.
>>>>> MARK) as per nature of the app's business.
>>>>>
>>>>> This API should indeed be helpful to some PMDs with
>>>>> regard to collecting upfront knowledge like this.
>>>>> At the same time, should be benign to those PMDs
>>>>> who do not need this knowledge and can enable
>>>>> delivery of metadata right when inserting the
>>>>> flow rules. So I hope the API does not create
>>>>> too much discomfort to vendors not needing it.
>>>>>
>>>>> Thank you.
>>>>>
>>>>> On Wed, 1 Feb 2023, Thomas Monjalon wrote:
>>>>>
>>>>>> 31/01/2023 17:17, Jerin Jacob:
>>>>>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
>>> <thomas@monjalon.net>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>>> Ferruh is proposing to have a command "port config <port_id>
>>> ..."
>>>>>>>>>>>> to configure the flags to negotiate.
>>>>>>>>>>>> Are you OK with this approach?
>>>>>>>>>>>
>>>>>>>>>>> Yes, we are fine to have such command to enable and disable the
>>>>>>>>>>> feature
>>>>>>>>>>> with default being it disabled if supported by PMD.
>>>>>>>>>>> Is default being disabled fine if the feature is supported by a
>>>>>>>>>>> PMD ?
>>>>>>>>>>
>>>>>>>>>> I think the default should be enabled for ease of use.
>>>>>>>>>
>>>>>>>>> Since testpmd is used extensively for benchmarking purposes, we
>>>>>>>>> thought it should have minimum features
>>>>>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
>>>>>>>>> offloads enabled(except for FAST FREE), default
>>>>>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
>>> when
>>>>>>>>> dumping packets.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Do we have similar features disables by default?
>>>>>>>>>> I mean do we know features in testpmd which require a "double
>>>>>>>>>> enablement"
>>>>>>>>>> like one configuration command + one rte_flow rule?
>>>>>>>>>
>>>>>>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
>>>>>>>>> RX_METADATA_NEGOTIATE(once)"
>>>>>>>>>
>>>>>>>>> Isn't it enough if
>>>>>>>>>
>>>>>>>>> #1 We have enough print when rte_flow is being create without
>>>>>>>>> negotiation done and ask user to enable rx metadata using
>>>>>>>>> "port config <port_id>..."
>>>>>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
>>>>>>>>> --rx-metadata") like other features to avoid calling another
>>>>>>>>> command before rte flow create.
>>>>>>>>
>>>>>>>> I'm not sure what is best.
>>>>>>>> I will let others discuss this part.
>>>>>>>
>>>>>>> IMO, enabling something always defeat the purpose to negotiate. IMO,
>>>>>>> someone needs to negotiate
>>>>>>> for a feature if the feature is needed. I think, the double enablement
>>>>>>> is part of the spec itself. In case, The PMD
>>>>>>> drivers won't like double enablement, no need to implement the PMD
>>>>>>> callback. That way, there is no change in the existing flow.
>>>>>>>
>>>>>>> The reason why cnxk driver thought of leveraging negotiate() feature
>>>>>>> so that it helps for optimization. e.s.p
>>>>>>> function template for multiprocess case as we know what features
>>>>>>> needed in fastpath upfront.
>>>>>>>
>>>>>>> If there still concerns with patch we can take up this to TB decide
>>>>>>> one way or another to make forward progress. Let us know.
>>>>>>
>>>>>> Ferruh, Andrew, Ori, Ivan, what is your opinion?
>>>>>> Let's progress with this patch to make it in -rc1.
>>>>
>>>> As I understand all agreed that we need testpmd command to
>>>> control negotiated Rx metadata. May be even command-line
>>>> option would be useful.
>>>>
>>>> So, remaining question is what should be the default value in
>>>> testpmd. Note that it is just testpmd question since default
>>>> value in an abstract application is nothing negotiated
>>>> (if I'm not mistaken).
>>>>
>>>> The key advantaan ge of the current behaviour is to avoid
>>>> "double-enabling" in testpmd. It preserves behaviour which
>>>> we had before before the API addition. It is a strong
>>>> argument. Basically it puts the feature into the same
>>>> basket as FAST_FREE - need an action to run faster.
>>>
>>> I think, there is a disconnect here. FAST_FREE is enabled by default.
Sorry, I'm lost here. Don't we need to enable FAST_FREE
offload? As far as I know all offloads are disabled by default.
>>> i.e We don't need any specific action to run faster. To align with performance
>>> test application, by default the configuration should be run faster. User
>>> needs to give explicit configuration to allow more offload or the one causes
>>> the mpps drops. IMO, That is the theme followed in testpmd.
>>>
>>>
>> I agree with Andrew, the default should stay the same, as now, PMD may already implement
>> logic to only enable the feature if there is a flow rule.
>> Changing the default will result in breaking applications.
>
> That's not what is discussed here.
> We are talking only about testpmd default.
>
>> I want to suggest new approach for this feature,
>> maybe we can use the rte_flow_configure, and add a new bit that says if those
>> actions are going to be used.
>> What do you think?
>
> Let's not change the API please.
>
>
>>>> I see no problem in such approach.
>>>>
>>>> The key disadvantage is the difference in testpmd and
>>>> other applications default behaviour.
>>>>
>>>> I'd look at the feature in the following way:
>>>> if an application theoretically wants to use
>>>> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
>>>> corresponding Rx metadata to ensure that the feature is
>>>> available and HW is informed that application may need it.
>>>> Since testpmd supports corresponding flow API actions and
>>>> flow tunnels, it tries to negotiate it by default, but do
>>>> not fail if the negotiation fails.
>>>>
>>>> So, I'd would vote to keeping the default value as is.
>>>>
>>
>
>
>
>
>
On Wed, Feb 1, 2023 at 2:37 PM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
>
> On 2/1/23 12:05, Thomas Monjalon wrote:
> > 01/02/2023 10:00, Ori Kam:
> >> Hi all,
> >>
> >> Sorry for jumping in late,
> >>
> >>> -----Original Message-----
> >>> From: Jerin Jacob <jerinjacobk@gmail.com>
> >>> Sent: Wednesday, 1 February 2023 10:53
> >>>
> >>> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
> >>> <andrew.rybchenko@oktetlabs.ru> wrote:
> >>>>
> >>>> On 2/1/23 09:10, Ivan Malov wrote:
> >>>>> Hello everyone,
> >>>>>
> >>>>> Since making automatic, or implicit, offload decisions does
> >>>>> not belong in testpmd responsibility domain, it should be
> >>>>> safer to avoid calling the "negotiate metadata delivery"
> >>>>> API with some default selection unless the user asks to
> >>>>> do so explicitly, via internal CLI or app options.
> >>>>>
> >>>>> With that in mind, port config <port_id> ... sounds OK.
> >>>>>
> >>>>> PMDs that support flow primitives which can generate metadata
> >>>>> but, if in started state, can't enable its delivery may pass
> >>>>> appropriate rte_error messages to the user suggesting
> >>>>> they enable delivery of such metadata from NIC to PMD
> >>>>> first. This way, if the person operating testpmd
> >>>>> enters a flow create command and that fails,
> >>>>> they can figure out the inconsistency, stop
> >>>>> the port, negotiate, start and try again.
> >>>>>
> >>>>> As for non-debug applications, their developers shall
> >>>>> be properly informed about the problem of enabling
> >>>>> delivery of metadata from NIC to PMD. This way,
> >>>>> they will invoke the negotiate API by default
> >>>>> in their apps, with the feature selection (eg.
> >>>>> MARK) as per nature of the app's business.
> >>>>>
> >>>>> This API should indeed be helpful to some PMDs with
> >>>>> regard to collecting upfront knowledge like this.
> >>>>> At the same time, should be benign to those PMDs
> >>>>> who do not need this knowledge and can enable
> >>>>> delivery of metadata right when inserting the
> >>>>> flow rules. So I hope the API does not create
> >>>>> too much discomfort to vendors not needing it.
> >>>>>
> >>>>> Thank you.
> >>>>>
> >>>>> On Wed, 1 Feb 2023, Thomas Monjalon wrote:
> >>>>>
> >>>>>> 31/01/2023 17:17, Jerin Jacob:
> >>>>>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
> >>> <thomas@monjalon.net>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> >>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> >>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>>>>> Ferruh is proposing to have a command "port config <port_id>
> >>> ..."
> >>>>>>>>>>>> to configure the flags to negotiate.
> >>>>>>>>>>>> Are you OK with this approach?
> >>>>>>>>>>>
> >>>>>>>>>>> Yes, we are fine to have such command to enable and disable the
> >>>>>>>>>>> feature
> >>>>>>>>>>> with default being it disabled if supported by PMD.
> >>>>>>>>>>> Is default being disabled fine if the feature is supported by a
> >>>>>>>>>>> PMD ?
> >>>>>>>>>>
> >>>>>>>>>> I think the default should be enabled for ease of use.
> >>>>>>>>>
> >>>>>>>>> Since testpmd is used extensively for benchmarking purposes, we
> >>>>>>>>> thought it should have minimum features
> >>>>>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
> >>>>>>>>> offloads enabled(except for FAST FREE), default
> >>>>>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
> >>> when
> >>>>>>>>> dumping packets.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Do we have similar features disables by default?
> >>>>>>>>>> I mean do we know features in testpmd which require a "double
> >>>>>>>>>> enablement"
> >>>>>>>>>> like one configuration command + one rte_flow rule?
> >>>>>>>>>
> >>>>>>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
> >>>>>>>>> RX_METADATA_NEGOTIATE(once)"
> >>>>>>>>>
> >>>>>>>>> Isn't it enough if
> >>>>>>>>>
> >>>>>>>>> #1 We have enough print when rte_flow is being create without
> >>>>>>>>> negotiation done and ask user to enable rx metadata using
> >>>>>>>>> "port config <port_id>..."
> >>>>>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
> >>>>>>>>> --rx-metadata") like other features to avoid calling another
> >>>>>>>>> command before rte flow create.
> >>>>>>>>
> >>>>>>>> I'm not sure what is best.
> >>>>>>>> I will let others discuss this part.
> >>>>>>>
> >>>>>>> IMO, enabling something always defeat the purpose to negotiate. IMO,
> >>>>>>> someone needs to negotiate
> >>>>>>> for a feature if the feature is needed. I think, the double enablement
> >>>>>>> is part of the spec itself. In case, The PMD
> >>>>>>> drivers won't like double enablement, no need to implement the PMD
> >>>>>>> callback. That way, there is no change in the existing flow.
> >>>>>>>
> >>>>>>> The reason why cnxk driver thought of leveraging negotiate() feature
> >>>>>>> so that it helps for optimization. e.s.p
> >>>>>>> function template for multiprocess case as we know what features
> >>>>>>> needed in fastpath upfront.
> >>>>>>>
> >>>>>>> If there still concerns with patch we can take up this to TB decide
> >>>>>>> one way or another to make forward progress. Let us know.
> >>>>>>
> >>>>>> Ferruh, Andrew, Ori, Ivan, what is your opinion?
> >>>>>> Let's progress with this patch to make it in -rc1.
> >>>>
> >>>> As I understand all agreed that we need testpmd command to
> >>>> control negotiated Rx metadata. May be even command-line
> >>>> option would be useful.
> >>>>
> >>>> So, remaining question is what should be the default value in
> >>>> testpmd. Note that it is just testpmd question since default
> >>>> value in an abstract application is nothing negotiated
> >>>> (if I'm not mistaken).
> >>>>
> >>>> The key advantaan ge of the current behaviour is to avoid
> >>>> "double-enabling" in testpmd. It preserves behaviour which
> >>>> we had before before the API addition. It is a strong
> >>>> argument. Basically it puts the feature into the same
> >>>> basket as FAST_FREE - need an action to run faster.
> >>>
> >>> I think, there is a disconnect here. FAST_FREE is enabled by default.
>
> Sorry, I'm lost here. Don't we need to enable FAST_FREE
> offload? As far as I know all offloads are disabled by default.
Not the case for FAST_FREE as disabling needs "more cycles on processor" side.
See app/test-pmd/testpmd.c
/*
* Ethernet device configuration.
*/
struct rte_eth_rxmode rx_mode;
struct rte_eth_txmode tx_mode = {
.offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE,
};
>
> >>> i.e We don't need any specific action to run faster. To align with performance
> >>> test application, by default the configuration should be run faster. User
> >>> needs to give explicit configuration to allow more offload or the one causes
> >>> the mpps drops. IMO, That is the theme followed in testpmd.
> >>>
> >>>
> >> I agree with Andrew, the default should stay the same, as now, PMD may already implement
> >> logic to only enable the feature if there is a flow rule.
> >> Changing the default will result in breaking applications.
> >
> > That's not what is discussed here.
> > We are talking only about testpmd default.
> >
> >> I want to suggest new approach for this feature,
> >> maybe we can use the rte_flow_configure, and add a new bit that says if those
> >> actions are going to be used.
> >> What do you think?
> >
> > Let's not change the API please.
> >
> >
> >>>> I see no problem in such approach.
> >>>>
> >>>> The key disadvantage is the difference in testpmd and
> >>>> other applications default behaviour.
> >>>>
> >>>> I'd look at the feature in the following way:
> >>>> if an application theoretically wants to use
> >>>> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
> >>>> corresponding Rx metadata to ensure that the feature is
> >>>> available and HW is informed that application may need it.
> >>>> Since testpmd supports corresponding flow API actions and
> >>>> flow tunnels, it tries to negotiate it by default, but do
> >>>> not fail if the negotiation fails.
> >>>>
> >>>> So, I'd would vote to keeping the default value as is.
> >>>>
> >>
> >
> >
> >
> >
> >
>
On 2/1/23 12:14, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 2:37 PM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
>>
>> On 2/1/23 12:05, Thomas Monjalon wrote:
>>> 01/02/2023 10:00, Ori Kam:
>>>> Hi all,
>>>>
>>>> Sorry for jumping in late,
>>>>
>>>>> -----Original Message-----
>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>> Sent: Wednesday, 1 February 2023 10:53
>>>>>
>>>>> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>
>>>>>> On 2/1/23 09:10, Ivan Malov wrote:
>>>>>>> Hello everyone,
>>>>>>>
>>>>>>> Since making automatic, or implicit, offload decisions does
>>>>>>> not belong in testpmd responsibility domain, it should be
>>>>>>> safer to avoid calling the "negotiate metadata delivery"
>>>>>>> API with some default selection unless the user asks to
>>>>>>> do so explicitly, via internal CLI or app options.
>>>>>>>
>>>>>>> With that in mind, port config <port_id> ... sounds OK.
>>>>>>>
>>>>>>> PMDs that support flow primitives which can generate metadata
>>>>>>> but, if in started state, can't enable its delivery may pass
>>>>>>> appropriate rte_error messages to the user suggesting
>>>>>>> they enable delivery of such metadata from NIC to PMD
>>>>>>> first. This way, if the person operating testpmd
>>>>>>> enters a flow create command and that fails,
>>>>>>> they can figure out the inconsistency, stop
>>>>>>> the port, negotiate, start and try again.
>>>>>>>
>>>>>>> As for non-debug applications, their developers shall
>>>>>>> be properly informed about the problem of enabling
>>>>>>> delivery of metadata from NIC to PMD. This way,
>>>>>>> they will invoke the negotiate API by default
>>>>>>> in their apps, with the feature selection (eg.
>>>>>>> MARK) as per nature of the app's business.
>>>>>>>
>>>>>>> This API should indeed be helpful to some PMDs with
>>>>>>> regard to collecting upfront knowledge like this.
>>>>>>> At the same time, should be benign to those PMDs
>>>>>>> who do not need this knowledge and can enable
>>>>>>> delivery of metadata right when inserting the
>>>>>>> flow rules. So I hope the API does not create
>>>>>>> too much discomfort to vendors not needing it.
>>>>>>>
>>>>>>> Thank you.
>>>>>>>
>>>>>>> On Wed, 1 Feb 2023, Thomas Monjalon wrote:
>>>>>>>
>>>>>>>> 31/01/2023 17:17, Jerin Jacob:
>>>>>>>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
>>>>> <thomas@monjalon.net>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
>>>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>>>>> Ferruh is proposing to have a command "port config <port_id>
>>>>> ..."
>>>>>>>>>>>>>> to configure the flags to negotiate.
>>>>>>>>>>>>>> Are you OK with this approach?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, we are fine to have such command to enable and disable the
>>>>>>>>>>>>> feature
>>>>>>>>>>>>> with default being it disabled if supported by PMD.
>>>>>>>>>>>>> Is default being disabled fine if the feature is supported by a
>>>>>>>>>>>>> PMD ?
>>>>>>>>>>>>
>>>>>>>>>>>> I think the default should be enabled for ease of use.
>>>>>>>>>>>
>>>>>>>>>>> Since testpmd is used extensively for benchmarking purposes, we
>>>>>>>>>>> thought it should have minimum features
>>>>>>>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
>>>>>>>>>>> offloads enabled(except for FAST FREE), default
>>>>>>>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
>>>>> when
>>>>>>>>>>> dumping packets.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Do we have similar features disables by default?
>>>>>>>>>>>> I mean do we know features in testpmd which require a "double
>>>>>>>>>>>> enablement"
>>>>>>>>>>>> like one configuration command + one rte_flow rule?
>>>>>>>>>>>
>>>>>>>>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
>>>>>>>>>>> RX_METADATA_NEGOTIATE(once)"
>>>>>>>>>>>
>>>>>>>>>>> Isn't it enough if
>>>>>>>>>>>
>>>>>>>>>>> #1 We have enough print when rte_flow is being create without
>>>>>>>>>>> negotiation done and ask user to enable rx metadata using
>>>>>>>>>>> "port config <port_id>..."
>>>>>>>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
>>>>>>>>>>> --rx-metadata") like other features to avoid calling another
>>>>>>>>>>> command before rte flow create.
>>>>>>>>>>
>>>>>>>>>> I'm not sure what is best.
>>>>>>>>>> I will let others discuss this part.
>>>>>>>>>
>>>>>>>>> IMO, enabling something always defeat the purpose to negotiate. IMO,
>>>>>>>>> someone needs to negotiate
>>>>>>>>> for a feature if the feature is needed. I think, the double enablement
>>>>>>>>> is part of the spec itself. In case, The PMD
>>>>>>>>> drivers won't like double enablement, no need to implement the PMD
>>>>>>>>> callback. That way, there is no change in the existing flow.
>>>>>>>>>
>>>>>>>>> The reason why cnxk driver thought of leveraging negotiate() feature
>>>>>>>>> so that it helps for optimization. e.s.p
>>>>>>>>> function template for multiprocess case as we know what features
>>>>>>>>> needed in fastpath upfront.
>>>>>>>>>
>>>>>>>>> If there still concerns with patch we can take up this to TB decide
>>>>>>>>> one way or another to make forward progress. Let us know.
>>>>>>>>
>>>>>>>> Ferruh, Andrew, Ori, Ivan, what is your opinion?
>>>>>>>> Let's progress with this patch to make it in -rc1.
>>>>>>
>>>>>> As I understand all agreed that we need testpmd command to
>>>>>> control negotiated Rx metadata. May be even command-line
>>>>>> option would be useful.
>>>>>>
>>>>>> So, remaining question is what should be the default value in
>>>>>> testpmd. Note that it is just testpmd question since default
>>>>>> value in an abstract application is nothing negotiated
>>>>>> (if I'm not mistaken).
>>>>>>
>>>>>> The key advantaan ge of the current behaviour is to avoid
>>>>>> "double-enabling" in testpmd. It preserves behaviour which
>>>>>> we had before before the API addition. It is a strong
>>>>>> argument. Basically it puts the feature into the same
>>>>>> basket as FAST_FREE - need an action to run faster.
>>>>>
>>>>> I think, there is a disconnect here. FAST_FREE is enabled by default.
>>
>> Sorry, I'm lost here. Don't we need to enable FAST_FREE
>> offload? As far as I know all offloads are disabled by default.
>
> Not the case for FAST_FREE as disabling needs "more cycles on processor" side.
>
> See app/test-pmd/testpmd.c
> /*
> * Ethernet device configuration.
> */
> struct rte_eth_rxmode rx_mode;
>
> struct rte_eth_txmode tx_mode = {
> .offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE,
> };
>
Surprised, thanks. So, it one more difference of the testpmd
defaults from an abstract application.
>>
>>>>> i.e We don't need any specific action to run faster. To align with performance
>>>>> test application, by default the configuration should be run faster. User
>>>>> needs to give explicit configuration to allow more offload or the one causes
>>>>> the mpps drops. IMO, That is the theme followed in testpmd.
>>>>>
>>>>>
>>>> I agree with Andrew, the default should stay the same, as now, PMD may already implement
>>>> logic to only enable the feature if there is a flow rule.
>>>> Changing the default will result in breaking applications.
>>>
>>> That's not what is discussed here.
>>> We are talking only about testpmd default.
>>>
>>>> I want to suggest new approach for this feature,
>>>> maybe we can use the rte_flow_configure, and add a new bit that says if those
>>>> actions are going to be used.
>>>> What do you think?
>>>
>>> Let's not change the API please.
>>>
>>>
>>>>>> I see no problem in such approach.
>>>>>>
>>>>>> The key disadvantage is the difference in testpmd and
>>>>>> other applications default behaviour.
>>>>>>
>>>>>> I'd look at the feature in the following way:
>>>>>> if an application theoretically wants to use
>>>>>> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
>>>>>> corresponding Rx metadata to ensure that the feature is
>>>>>> available and HW is informed that application may need it.
>>>>>> Since testpmd supports corresponding flow API actions and
>>>>>> flow tunnels, it tries to negotiate it by default, but do
>>>>>> not fail if the negotiation fails.
>>>>>>
>>>>>> So, I'd would vote to keeping the default value as is.
Two above paragraphs still stand.
Frankly speaking I don't understand why default value is so
important if we have a way to change it. Reasons should be
really strong to change existing defaults.
On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
>
> On 2/1/23 12:14, Jerin Jacob wrote:
> > On Wed, Feb 1, 2023 at 2:37 PM Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru> wrote:
> >>
> >> On 2/1/23 12:05, Thomas Monjalon wrote:
> >>> 01/02/2023 10:00, Ori Kam:
> >>>> Hi all,
> >>>>
> >>>> Sorry for jumping in late,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
> >>>>> Sent: Wednesday, 1 February 2023 10:53
> >>>>>
> >>>>> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
> >>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
> >>>>>>
> >>>>>> On 2/1/23 09:10, Ivan Malov wrote:
> >>>>>>> Hello everyone,
> >>>>>>>
> >>>>>>> Since making automatic, or implicit, offload decisions does
> >>>>>>> not belong in testpmd responsibility domain, it should be
> >>>>>>> safer to avoid calling the "negotiate metadata delivery"
> >>>>>>> API with some default selection unless the user asks to
> >>>>>>> do so explicitly, via internal CLI or app options.
> >>>>>>>
> >>>>>>> With that in mind, port config <port_id> ... sounds OK.
> >>>>>>>
> >>>>>>> PMDs that support flow primitives which can generate metadata
> >>>>>>> but, if in started state, can't enable its delivery may pass
> >>>>>>> appropriate rte_error messages to the user suggesting
> >>>>>>> they enable delivery of such metadata from NIC to PMD
> >>>>>>> first. This way, if the person operating testpmd
> >>>>>>> enters a flow create command and that fails,
> >>>>>>> they can figure out the inconsistency, stop
> >>>>>>> the port, negotiate, start and try again.
> >>>>>>>
> >>>>>>> As for non-debug applications, their developers shall
> >>>>>>> be properly informed about the problem of enabling
> >>>>>>> delivery of metadata from NIC to PMD. This way,
> >>>>>>> they will invoke the negotiate API by default
> >>>>>>> in their apps, with the feature selection (eg.
> >>>>>>> MARK) as per nature of the app's business.
> >>>>>>>
> >>>>>>> This API should indeed be helpful to some PMDs with
> >>>>>>> regard to collecting upfront knowledge like this.
> >>>>>>> At the same time, should be benign to those PMDs
> >>>>>>> who do not need this knowledge and can enable
> >>>>>>> delivery of metadata right when inserting the
> >>>>>>> flow rules. So I hope the API does not create
> >>>>>>> too much discomfort to vendors not needing it.
> >>>>>>>
> >>>>>>> Thank you.
> >>>>>>>
> >>>>>>> On Wed, 1 Feb 2023, Thomas Monjalon wrote:
> >>>>>>>
> >>>>>>>> 31/01/2023 17:17, Jerin Jacob:
> >>>>>>>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
> >>>>> <thomas@monjalon.net>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> >>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> >>>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>>>>>>> Ferruh is proposing to have a command "port config <port_id>
> >>>>> ..."
> >>>>>>>>>>>>>> to configure the flags to negotiate.
> >>>>>>>>>>>>>> Are you OK with this approach?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Yes, we are fine to have such command to enable and disable the
> >>>>>>>>>>>>> feature
> >>>>>>>>>>>>> with default being it disabled if supported by PMD.
> >>>>>>>>>>>>> Is default being disabled fine if the feature is supported by a
> >>>>>>>>>>>>> PMD ?
> >>>>>>>>>>>>
> >>>>>>>>>>>> I think the default should be enabled for ease of use.
> >>>>>>>>>>>
> >>>>>>>>>>> Since testpmd is used extensively for benchmarking purposes, we
> >>>>>>>>>>> thought it should have minimum features
> >>>>>>>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
> >>>>>>>>>>> offloads enabled(except for FAST FREE), default
> >>>>>>>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
> >>>>> when
> >>>>>>>>>>> dumping packets.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> Do we have similar features disables by default?
> >>>>>>>>>>>> I mean do we know features in testpmd which require a "double
> >>>>>>>>>>>> enablement"
> >>>>>>>>>>>> like one configuration command + one rte_flow rule?
> >>>>>>>>>>>
> >>>>>>>>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
> >>>>>>>>>>> RX_METADATA_NEGOTIATE(once)"
> >>>>>>>>>>>
> >>>>>>>>>>> Isn't it enough if
> >>>>>>>>>>>
> >>>>>>>>>>> #1 We have enough print when rte_flow is being create without
> >>>>>>>>>>> negotiation done and ask user to enable rx metadata using
> >>>>>>>>>>> "port config <port_id>..."
> >>>>>>>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
> >>>>>>>>>>> --rx-metadata") like other features to avoid calling another
> >>>>>>>>>>> command before rte flow create.
> >>>>>>>>>>
> >>>>>>>>>> I'm not sure what is best.
> >>>>>>>>>> I will let others discuss this part.
> >>>>>>>>>
> >>>>>>>>> IMO, enabling something always defeat the purpose to negotiate. IMO,
> >>>>>>>>> someone needs to negotiate
> >>>>>>>>> for a feature if the feature is needed. I think, the double enablement
> >>>>>>>>> is part of the spec itself. In case, The PMD
> >>>>>>>>> drivers won't like double enablement, no need to implement the PMD
> >>>>>>>>> callback. That way, there is no change in the existing flow.
> >>>>>>>>>
> >>>>>>>>> The reason why cnxk driver thought of leveraging negotiate() feature
> >>>>>>>>> so that it helps for optimization. e.s.p
> >>>>>>>>> function template for multiprocess case as we know what features
> >>>>>>>>> needed in fastpath upfront.
> >>>>>>>>>
> >>>>>>>>> If there still concerns with patch we can take up this to TB decide
> >>>>>>>>> one way or another to make forward progress. Let us know.
> >>>>>>>>
> >>>>>>>> Ferruh, Andrew, Ori, Ivan, what is your opinion?
> >>>>>>>> Let's progress with this patch to make it in -rc1.
> >>>>>>
> >>>>>> As I understand all agreed that we need testpmd command to
> >>>>>> control negotiated Rx metadata. May be even command-line
> >>>>>> option would be useful.
> >>>>>>
> >>>>>> So, remaining question is what should be the default value in
> >>>>>> testpmd. Note that it is just testpmd question since default
> >>>>>> value in an abstract application is nothing negotiated
> >>>>>> (if I'm not mistaken).
> >>>>>>
> >>>>>> The key advantaan ge of the current behaviour is to avoid
> >>>>>> "double-enabling" in testpmd. It preserves behaviour which
> >>>>>> we had before before the API addition. It is a strong
> >>>>>> argument. Basically it puts the feature into the same
> >>>>>> basket as FAST_FREE - need an action to run faster.
> >>>>>
> >>>>> I think, there is a disconnect here. FAST_FREE is enabled by default.
> >>
> >> Sorry, I'm lost here. Don't we need to enable FAST_FREE
> >> offload? As far as I know all offloads are disabled by default.
> >
> > Not the case for FAST_FREE as disabling needs "more cycles on processor" side.
> >
> > See app/test-pmd/testpmd.c
> > /*
> > * Ethernet device configuration.
> > */
> > struct rte_eth_rxmode rx_mode;
> >
> > struct rte_eth_txmode tx_mode = {
> > .offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE,
> > };
> >
>
> Surprised, thanks. So, it one more difference of the testpmd
> defaults from an abstract application.
It is not, even l2fwd is same as testpmd. In my view, the ideal
application just to select what is
really needed.
>
> >>
> >>>>> i.e We don't need any specific action to run faster. To align with performance
> >>>>> test application, by default the configuration should be run faster. User
> >>>>> needs to give explicit configuration to allow more offload or the one causes
> >>>>> the mpps drops. IMO, That is the theme followed in testpmd.
> >>>>>
> >>>>>
> >>>> I agree with Andrew, the default should stay the same, as now, PMD may already implement
> >>>> logic to only enable the feature if there is a flow rule.
> >>>> Changing the default will result in breaking applications.
> >>>
> >>> That's not what is discussed here.
> >>> We are talking only about testpmd default.
> >>>
> >>>> I want to suggest new approach for this feature,
> >>>> maybe we can use the rte_flow_configure, and add a new bit that says if those
> >>>> actions are going to be used.
> >>>> What do you think?
> >>>
> >>> Let's not change the API please.
> >>>
> >>>
> >>>>>> I see no problem in such approach.
> >>>>>>
> >>>>>> The key disadvantage is the difference in testpmd and
> >>>>>> other applications default behaviour.
But none of the other applications in not negotiating by default.
> >>>>>>
> >>>>>> I'd look at the feature in the following way:
> >>>>>> if an application theoretically wants to use
> >>>>>> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
> >>>>>> corresponding Rx metadata to ensure that the feature is
> >>>>>> available and HW is informed that application may need it.
> >>>>>> Since testpmd supports corresponding flow API actions and
> >>>>>> flow tunnels, it tries to negotiate it by default, but do
> >>>>>> not fail if the negotiation fails.
> >>>>>>
> >>>>>> So, I'd would vote to keeping the default value as is.
>
> Two above paragraphs still stand.
>
> Frankly speaking I don't understand why default value is so
> important if we have a way to change it. Reasons should be
> really strong to change existing defaults.
The only reason is, typically testpmd will be used performance
benchmarking as an industry standard. It is difficult to tell/educate
the QA or customers
that, "BTW if you need to get better performance add more flag to
testpmd command line".
To make that worst, only some PMD needs to give the additional
parameter to get better number.
And also, testpmd usage will be treated as application modeling.
Since this feature only used on sfc and cnxk driver, What is the
situation with sfc driver?
Keeping it as negotiated and not use the feature, will impact the per
core performance of sfc or
is it just PCI bandwidth thing which really dont show any difference in testpmd?
On 2/1/23 13:48, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
>>
>> On 2/1/23 12:14, Jerin Jacob wrote:
>>> On Wed, Feb 1, 2023 at 2:37 PM Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>
>>>> On 2/1/23 12:05, Thomas Monjalon wrote:
>>>>> 01/02/2023 10:00, Ori Kam:
>>>>>> Hi all,
>>>>>>
>>>>>> Sorry for jumping in late,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>>>> Sent: Wednesday, 1 February 2023 10:53
>>>>>>>
>>>>>>> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>
>>>>>>>> On 2/1/23 09:10, Ivan Malov wrote:
>>>>>>>>> Hello everyone,
>>>>>>>>>
>>>>>>>>> Since making automatic, or implicit, offload decisions does
>>>>>>>>> not belong in testpmd responsibility domain, it should be
>>>>>>>>> safer to avoid calling the "negotiate metadata delivery"
>>>>>>>>> API with some default selection unless the user asks to
>>>>>>>>> do so explicitly, via internal CLI or app options.
>>>>>>>>>
>>>>>>>>> With that in mind, port config <port_id> ... sounds OK.
>>>>>>>>>
>>>>>>>>> PMDs that support flow primitives which can generate metadata
>>>>>>>>> but, if in started state, can't enable its delivery may pass
>>>>>>>>> appropriate rte_error messages to the user suggesting
>>>>>>>>> they enable delivery of such metadata from NIC to PMD
>>>>>>>>> first. This way, if the person operating testpmd
>>>>>>>>> enters a flow create command and that fails,
>>>>>>>>> they can figure out the inconsistency, stop
>>>>>>>>> the port, negotiate, start and try again.
>>>>>>>>>
>>>>>>>>> As for non-debug applications, their developers shall
>>>>>>>>> be properly informed about the problem of enabling
>>>>>>>>> delivery of metadata from NIC to PMD. This way,
>>>>>>>>> they will invoke the negotiate API by default
>>>>>>>>> in their apps, with the feature selection (eg.
>>>>>>>>> MARK) as per nature of the app's business.
>>>>>>>>>
>>>>>>>>> This API should indeed be helpful to some PMDs with
>>>>>>>>> regard to collecting upfront knowledge like this.
>>>>>>>>> At the same time, should be benign to those PMDs
>>>>>>>>> who do not need this knowledge and can enable
>>>>>>>>> delivery of metadata right when inserting the
>>>>>>>>> flow rules. So I hope the API does not create
>>>>>>>>> too much discomfort to vendors not needing it.
>>>>>>>>>
>>>>>>>>> Thank you.
>>>>>>>>>
>>>>>>>>> On Wed, 1 Feb 2023, Thomas Monjalon wrote:
>>>>>>>>>
>>>>>>>>>> 31/01/2023 17:17, Jerin Jacob:
>>>>>>>>>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
>>>>>>> <thomas@monjalon.net>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
>>>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
>>>>>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>>>>>>> Ferruh is proposing to have a command "port config <port_id>
>>>>>>> ..."
>>>>>>>>>>>>>>>> to configure the flags to negotiate.
>>>>>>>>>>>>>>>> Are you OK with this approach?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes, we are fine to have such command to enable and disable the
>>>>>>>>>>>>>>> feature
>>>>>>>>>>>>>>> with default being it disabled if supported by PMD.
>>>>>>>>>>>>>>> Is default being disabled fine if the feature is supported by a
>>>>>>>>>>>>>>> PMD ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think the default should be enabled for ease of use.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Since testpmd is used extensively for benchmarking purposes, we
>>>>>>>>>>>>> thought it should have minimum features
>>>>>>>>>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
>>>>>>>>>>>>> offloads enabled(except for FAST FREE), default
>>>>>>>>>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
>>>>>>> when
>>>>>>>>>>>>> dumping packets.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Do we have similar features disables by default?
>>>>>>>>>>>>>> I mean do we know features in testpmd which require a "double
>>>>>>>>>>>>>> enablement"
>>>>>>>>>>>>>> like one configuration command + one rte_flow rule?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
>>>>>>>>>>>>> RX_METADATA_NEGOTIATE(once)"
>>>>>>>>>>>>>
>>>>>>>>>>>>> Isn't it enough if
>>>>>>>>>>>>>
>>>>>>>>>>>>> #1 We have enough print when rte_flow is being create without
>>>>>>>>>>>>> negotiation done and ask user to enable rx metadata using
>>>>>>>>>>>>> "port config <port_id>..."
>>>>>>>>>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
>>>>>>>>>>>>> --rx-metadata") like other features to avoid calling another
>>>>>>>>>>>>> command before rte flow create.
>>>>>>>>>>>>
>>>>>>>>>>>> I'm not sure what is best.
>>>>>>>>>>>> I will let others discuss this part.
>>>>>>>>>>>
>>>>>>>>>>> IMO, enabling something always defeat the purpose to negotiate. IMO,
>>>>>>>>>>> someone needs to negotiate
>>>>>>>>>>> for a feature if the feature is needed. I think, the double enablement
>>>>>>>>>>> is part of the spec itself. In case, The PMD
>>>>>>>>>>> drivers won't like double enablement, no need to implement the PMD
>>>>>>>>>>> callback. That way, there is no change in the existing flow.
>>>>>>>>>>>
>>>>>>>>>>> The reason why cnxk driver thought of leveraging negotiate() feature
>>>>>>>>>>> so that it helps for optimization. e.s.p
>>>>>>>>>>> function template for multiprocess case as we know what features
>>>>>>>>>>> needed in fastpath upfront.
>>>>>>>>>>>
>>>>>>>>>>> If there still concerns with patch we can take up this to TB decide
>>>>>>>>>>> one way or another to make forward progress. Let us know.
>>>>>>>>>>
>>>>>>>>>> Ferruh, Andrew, Ori, Ivan, what is your opinion?
>>>>>>>>>> Let's progress with this patch to make it in -rc1.
>>>>>>>>
>>>>>>>> As I understand all agreed that we need testpmd command to
>>>>>>>> control negotiated Rx metadata. May be even command-line
>>>>>>>> option would be useful.
>>>>>>>>
>>>>>>>> So, remaining question is what should be the default value in
>>>>>>>> testpmd. Note that it is just testpmd question since default
>>>>>>>> value in an abstract application is nothing negotiated
>>>>>>>> (if I'm not mistaken).
>>>>>>>>
>>>>>>>> The key advantaan ge of the current behaviour is to avoid
>>>>>>>> "double-enabling" in testpmd. It preserves behaviour which
>>>>>>>> we had before before the API addition. It is a strong
>>>>>>>> argument. Basically it puts the feature into the same
>>>>>>>> basket as FAST_FREE - need an action to run faster.
>>>>>>>
>>>>>>> I think, there is a disconnect here. FAST_FREE is enabled by default.
>>>>
>>>> Sorry, I'm lost here. Don't we need to enable FAST_FREE
>>>> offload? As far as I know all offloads are disabled by default.
>>>
>>> Not the case for FAST_FREE as disabling needs "more cycles on processor" side.
>>>
>>> See app/test-pmd/testpmd.c
>>> /*
>>> * Ethernet device configuration.
>>> */
>>> struct rte_eth_rxmode rx_mode;
>>>
>>> struct rte_eth_txmode tx_mode = {
>>> .offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE,
>>> };
>>>
>>
>> Surprised, thanks. So, it one more difference of the testpmd
>> defaults from an abstract application.
>
> It is not, even l2fwd is same as testpmd. In my view, the ideal
> application just to select what is
> really needed.
>
>>
>>>>
>>>>>>> i.e We don't need any specific action to run faster. To align with performance
>>>>>>> test application, by default the configuration should be run faster. User
>>>>>>> needs to give explicit configuration to allow more offload or the one causes
>>>>>>> the mpps drops. IMO, That is the theme followed in testpmd.
>>>>>>>
>>>>>>>
>>>>>> I agree with Andrew, the default should stay the same, as now, PMD may already implement
>>>>>> logic to only enable the feature if there is a flow rule.
>>>>>> Changing the default will result in breaking applications.
>>>>>
>>>>> That's not what is discussed here.
>>>>> We are talking only about testpmd default.
>>>>>
>>>>>> I want to suggest new approach for this feature,
>>>>>> maybe we can use the rte_flow_configure, and add a new bit that says if those
>>>>>> actions are going to be used.
>>>>>> What do you think?
>>>>>
>>>>> Let's not change the API please.
>>>>>
>>>>>
>>>>>>>> I see no problem in such approach.
>>>>>>>>
>>>>>>>> The key disadvantage is the difference in testpmd and
>>>>>>>> other applications default behaviour.
>
> But none of the other applications in not negotiating by default.
>
>>>>>>>>
>>>>>>>> I'd look at the feature in the following way:
>>>>>>>> if an application theoretically wants to use
>>>>>>>> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
>>>>>>>> corresponding Rx metadata to ensure that the feature is
>>>>>>>> available and HW is informed that application may need it.
>>>>>>>> Since testpmd supports corresponding flow API actions and
>>>>>>>> flow tunnels, it tries to negotiate it by default, but do
>>>>>>>> not fail if the negotiation fails.
>>>>>>>>
>>>>>>>> So, I'd would vote to keeping the default value as is.
>>
>> Two above paragraphs still stand.
>>
>> Frankly speaking I don't understand why default value is so
>> important if we have a way to change it. Reasons should be
>> really strong to change existing defaults.
>
> The only reason is, typically testpmd will be used performance
> benchmarking as an industry standard. It is difficult to tell/educate
> the QA or customers
> that, "BTW if you need to get better performance add more flag to
> testpmd command line".
> To make that worst, only some PMD needs to give the additional
> parameter to get better number.
> And also, testpmd usage will be treated as application modeling.
>
> Since this feature only used on sfc and cnxk driver, What is the
> situation with sfc driver?
> Keeping it as negotiated and not use the feature, will impact the per
> core performance of sfc or
> is it just PCI bandwidth thing which really dont show any difference in testpmd?
Yes, sfc could run faster if no Rx metadata are negotiated. So,
it is better to negotiate nothing by default. But it is always
painful to change defaults. You need to explain that now you
need to negotiate Rx metadata to use mark, flag and tunnel offloads.
Yes, it will be required on sfc and cnxk only.
As an sfc maintainer I don't mind to change testpmd defaults.
01/02/2023 11:58, Andrew Rybchenko:
> On 2/1/23 13:48, Jerin Jacob wrote:
> > On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru> wrote:
> >> Frankly speaking I don't understand why default value is so
> >> important if we have a way to change it. Reasons should be
> >> really strong to change existing defaults.
> >
> > The only reason is, typically testpmd will be used performance
> > benchmarking as an industry standard. It is difficult to tell/educate
> > the QA or customers
> > that, "BTW if you need to get better performance add more flag to
> > testpmd command line".
I disagree.
When you do performance benchmark, you tune settings accordingly.
> > To make that worst, only some PMD needs to give the additional
> > parameter to get better number.
> > And also, testpmd usage will be treated as application modeling.
> >
> > Since this feature only used on sfc and cnxk driver, What is the
> > situation with sfc driver?
> > Keeping it as negotiated and not use the feature, will impact the per
> > core performance of sfc or
> > is it just PCI bandwidth thing which really dont show any difference in testpmd?
>
> Yes, sfc could run faster if no Rx metadata are negotiated. So,
> it is better to negotiate nothing by default. But it is always
> painful to change defaults. You need to explain that now you
> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
> Yes, it will be required on sfc and cnxk only.
> As an sfc maintainer I don't mind to change testpmd defaults.
If we change testpmd defaults to "do nothing",
then we should disable MBUF_FAST_FREE as well.
On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 01/02/2023 11:58, Andrew Rybchenko:
> > On 2/1/23 13:48, Jerin Jacob wrote:
> > > On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
> > > <andrew.rybchenko@oktetlabs.ru> wrote:
> > >> Frankly speaking I don't understand why default value is so
> > >> important if we have a way to change it. Reasons should be
> > >> really strong to change existing defaults.
> > >
> > > The only reason is, typically testpmd will be used performance
> > > benchmarking as an industry standard. It is difficult to tell/educate
> > > the QA or customers
> > > that, "BTW if you need to get better performance add more flag to
> > > testpmd command line".
>
> I disagree.
> When you do performance benchmark, you tune settings accordingly.
IMO, We tune the system resources like queue depth not the disabling
features for raw performance.
queue depth etc people know to tune so it is obvious. What is not
obvious is, testpmd only
negotiated some features by default.I am not using that feature, hence
I need to explicitly
disable it.
>
> > > To make that worst, only some PMD needs to give the additional
> > > parameter to get better number.
> > > And also, testpmd usage will be treated as application modeling.
> > >
> > > Since this feature only used on sfc and cnxk driver, What is the
> > > situation with sfc driver?
> > > Keeping it as negotiated and not use the feature, will impact the per
> > > core performance of sfc or
> > > is it just PCI bandwidth thing which really dont show any difference in testpmd?
> >
> > Yes, sfc could run faster if no Rx metadata are negotiated. So,
> > it is better to negotiate nothing by default. But it is always
> > painful to change defaults. You need to explain that now you
> > need to negotiate Rx metadata to use mark, flag and tunnel offloads.
> > Yes, it will be required on sfc and cnxk only.
> > As an sfc maintainer I don't mind to change testpmd defaults.
>
> If we change testpmd defaults to "do nothing",
> then we should disable MBUF_FAST_FREE as well.
if you see MBUF_FAST_FREE, it does nothing. Actually,
!MBUF_FAST_FREE is doing more work.
>
>
Hi Thomas,
On Wed, 1 Feb 2023, Thomas Monjalon wrote:
> 01/02/2023 11:58, Andrew Rybchenko:
>> On 2/1/23 13:48, Jerin Jacob wrote:
>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>> Frankly speaking I don't understand why default value is so
>>>> important if we have a way to change it. Reasons should be
>>>> really strong to change existing defaults.
>>>
>>> The only reason is, typically testpmd will be used performance
>>> benchmarking as an industry standard. It is difficult to tell/educate
>>> the QA or customers
>>> that, "BTW if you need to get better performance add more flag to
>>> testpmd command line".
>
> I disagree.
> When you do performance benchmark, you tune settings accordingly.
>
>>> To make that worst, only some PMD needs to give the additional
>>> parameter to get better number.
>>> And also, testpmd usage will be treated as application modeling.
>>>
>>> Since this feature only used on sfc and cnxk driver, What is the
>>> situation with sfc driver?
>>> Keeping it as negotiated and not use the feature, will impact the per
>>> core performance of sfc or
>>> is it just PCI bandwidth thing which really dont show any difference in testpmd?
>>
>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>> it is better to negotiate nothing by default. But it is always
>> painful to change defaults. You need to explain that now you
>> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
>> Yes, it will be required on sfc and cnxk only.
>> As an sfc maintainer I don't mind to change testpmd defaults.
>
> If we change testpmd defaults to "do nothing",
> then we should disable MBUF_FAST_FREE as well.
No. These are completely different. MBUF_FAST_FREE is an
optimisation technique, it's in the offload namespace.
Whilst "negotiate metadata" does not offload anything.
It just tells the PMD to enable *delivery* of some
data *when it is present*. And whether it will be
present or not, - this is decided by flow actions.
And, flow actions, in turn, belong in the domain
of decisions made by a specific person operating
the application. So no need to remove FAST_FREE.
>
>
>
On 2/1/2023 11:15 AM, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> 01/02/2023 11:58, Andrew Rybchenko:
>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>> Frankly speaking I don't understand why default value is so
>>>>> important if we have a way to change it. Reasons should be
>>>>> really strong to change existing defaults.
>>>>
>>>> The only reason is, typically testpmd will be used performance
>>>> benchmarking as an industry standard. It is difficult to tell/educate
>>>> the QA or customers
>>>> that, "BTW if you need to get better performance add more flag to
>>>> testpmd command line".
>>
>> I disagree.
>> When you do performance benchmark, you tune settings accordingly.
>
> IMO, We tune the system resources like queue depth not the disabling
> features for raw performance.
> queue depth etc people know to tune so it is obvious. What is not
> obvious is, testpmd only
> negotiated some features by default.I am not using that feature, hence
> I need to explicitly
> disable it.
>
When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
believe that is the case for almost all applications since API is a
relatively new one, PMD default behavior should be to enable Rx metadata
flow rules, in case user requests them later.
So, enabling all in application is same with not calling the API at all.
In this perspective, disabling Rx metadata is additional
optimization/tuning that application can do if it is sure that Rx
metadata flow rules won't be used at all.
And API is more meaningful when it is used to disable Rx metadata.
I think it is reasonable to enable all Rx metadata by default in testpmd
with a capability to disable it when wanted.
OR
May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in
testpmd, it is only called when it is requested explicitly from user,
enable or disable.
>
>>
>>>> To make that worst, only some PMD needs to give the additional
>>>> parameter to get better number.
>>>> And also, testpmd usage will be treated as application modeling.
>>>>
>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>> situation with sfc driver?
>>>> Keeping it as negotiated and not use the feature, will impact the per
>>>> core performance of sfc or
>>>> is it just PCI bandwidth thing which really dont show any difference in testpmd?
>>>
>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>> it is better to negotiate nothing by default. But it is always
>>> painful to change defaults. You need to explain that now you
>>> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
>>> Yes, it will be required on sfc and cnxk only.
>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>
>> If we change testpmd defaults to "do nothing",
>> then we should disable MBUF_FAST_FREE as well.
>
> if you see MBUF_FAST_FREE, it does nothing. Actually,
> !MBUF_FAST_FREE is doing more work.
>
>
>>
>>
On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
> > On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>
> >> 01/02/2023 11:58, Andrew Rybchenko:
> >>> On 2/1/23 13:48, Jerin Jacob wrote:
> >>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
> >>>> <andrew.rybchenko@oktetlabs.ru> wrote:
> >>>>> Frankly speaking I don't understand why default value is so
> >>>>> important if we have a way to change it. Reasons should be
> >>>>> really strong to change existing defaults.
> >>>>
> >>>> The only reason is, typically testpmd will be used performance
> >>>> benchmarking as an industry standard. It is difficult to tell/educate
> >>>> the QA or customers
> >>>> that, "BTW if you need to get better performance add more flag to
> >>>> testpmd command line".
> >>
> >> I disagree.
> >> When you do performance benchmark, you tune settings accordingly.
> >
> > IMO, We tune the system resources like queue depth not the disabling
> > features for raw performance.
> > queue depth etc people know to tune so it is obvious. What is not
> > obvious is, testpmd only
> > negotiated some features by default.I am not using that feature, hence
> > I need to explicitly
> > disable it.
> >
>
> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
> believe that is the case for almost all applications since API is a
> relatively new one, PMD default behavior should be to enable Rx metadata
> flow rules, in case user requests them later.
>
> So, enabling all in application is same with not calling the API at all.
>
> In this perspective, disabling Rx metadata is additional
> optimization/tuning that application can do if it is sure that Rx
> metadata flow rules won't be used at all.
> And API is more meaningful when it is used to disable Rx metadata.
>
> I think it is reasonable to enable all Rx metadata by default in testpmd
> with a capability to disable it when wanted.
>
> OR
>
> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in
> testpmd, it is only called when it is requested explicitly from user,
> enable or disable.
Second option looks good to me.
When
1) user request for action which is needed negotiate(),
AND
2) rte_eth_rx_metadata_negotiate() != ENOSUP
then, testpmd print a warning that need to enable
rte_eth_rx_metadata_negotiate().
> >
> >>
> >>>> To make that worst, only some PMD needs to give the additional
> >>>> parameter to get better number.
> >>>> And also, testpmd usage will be treated as application modeling.
> >>>>
> >>>> Since this feature only used on sfc and cnxk driver, What is the
> >>>> situation with sfc driver?
> >>>> Keeping it as negotiated and not use the feature, will impact the per
> >>>> core performance of sfc or
> >>>> is it just PCI bandwidth thing which really dont show any difference in testpmd?
> >>>
> >>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
> >>> it is better to negotiate nothing by default. But it is always
> >>> painful to change defaults. You need to explain that now you
> >>> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
> >>> Yes, it will be required on sfc and cnxk only.
> >>> As an sfc maintainer I don't mind to change testpmd defaults.
> >>
> >> If we change testpmd defaults to "do nothing",
> >> then we should disable MBUF_FAST_FREE as well.
> >
> > if you see MBUF_FAST_FREE, it does nothing. Actually,
> > !MBUF_FAST_FREE is doing more work.
> >
> >
> >>
> >>
>
On 2/1/2023 1:48 PM, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>
>>>> 01/02/2023 11:58, Andrew Rybchenko:
>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>> Frankly speaking I don't understand why default value is so
>>>>>>> important if we have a way to change it. Reasons should be
>>>>>>> really strong to change existing defaults.
>>>>>>
>>>>>> The only reason is, typically testpmd will be used performance
>>>>>> benchmarking as an industry standard. It is difficult to tell/educate
>>>>>> the QA or customers
>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>> testpmd command line".
>>>>
>>>> I disagree.
>>>> When you do performance benchmark, you tune settings accordingly.
>>>
>>> IMO, We tune the system resources like queue depth not the disabling
>>> features for raw performance.
>>> queue depth etc people know to tune so it is obvious. What is not
>>> obvious is, testpmd only
>>> negotiated some features by default.I am not using that feature, hence
>>> I need to explicitly
>>> disable it.
>>>
>>
>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
>> believe that is the case for almost all applications since API is a
>> relatively new one, PMD default behavior should be to enable Rx metadata
>> flow rules, in case user requests them later.
>>
>> So, enabling all in application is same with not calling the API at all.
>>
>> In this perspective, disabling Rx metadata is additional
>> optimization/tuning that application can do if it is sure that Rx
>> metadata flow rules won't be used at all.
>> And API is more meaningful when it is used to disable Rx metadata.
>>
>> I think it is reasonable to enable all Rx metadata by default in testpmd
>> with a capability to disable it when wanted.
>>
>> OR
>>
>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in
>> testpmd, it is only called when it is requested explicitly from user,
>> enable or disable.
>
> Second option looks good to me.
> When
> 1) user request for action which is needed negotiate(),
> AND
> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
> then, testpmd print a warning that need to enable
> rte_eth_rx_metadata_negotiate().
>
We are not suggesting same thing.
What you described above assumes PMD disabled Rx metadata flow rule
support by default, and it needs to be enabled explicitly by
'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
functionality.
As far as I understand PMD wants to disable this flow rule by default
because of performance concerns. But this creates inconsistency between
PMDs, because rest of them will enable this flow rule by default (if it
is supported) and be ready to use it when proper flow rule created.
With this approach some PMDs will need 'rte_eth_rx_metadata_negotiate()'
to enable Rx metadata flow rules, some won't. This can be confusing for
applications that *some* PMDs require double enabling with specific API
call.
Instead what I was trying to suggest is reverse,
all PMDs enable the Rx metadata flow rule by default, and don't require
double enabling.
But if application knows that it won't use Rx metadata flow rule, it can
disable it to optimize the performance.
This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
for testpmd context it can be called via a command on demand by user for
optimization purpose.
>
>>>
>>>>
>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>> parameter to get better number.
>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>
>>>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>>>> situation with sfc driver?
>>>>>> Keeping it as negotiated and not use the feature, will impact the per
>>>>>> core performance of sfc or
>>>>>> is it just PCI bandwidth thing which really dont show any difference in testpmd?
>>>>>
>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>>>> it is better to negotiate nothing by default. But it is always
>>>>> painful to change defaults. You need to explain that now you
>>>>> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
>>>>> Yes, it will be required on sfc and cnxk only.
>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>>>
>>>> If we change testpmd defaults to "do nothing",
>>>> then we should disable MBUF_FAST_FREE as well.
>>>
>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
>>> !MBUF_FAST_FREE is doing more work.
>>>
>>>
>>>>
>>>>
>>
On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
> > On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
> >>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>>>
> >>>> 01/02/2023 11:58, Andrew Rybchenko:
> >>>>> On 2/1/23 13:48, Jerin Jacob wrote:
> >>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
> >>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
> >>>>>>> Frankly speaking I don't understand why default value is so
> >>>>>>> important if we have a way to change it. Reasons should be
> >>>>>>> really strong to change existing defaults.
> >>>>>>
> >>>>>> The only reason is, typically testpmd will be used performance
> >>>>>> benchmarking as an industry standard. It is difficult to tell/educate
> >>>>>> the QA or customers
> >>>>>> that, "BTW if you need to get better performance add more flag to
> >>>>>> testpmd command line".
> >>>>
> >>>> I disagree.
> >>>> When you do performance benchmark, you tune settings accordingly.
> >>>
> >>> IMO, We tune the system resources like queue depth not the disabling
> >>> features for raw performance.
> >>> queue depth etc people know to tune so it is obvious. What is not
> >>> obvious is, testpmd only
> >>> negotiated some features by default.I am not using that feature, hence
> >>> I need to explicitly
> >>> disable it.
> >>>
> >>
> >> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
> >> believe that is the case for almost all applications since API is a
> >> relatively new one, PMD default behavior should be to enable Rx metadata
> >> flow rules, in case user requests them later.
> >>
> >> So, enabling all in application is same with not calling the API at all.
> >>
> >> In this perspective, disabling Rx metadata is additional
> >> optimization/tuning that application can do if it is sure that Rx
> >> metadata flow rules won't be used at all.
> >> And API is more meaningful when it is used to disable Rx metadata.
> >>
> >> I think it is reasonable to enable all Rx metadata by default in testpmd
> >> with a capability to disable it when wanted.
> >>
> >> OR
> >>
> >> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in
> >> testpmd, it is only called when it is requested explicitly from user,
> >> enable or disable.
> >
> > Second option looks good to me.
> > When
> > 1) user request for action which is needed negotiate(),
> > AND
> > 2) rte_eth_rx_metadata_negotiate() != ENOSUP
> > then, testpmd print a warning that need to enable
> > rte_eth_rx_metadata_negotiate().
> >
>
> We are not suggesting same thing.
>
> What you described above assumes PMD disabled Rx metadata flow rule
> support by default, and it needs to be enabled explicitly by
> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
> functionality.
>
> As far as I understand PMD wants to disable this flow rule by default
> because of performance concerns. But this creates inconsistency between
> PMDs, because rest of them will enable this flow rule by default (if it
> is supported) and be ready to use it when proper flow rule created.
>
> With this approach some PMDs will need 'rte_eth_rx_metadata_negotiate()'
> to enable Rx metadata flow rules, some won't. This can be confusing for
> applications that *some* PMDs require double enabling with specific API
> call.
>
>
> Instead what I was trying to suggest is reverse,
> all PMDs enable the Rx metadata flow rule by default, and don't require
> double enabling.
> But if application knows that it won't use Rx metadata flow rule, it can
> disable it to optimize the performance.
> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
> for testpmd context it can be called via a command on demand by user for
> optimization purpose.
This won't solve concern I have outlined earlier[1].
I think, The part of the problem there is no enough adaption of
rte_eth_rx_metadata_negotiate(),
The view is total different from PMD maintainer PoV vs testpmd application PoV.
Just to avoid back and forth. We will call off this patch and remove
rte_eth_rx_metadata_negotiate()
PMD callback from cnxk driver. Keep it as old behavior, so we don't need to care
about rte_eth_rx_metadata_negotiate().
[1]
The only reason is, typically testpmd will be used performance
benchmarking as an industry standard. It is difficult to tell/educate
the QA or customers
that, "BTW if you need to get better performance add more flag to
testpmd command line".
To make that worst, only some PMD needs to give the additional
parameter to get better number.
And also, testpmd usage will be treated as application modeling.
>
>
>
>
> >
> >>>
> >>>>
> >>>>>> To make that worst, only some PMD needs to give the additional
> >>>>>> parameter to get better number.
> >>>>>> And also, testpmd usage will be treated as application modeling.
> >>>>>>
> >>>>>> Since this feature only used on sfc and cnxk driver, What is the
> >>>>>> situation with sfc driver?
> >>>>>> Keeping it as negotiated and not use the feature, will impact the per
> >>>>>> core performance of sfc or
> >>>>>> is it just PCI bandwidth thing which really dont show any difference in testpmd?
> >>>>>
> >>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
> >>>>> it is better to negotiate nothing by default. But it is always
> >>>>> painful to change defaults. You need to explain that now you
> >>>>> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
> >>>>> Yes, it will be required on sfc and cnxk only.
> >>>>> As an sfc maintainer I don't mind to change testpmd defaults.
> >>>>
> >>>> If we change testpmd defaults to "do nothing",
> >>>> then we should disable MBUF_FAST_FREE as well.
> >>>
> >>> if you see MBUF_FAST_FREE, it does nothing. Actually,
> >>> !MBUF_FAST_FREE is doing more work.
> >>>
> >>>
> >>>>
> >>>>
> >>
>
On 2/1/2023 3:22 PM, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>>
>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>>
>>>>>> 01/02/2023 11:58, Andrew Rybchenko:
>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>> Frankly speaking I don't understand why default value is so
>>>>>>>>> important if we have a way to change it. Reasons should be
>>>>>>>>> really strong to change existing defaults.
>>>>>>>>
>>>>>>>> The only reason is, typically testpmd will be used performance
>>>>>>>> benchmarking as an industry standard. It is difficult to tell/educate
>>>>>>>> the QA or customers
>>>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>>>> testpmd command line".
>>>>>>
>>>>>> I disagree.
>>>>>> When you do performance benchmark, you tune settings accordingly.
>>>>>
>>>>> IMO, We tune the system resources like queue depth not the disabling
>>>>> features for raw performance.
>>>>> queue depth etc people know to tune so it is obvious. What is not
>>>>> obvious is, testpmd only
>>>>> negotiated some features by default.I am not using that feature, hence
>>>>> I need to explicitly
>>>>> disable it.
>>>>>
>>>>
>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
>>>> believe that is the case for almost all applications since API is a
>>>> relatively new one, PMD default behavior should be to enable Rx metadata
>>>> flow rules, in case user requests them later.
>>>>
>>>> So, enabling all in application is same with not calling the API at all.
>>>>
>>>> In this perspective, disabling Rx metadata is additional
>>>> optimization/tuning that application can do if it is sure that Rx
>>>> metadata flow rules won't be used at all.
>>>> And API is more meaningful when it is used to disable Rx metadata.
>>>>
>>>> I think it is reasonable to enable all Rx metadata by default in testpmd
>>>> with a capability to disable it when wanted.
>>>>
>>>> OR
>>>>
>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in
>>>> testpmd, it is only called when it is requested explicitly from user,
>>>> enable or disable.
>>>
>>> Second option looks good to me.
>>> When
>>> 1) user request for action which is needed negotiate(),
>>> AND
>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
>>> then, testpmd print a warning that need to enable
>>> rte_eth_rx_metadata_negotiate().
>>>
>>
>> We are not suggesting same thing.
>>
>> What you described above assumes PMD disabled Rx metadata flow rule
>> support by default, and it needs to be enabled explicitly by
>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
>> functionality.
>>
>> As far as I understand PMD wants to disable this flow rule by default
>> because of performance concerns. But this creates inconsistency between
>> PMDs, because rest of them will enable this flow rule by default (if it
>> is supported) and be ready to use it when proper flow rule created.
>>
>> With this approach some PMDs will need 'rte_eth_rx_metadata_negotiate()'
>> to enable Rx metadata flow rules, some won't. This can be confusing for
>> applications that *some* PMDs require double enabling with specific API
>> call.
>>
>>
>> Instead what I was trying to suggest is reverse,
>> all PMDs enable the Rx metadata flow rule by default, and don't require
>> double enabling.
>> But if application knows that it won't use Rx metadata flow rule, it can
>> disable it to optimize the performance.
>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
>> for testpmd context it can be called via a command on demand by user for
>> optimization purpose.
>
> This won't solve concern I have outlined earlier[1].
>
Yes, it won't.
> I think, The part of the problem there is no enough adaption of
> rte_eth_rx_metadata_negotiate(),
>
> The view is total different from PMD maintainer PoV vs testpmd application PoV.
>
Agree,
and I assume it is different for user application too, which may
prioritize consistency and portability.
Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I
think it is confusing.
> Just to avoid back and forth. We will call off this patch and remove
> rte_eth_rx_metadata_negotiate()
> PMD callback from cnxk driver. Keep it as old behavior, so we don't need to care
> about rte_eth_rx_metadata_negotiate().
>
When you remove 'rx_metadata_negotiate' callback, what will be the PMD
behavior? I assume PMD will do the required preparations as if all Rx
metadata is enabled.
And what is the performance impact, is removing callback improve the
performance?
> [1]
> The only reason is, typically testpmd will be used performance
> benchmarking as an industry standard. It is difficult to tell/educate
> the QA or customers
> that, "BTW if you need to get better performance add more flag to
> testpmd command line".
> To make that worst, only some PMD needs to give the additional
> parameter to get better number.
> And also, testpmd usage will be treated as application modeling.
>
>>
>>
>>
>>
>>>
>>>>>
>>>>>>
>>>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>>>> parameter to get better number.
>>>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>>>
>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>>>>>> situation with sfc driver?
>>>>>>>> Keeping it as negotiated and not use the feature, will impact the per
>>>>>>>> core performance of sfc or
>>>>>>>> is it just PCI bandwidth thing which really dont show any difference in testpmd?
>>>>>>>
>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>>>>>> it is better to negotiate nothing by default. But it is always
>>>>>>> painful to change defaults. You need to explain that now you
>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
>>>>>>> Yes, it will be required on sfc and cnxk only.
>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>>>>>
>>>>>> If we change testpmd defaults to "do nothing",
>>>>>> then we should disable MBUF_FAST_FREE as well.
>>>>>
>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
>>>>> !MBUF_FAST_FREE is doing more work.
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>
>>
On Thu, 2 Feb 2023, Ferruh Yigit wrote:
> On 2/1/2023 3:22 PM, Jerin Jacob wrote:
>> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>
>>> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
>>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>>>
>>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
>>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>>>
>>>>>>> 01/02/2023 11:58, Andrew Rybchenko:
>>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>>> Frankly speaking I don't understand why default value is so
>>>>>>>>>> important if we have a way to change it. Reasons should be
>>>>>>>>>> really strong to change existing defaults.
>>>>>>>>>
>>>>>>>>> The only reason is, typically testpmd will be used performance
>>>>>>>>> benchmarking as an industry standard. It is difficult to tell/educate
>>>>>>>>> the QA or customers
>>>>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>>>>> testpmd command line".
>>>>>>>
>>>>>>> I disagree.
>>>>>>> When you do performance benchmark, you tune settings accordingly.
>>>>>>
>>>>>> IMO, We tune the system resources like queue depth not the disabling
>>>>>> features for raw performance.
>>>>>> queue depth etc people know to tune so it is obvious. What is not
>>>>>> obvious is, testpmd only
>>>>>> negotiated some features by default.I am not using that feature, hence
>>>>>> I need to explicitly
>>>>>> disable it.
>>>>>>
>>>>>
>>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
>>>>> believe that is the case for almost all applications since API is a
>>>>> relatively new one, PMD default behavior should be to enable Rx metadata
>>>>> flow rules, in case user requests them later.
>>>>>
>>>>> So, enabling all in application is same with not calling the API at all.
>>>>>
>>>>> In this perspective, disabling Rx metadata is additional
>>>>> optimization/tuning that application can do if it is sure that Rx
>>>>> metadata flow rules won't be used at all.
>>>>> And API is more meaningful when it is used to disable Rx metadata.
>>>>>
>>>>> I think it is reasonable to enable all Rx metadata by default in testpmd
>>>>> with a capability to disable it when wanted.
>>>>>
>>>>> OR
>>>>>
>>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in
>>>>> testpmd, it is only called when it is requested explicitly from user,
>>>>> enable or disable.
>>>>
>>>> Second option looks good to me.
>>>> When
>>>> 1) user request for action which is needed negotiate(),
>>>> AND
>>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
>>>> then, testpmd print a warning that need to enable
>>>> rte_eth_rx_metadata_negotiate().
>>>>
>>>
>>> We are not suggesting same thing.
>>>
>>> What you described above assumes PMD disabled Rx metadata flow rule
>>> support by default, and it needs to be enabled explicitly by
>>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
>>> functionality.
>>>
>>> As far as I understand PMD wants to disable this flow rule by default
>>> because of performance concerns. But this creates inconsistency between
>>> PMDs, because rest of them will enable this flow rule by default (if it
>>> is supported) and be ready to use it when proper flow rule created.
>>>
>>> With this approach some PMDs will need 'rte_eth_rx_metadata_negotiate()'
>>> to enable Rx metadata flow rules, some won't. This can be confusing for
>>> applications that *some* PMDs require double enabling with specific API
>>> call.
>>>
>>>
>>> Instead what I was trying to suggest is reverse,
>>> all PMDs enable the Rx metadata flow rule by default, and don't require
>>> double enabling.
>>> But if application knows that it won't use Rx metadata flow rule, it can
>>> disable it to optimize the performance.
>>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
>>> for testpmd context it can be called via a command on demand by user for
>>> optimization purpose.
>>
>> This won't solve concern I have outlined earlier[1].
>>
>
> Yes, it won't.
>
>> I think, The part of the problem there is no enough adaption of
>> rte_eth_rx_metadata_negotiate(),
>>
>> The view is total different from PMD maintainer PoV vs testpmd application PoV.
>>
>
> Agree,
> and I assume it is different for user application too, which may
> prioritize consistency and portability.
>
> Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I
> think it is confusing.
Forgive me, in which way is it confusing?
>
>> Just to avoid back and forth. We will call off this patch and remove
>> rte_eth_rx_metadata_negotiate()
>> PMD callback from cnxk driver. Keep it as old behavior, so we don't need to care
>> about rte_eth_rx_metadata_negotiate().
>>
>
> When you remove 'rx_metadata_negotiate' callback, what will be the PMD
> behavior? I assume PMD will do the required preparations as if all Rx
> metadata is enabled.
> And what is the performance impact, is removing callback improve the
> performance?
>
>
>> [1]
>> The only reason is, typically testpmd will be used performance
>> benchmarking as an industry standard. It is difficult to tell/educate
>> the QA or customers
>> that, "BTW if you need to get better performance add more flag to
>> testpmd command line".
>> To make that worst, only some PMD needs to give the additional
>> parameter to get better number.
>> And also, testpmd usage will be treated as application modeling.
>>
>>>
>>>
>>>
>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>>>>> parameter to get better number.
>>>>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>>>>
>>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>>>>>>> situation with sfc driver?
>>>>>>>>> Keeping it as negotiated and not use the feature, will impact the per
>>>>>>>>> core performance of sfc or
>>>>>>>>> is it just PCI bandwidth thing which really dont show any difference in testpmd?
>>>>>>>>
>>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>>>>>>> it is better to negotiate nothing by default. But it is always
>>>>>>>> painful to change defaults. You need to explain that now you
>>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel offloads.
>>>>>>>> Yes, it will be required on sfc and cnxk only.
>>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>>>>>>
>>>>>>> If we change testpmd defaults to "do nothing",
>>>>>>> then we should disable MBUF_FAST_FREE as well.
>>>>>>
>>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
>>>>>> !MBUF_FAST_FREE is doing more work.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>
>
On 2/2/2023 8:50 AM, Ivan Malov wrote:
> On Thu, 2 Feb 2023, Ferruh Yigit wrote:
>
>> On 2/1/2023 3:22 PM, Jerin Jacob wrote:
>>> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>> wrote:
>>>>
>>>> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
>>>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>>>> wrote:
>>>>>>
>>>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
>>>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon
>>>>>>> <thomas@monjalon.net> wrote:
>>>>>>>>
>>>>>>>> 01/02/2023 11:58, Andrew Rybchenko:
>>>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>>>> Frankly speaking I don't understand why default value is so
>>>>>>>>>>> important if we have a way to change it. Reasons should be
>>>>>>>>>>> really strong to change existing defaults.
>>>>>>>>>>
>>>>>>>>>> The only reason is, typically testpmd will be used performance
>>>>>>>>>> benchmarking as an industry standard. It is difficult to
>>>>>>>>>> tell/educate
>>>>>>>>>> the QA or customers
>>>>>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>>>>>> testpmd command line".
>>>>>>>>
>>>>>>>> I disagree.
>>>>>>>> When you do performance benchmark, you tune settings accordingly.
>>>>>>>
>>>>>>> IMO, We tune the system resources like queue depth not the disabling
>>>>>>> features for raw performance.
>>>>>>> queue depth etc people know to tune so it is obvious. What is not
>>>>>>> obvious is, testpmd only
>>>>>>> negotiated some features by default.I am not using that feature,
>>>>>>> hence
>>>>>>> I need to explicitly
>>>>>>> disable it.
>>>>>>>
>>>>>>
>>>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
>>>>>> believe that is the case for almost all applications since API is a
>>>>>> relatively new one, PMD default behavior should be to enable Rx
>>>>>> metadata
>>>>>> flow rules, in case user requests them later.
>>>>>>
>>>>>> So, enabling all in application is same with not calling the API
>>>>>> at all.
>>>>>>
>>>>>> In this perspective, disabling Rx metadata is additional
>>>>>> optimization/tuning that application can do if it is sure that Rx
>>>>>> metadata flow rules won't be used at all.
>>>>>> And API is more meaningful when it is used to disable Rx metadata.
>>>>>>
>>>>>> I think it is reasonable to enable all Rx metadata by default in
>>>>>> testpmd
>>>>>> with a capability to disable it when wanted.
>>>>>>
>>>>>> OR
>>>>>>
>>>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by
>>>>>> default in
>>>>>> testpmd, it is only called when it is requested explicitly from user,
>>>>>> enable or disable.
>>>>>
>>>>> Second option looks good to me.
>>>>> When
>>>>> 1) user request for action which is needed negotiate(),
>>>>> AND
>>>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
>>>>> then, testpmd print a warning that need to enable
>>>>> rte_eth_rx_metadata_negotiate().
>>>>>
>>>>
>>>> We are not suggesting same thing.
>>>>
>>>> What you described above assumes PMD disabled Rx metadata flow rule
>>>> support by default, and it needs to be enabled explicitly by
>>>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
>>>> functionality.
>>>>
>>>> As far as I understand PMD wants to disable this flow rule by default
>>>> because of performance concerns. But this creates inconsistency between
>>>> PMDs, because rest of them will enable this flow rule by default (if it
>>>> is supported) and be ready to use it when proper flow rule created.
>>>>
>>>> With this approach some PMDs will need
>>>> 'rte_eth_rx_metadata_negotiate()'
>>>> to enable Rx metadata flow rules, some won't. This can be confusing for
>>>> applications that *some* PMDs require double enabling with specific API
>>>> call.
>>>>
>>>>
>>>> Instead what I was trying to suggest is reverse,
>>>> all PMDs enable the Rx metadata flow rule by default, and don't require
>>>> double enabling.
>>>> But if application knows that it won't use Rx metadata flow rule, it
>>>> can
>>>> disable it to optimize the performance.
>>>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
>>>> for testpmd context it can be called via a command on demand by user
>>>> for
>>>> optimization purpose.
>>>
>>> This won't solve concern I have outlined earlier[1].
>>>
>>
>> Yes, it won't.
>>
>>> I think, The part of the problem there is no enough adaption of
>>> rte_eth_rx_metadata_negotiate(),
>>>
>>> The view is total different from PMD maintainer PoV vs testpmd
>>> application PoV.
>>>
>>
>> Agree,
>> and I assume it is different for user application too, which may
>> prioritize consistency and portability.
>>
>> Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I
>> think it is confusing.
>
> Forgive me, in which way is it confusing?
>
All other flow rules enabled by creating flow rule, but this one
requires an API to enable it, so I believe it is inconsistent in that way.
From application perspective, assume that it doesn't know NIC details,
should it call this API or not? Without API call should application
assume Rx metadata flow rules are enabled or disabled?
As I understand intention is to get hint from application if it will
require Rx metadata flow rules so that PMD can optimize better, but if
PMD doesn't enable Rx metadata flow rules when this API call is missing
than it becomes a mandatory API to configure the device. But I think it
should be optional for optimization.
Also if application not sure to use this flow rule or not, it will by
default enable all in any case, this will reduce the benefit. As done in
testpmd.
API works in both ways, it request to enable some Rx metadata flow rule,
but based on what PMD returns application can know what device supports,
this also inconsistent with how other flow rules work, we don't have API
to get capability but detect them via flow create/validate.
Can there be a case API returns a flow rule negotiated, but it fails
when tried to create the flow rule, isn't this confusing for application
I think if we continue this approach there can be multiple enable and
capability learning APIs for various flow rules or flow rule groups, and
this can make flow API much more harder to use for applications.
>>
>>> Just to avoid back and forth. We will call off this patch and remove
>>> rte_eth_rx_metadata_negotiate()
>>> PMD callback from cnxk driver. Keep it as old behavior, so we don't
>>> need to care
>>> about rte_eth_rx_metadata_negotiate().
>>>
>>
>> When you remove 'rx_metadata_negotiate' callback, what will be the PMD
>> behavior? I assume PMD will do the required preparations as if all Rx
>> metadata is enabled.
>> And what is the performance impact, is removing callback improve the
>> performance?
>>
>>
>>> [1]
>>> The only reason is, typically testpmd will be used performance
>>> benchmarking as an industry standard. It is difficult to tell/educate
>>> the QA or customers
>>> that, "BTW if you need to get better performance add more flag to
>>> testpmd command line".
>>> To make that worst, only some PMD needs to give the additional
>>> parameter to get better number.
>>> And also, testpmd usage will be treated as application modeling.
>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>>>>>> parameter to get better number.
>>>>>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>>>>>
>>>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>>>>>>>> situation with sfc driver?
>>>>>>>>>> Keeping it as negotiated and not use the feature, will impact
>>>>>>>>>> the per
>>>>>>>>>> core performance of sfc or
>>>>>>>>>> is it just PCI bandwidth thing which really dont show any
>>>>>>>>>> difference in testpmd?
>>>>>>>>>
>>>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>>>>>>>> it is better to negotiate nothing by default. But it is always
>>>>>>>>> painful to change defaults. You need to explain that now you
>>>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel
>>>>>>>>> offloads.
>>>>>>>>> Yes, it will be required on sfc and cnxk only.
>>>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>>>>>>>
>>>>>>>> If we change testpmd defaults to "do nothing",
>>>>>>>> then we should disable MBUF_FAST_FREE as well.
>>>>>>>
>>>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
>>>>>>> !MBUF_FAST_FREE is doing more work.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>>
On Thu, 2 Feb 2023, Ferruh Yigit wrote:
> On 2/2/2023 8:50 AM, Ivan Malov wrote:
>> On Thu, 2 Feb 2023, Ferruh Yigit wrote:
>>
>>> On 2/1/2023 3:22 PM, Jerin Jacob wrote:
>>>> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>>> wrote:
>>>>>
>>>>> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
>>>>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
>>>>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon
>>>>>>>> <thomas@monjalon.net> wrote:
>>>>>>>>>
>>>>>>>>> 01/02/2023 11:58, Andrew Rybchenko:
>>>>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>>>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>>>>> Frankly speaking I don't understand why default value is so
>>>>>>>>>>>> important if we have a way to change it. Reasons should be
>>>>>>>>>>>> really strong to change existing defaults.
>>>>>>>>>>>
>>>>>>>>>>> The only reason is, typically testpmd will be used performance
>>>>>>>>>>> benchmarking as an industry standard. It is difficult to
>>>>>>>>>>> tell/educate
>>>>>>>>>>> the QA or customers
>>>>>>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>>>>>>> testpmd command line".
>>>>>>>>>
>>>>>>>>> I disagree.
>>>>>>>>> When you do performance benchmark, you tune settings accordingly.
>>>>>>>>
>>>>>>>> IMO, We tune the system resources like queue depth not the disabling
>>>>>>>> features for raw performance.
>>>>>>>> queue depth etc people know to tune so it is obvious. What is not
>>>>>>>> obvious is, testpmd only
>>>>>>>> negotiated some features by default.I am not using that feature,
>>>>>>>> hence
>>>>>>>> I need to explicitly
>>>>>>>> disable it.
>>>>>>>>
>>>>>>>
>>>>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
>>>>>>> believe that is the case for almost all applications since API is a
>>>>>>> relatively new one, PMD default behavior should be to enable Rx
>>>>>>> metadata
>>>>>>> flow rules, in case user requests them later.
>>>>>>>
>>>>>>> So, enabling all in application is same with not calling the API
>>>>>>> at all.
>>>>>>>
>>>>>>> In this perspective, disabling Rx metadata is additional
>>>>>>> optimization/tuning that application can do if it is sure that Rx
>>>>>>> metadata flow rules won't be used at all.
>>>>>>> And API is more meaningful when it is used to disable Rx metadata.
>>>>>>>
>>>>>>> I think it is reasonable to enable all Rx metadata by default in
>>>>>>> testpmd
>>>>>>> with a capability to disable it when wanted.
>>>>>>>
>>>>>>> OR
>>>>>>>
>>>>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by
>>>>>>> default in
>>>>>>> testpmd, it is only called when it is requested explicitly from user,
>>>>>>> enable or disable.
>>>>>>
>>>>>> Second option looks good to me.
>>>>>> When
>>>>>> 1) user request for action which is needed negotiate(),
>>>>>> AND
>>>>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
>>>>>> then, testpmd print a warning that need to enable
>>>>>> rte_eth_rx_metadata_negotiate().
>>>>>>
>>>>>
>>>>> We are not suggesting same thing.
>>>>>
>>>>> What you described above assumes PMD disabled Rx metadata flow rule
>>>>> support by default, and it needs to be enabled explicitly by
>>>>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
>>>>> functionality.
>>>>>
>>>>> As far as I understand PMD wants to disable this flow rule by default
>>>>> because of performance concerns. But this creates inconsistency between
>>>>> PMDs, because rest of them will enable this flow rule by default (if it
>>>>> is supported) and be ready to use it when proper flow rule created.
>>>>>
>>>>> With this approach some PMDs will need
>>>>> 'rte_eth_rx_metadata_negotiate()'
>>>>> to enable Rx metadata flow rules, some won't. This can be confusing for
>>>>> applications that *some* PMDs require double enabling with specific API
>>>>> call.
>>>>>
>>>>>
>>>>> Instead what I was trying to suggest is reverse,
>>>>> all PMDs enable the Rx metadata flow rule by default, and don't require
>>>>> double enabling.
>>>>> But if application knows that it won't use Rx metadata flow rule, it
>>>>> can
>>>>> disable it to optimize the performance.
>>>>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
>>>>> for testpmd context it can be called via a command on demand by user
>>>>> for
>>>>> optimization purpose.
>>>>
>>>> This won't solve concern I have outlined earlier[1].
>>>>
>>>
>>> Yes, it won't.
>>>
>>>> I think, The part of the problem there is no enough adaption of
>>>> rte_eth_rx_metadata_negotiate(),
>>>>
>>>> The view is total different from PMD maintainer PoV vs testpmd
>>>> application PoV.
>>>>
>>>
>>> Agree,
>>> and I assume it is different for user application too, which may
>>> prioritize consistency and portability.
>>>
>>> Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I
>>> think it is confusing.
>>
>> Forgive me, in which way is it confusing?
>>
>
> All other flow rules enabled by creating flow rule, but this one
Hold on.. Did you just say "flow rules"? But this API is *not* about
flow rules. I suggest you please re-read description of the API.
It reads: "Negotiate the NIC's ability to deliver specific
kinds of metadata to the PMD". Nothing about flows there.
Why is it decoupled from flow library this way? Because there is
a drastic difference between generating and plumbing metadata
INSIDE the NIC's flow engine, on the one hand, and delivering
these data from the NIC to the host driver, on the other.
Let me explain. Say, one creates the following flow rules:
(a) flow create 0 transfer group 0 pattern [A] / end \
actions mark id XXX jump group 1 / end
(b) flow create 0 transfer group 0 pattern [B] / end \
actions mark id YYY jump group 1 / end
(c) flow create 0 transfer group 1 pattern mark id is XXX / end \
actions represented_port ethdev_id 1 / end
(d) flow create 0 transfer group 1 pattern mark id is YYY / end \
actions drop / end
In this example, metadata of type "MARK" is used to partition
flow group (table) 1 into multiple lookup sections. So the
mark value is GENERATED by the flow engine and then it is
CONSUMED by this very flow engine. The application may
NOT necessarily want to receive the mark with mbufs...
And it is only when the application wants these metadata
DELIVERED to it that it has to call the negotiate API.
The short of it, nothing prevents the driver from accepting
flow create requests that leverage some meta items/actions.
Drivers do not need the negotiate API to *configure flow*.
They only need this API in order to let the application
choose whether metadata will be DELIVERED (!) or not.
> requires an API to enable it, so I believe it is inconsistent in that way.
Please see above. Everything is consistent as *flow library*
and *negotiate metadata delivery* API are totally decoupled.
>
> From application perspective, assume that it doesn't know NIC details,
> should it call this API or not? Without API call should application
> assume Rx metadata flow rules are enabled or disabled?
Frankly, applications like testpmd need never call this API.
Simply because seeing, for example, MARK values in mbufs is
useless to it. This API is needed by other applications.
For example, OvS has partial MARK+RSS offload. It adds
flows that distribute packets across multiple queues
and set some MARK values for them. OvS is interested
in getting this values with mbufs since they affect
further lookups in software... So, OvS, knowing it
wants these metadata DELIVERED (!), should invoke
this metadata negotiate API to ensure that.
>
>
> As I understand intention is to get hint from application if it will
> require Rx metadata flow rules so that PMD can optimize better, but if
No, nothing about flow rules. Just delivery of metadata with mbufs.
> PMD doesn't enable Rx metadata flow rules when this API call is missing
Again, PMD shall not make decisions on whether to enable or disable
support for some FLOW primitives based on interactions via this API.
This API exists solely to let PMD configure delivery of metadata,
i.e. not the way it is generated in the first instance.
> than it becomes a mandatory API to configure the device. But I think it
> should be optional for optimization.
>
> Also if application not sure to use this flow rule or not, it will by
> default enable all in any case, this will reduce the benefit. As done in
> testpmd.
Please see above. PMD does not need this API, I take it.
>
>
> API works in both ways, it request to enable some Rx metadata flow rule,
> but based on what PMD returns application can know what device supports,
> this also inconsistent with how other flow rules work, we don't have API
> to get capability but detect them via flow create/validate.
> Can there be a case API returns a flow rule negotiated, but it fails
> when tried to create the flow rule, isn't this confusing for application
>
>
> I think if we continue this approach there can be multiple enable and
> capability learning APIs for various flow rules or flow rule groups, and
> this can make flow API much more harder to use for applications.
See my explanations above. This API is not about flows. Period.
>
>
>
>>>
>>>> Just to avoid back and forth. We will call off this patch and remove
>>>> rte_eth_rx_metadata_negotiate()
>>>> PMD callback from cnxk driver. Keep it as old behavior, so we don't
>>>> need to care
>>>> about rte_eth_rx_metadata_negotiate().
>>>>
>>>
>>> When you remove 'rx_metadata_negotiate' callback, what will be the PMD
>>> behavior? I assume PMD will do the required preparations as if all Rx
>>> metadata is enabled.
>>> And what is the performance impact, is removing callback improve the
>>> performance?
>>>
>>>
>>>> [1]
>>>> The only reason is, typically testpmd will be used performance
>>>> benchmarking as an industry standard. It is difficult to tell/educate
>>>> the QA or customers
>>>> that, "BTW if you need to get better performance add more flag to
>>>> testpmd command line".
>>>> To make that worst, only some PMD needs to give the additional
>>>> parameter to get better number.
>>>> And also, testpmd usage will be treated as application modeling.
>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>>>>>>> parameter to get better number.
>>>>>>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>>>>>>
>>>>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>>>>>>>>> situation with sfc driver?
>>>>>>>>>>> Keeping it as negotiated and not use the feature, will impact
>>>>>>>>>>> the per
>>>>>>>>>>> core performance of sfc or
>>>>>>>>>>> is it just PCI bandwidth thing which really dont show any
>>>>>>>>>>> difference in testpmd?
>>>>>>>>>>
>>>>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>>>>>>>>> it is better to negotiate nothing by default. But it is always
>>>>>>>>>> painful to change defaults. You need to explain that now you
>>>>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel
>>>>>>>>>> offloads.
>>>>>>>>>> Yes, it will be required on sfc and cnxk only.
>>>>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>>>>>>>>
>>>>>>>>> If we change testpmd defaults to "do nothing",
>>>>>>>>> then we should disable MBUF_FAST_FREE as well.
>>>>>>>>
>>>>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
>>>>>>>> !MBUF_FAST_FREE is doing more work.
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>>
>
>
I apologise, there was a typo in the previous mail: "PMD does
not need this API". Should read as "TESTPMD does not need it".
Thank you.
On Thu, 2 Feb 2023, Ivan Malov wrote:
> On Thu, 2 Feb 2023, Ferruh Yigit wrote:
>
>> On 2/2/2023 8:50 AM, Ivan Malov wrote:
>>> On Thu, 2 Feb 2023, Ferruh Yigit wrote:
>>>
>>>> On 2/1/2023 3:22 PM, Jerin Jacob wrote:
>>>>> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>>>> wrote:
>>>>>>
>>>>>> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
>>>>>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
>>>>>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon
>>>>>>>>> <thomas@monjalon.net> wrote:
>>>>>>>>>>
>>>>>>>>>> 01/02/2023 11:58, Andrew Rybchenko:
>>>>>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>>>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>>>>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>>>>>> Frankly speaking I don't understand why default value is so
>>>>>>>>>>>>> important if we have a way to change it. Reasons should be
>>>>>>>>>>>>> really strong to change existing defaults.
>>>>>>>>>>>>
>>>>>>>>>>>> The only reason is, typically testpmd will be used performance
>>>>>>>>>>>> benchmarking as an industry standard. It is difficult to
>>>>>>>>>>>> tell/educate
>>>>>>>>>>>> the QA or customers
>>>>>>>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>>>>>>>> testpmd command line".
>>>>>>>>>>
>>>>>>>>>> I disagree.
>>>>>>>>>> When you do performance benchmark, you tune settings accordingly.
>>>>>>>>>
>>>>>>>>> IMO, We tune the system resources like queue depth not the disabling
>>>>>>>>> features for raw performance.
>>>>>>>>> queue depth etc people know to tune so it is obvious. What is not
>>>>>>>>> obvious is, testpmd only
>>>>>>>>> negotiated some features by default.I am not using that feature,
>>>>>>>>> hence
>>>>>>>>> I need to explicitly
>>>>>>>>> disable it.
>>>>>>>>>
>>>>>>>>
>>>>>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
>>>>>>>> believe that is the case for almost all applications since API is a
>>>>>>>> relatively new one, PMD default behavior should be to enable Rx
>>>>>>>> metadata
>>>>>>>> flow rules, in case user requests them later.
>>>>>>>>
>>>>>>>> So, enabling all in application is same with not calling the API
>>>>>>>> at all.
>>>>>>>>
>>>>>>>> In this perspective, disabling Rx metadata is additional
>>>>>>>> optimization/tuning that application can do if it is sure that Rx
>>>>>>>> metadata flow rules won't be used at all.
>>>>>>>> And API is more meaningful when it is used to disable Rx metadata.
>>>>>>>>
>>>>>>>> I think it is reasonable to enable all Rx metadata by default in
>>>>>>>> testpmd
>>>>>>>> with a capability to disable it when wanted.
>>>>>>>>
>>>>>>>> OR
>>>>>>>>
>>>>>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by
>>>>>>>> default in
>>>>>>>> testpmd, it is only called when it is requested explicitly from user,
>>>>>>>> enable or disable.
>>>>>>>
>>>>>>> Second option looks good to me.
>>>>>>> When
>>>>>>> 1) user request for action which is needed negotiate(),
>>>>>>> AND
>>>>>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
>>>>>>> then, testpmd print a warning that need to enable
>>>>>>> rte_eth_rx_metadata_negotiate().
>>>>>>>
>>>>>>
>>>>>> We are not suggesting same thing.
>>>>>>
>>>>>> What you described above assumes PMD disabled Rx metadata flow rule
>>>>>> support by default, and it needs to be enabled explicitly by
>>>>>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
>>>>>> functionality.
>>>>>>
>>>>>> As far as I understand PMD wants to disable this flow rule by default
>>>>>> because of performance concerns. But this creates inconsistency between
>>>>>> PMDs, because rest of them will enable this flow rule by default (if it
>>>>>> is supported) and be ready to use it when proper flow rule created.
>>>>>>
>>>>>> With this approach some PMDs will need
>>>>>> 'rte_eth_rx_metadata_negotiate()'
>>>>>> to enable Rx metadata flow rules, some won't. This can be confusing for
>>>>>> applications that *some* PMDs require double enabling with specific API
>>>>>> call.
>>>>>>
>>>>>>
>>>>>> Instead what I was trying to suggest is reverse,
>>>>>> all PMDs enable the Rx metadata flow rule by default, and don't require
>>>>>> double enabling.
>>>>>> But if application knows that it won't use Rx metadata flow rule, it
>>>>>> can
>>>>>> disable it to optimize the performance.
>>>>>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
>>>>>> for testpmd context it can be called via a command on demand by user
>>>>>> for
>>>>>> optimization purpose.
>>>>>
>>>>> This won't solve concern I have outlined earlier[1].
>>>>>
>>>>
>>>> Yes, it won't.
>>>>
>>>>> I think, The part of the problem there is no enough adaption of
>>>>> rte_eth_rx_metadata_negotiate(),
>>>>>
>>>>> The view is total different from PMD maintainer PoV vs testpmd
>>>>> application PoV.
>>>>>
>>>>
>>>> Agree,
>>>> and I assume it is different for user application too, which may
>>>> prioritize consistency and portability.
>>>>
>>>> Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I
>>>> think it is confusing.
>>>
>>> Forgive me, in which way is it confusing?
>>>
>>
>> All other flow rules enabled by creating flow rule, but this one
>
> Hold on.. Did you just say "flow rules"? But this API is *not* about
> flow rules. I suggest you please re-read description of the API.
> It reads: "Negotiate the NIC's ability to deliver specific
> kinds of metadata to the PMD". Nothing about flows there.
>
> Why is it decoupled from flow library this way? Because there is
> a drastic difference between generating and plumbing metadata
> INSIDE the NIC's flow engine, on the one hand, and delivering
> these data from the NIC to the host driver, on the other.
>
> Let me explain. Say, one creates the following flow rules:
>
> (a) flow create 0 transfer group 0 pattern [A] / end \
> actions mark id XXX jump group 1 / end
> (b) flow create 0 transfer group 0 pattern [B] / end \
> actions mark id YYY jump group 1 / end
>
> (c) flow create 0 transfer group 1 pattern mark id is XXX / end \
> actions represented_port ethdev_id 1 / end
> (d) flow create 0 transfer group 1 pattern mark id is YYY / end \
> actions drop / end
>
> In this example, metadata of type "MARK" is used to partition
> flow group (table) 1 into multiple lookup sections. So the
> mark value is GENERATED by the flow engine and then it is
> CONSUMED by this very flow engine. The application may
> NOT necessarily want to receive the mark with mbufs...
>
> And it is only when the application wants these metadata
> DELIVERED to it that it has to call the negotiate API.
>
> The short of it, nothing prevents the driver from accepting
> flow create requests that leverage some meta items/actions.
> Drivers do not need the negotiate API to *configure flow*.
> They only need this API in order to let the application
> choose whether metadata will be DELIVERED (!) or not.
>
>> requires an API to enable it, so I believe it is inconsistent in that way.
>
> Please see above. Everything is consistent as *flow library*
> and *negotiate metadata delivery* API are totally decoupled.
>
>>
>> From application perspective, assume that it doesn't know NIC details,
>> should it call this API or not? Without API call should application
>> assume Rx metadata flow rules are enabled or disabled?
>
> Frankly, applications like testpmd need never call this API.
> Simply because seeing, for example, MARK values in mbufs is
> useless to it. This API is needed by other applications.
> For example, OvS has partial MARK+RSS offload. It adds
> flows that distribute packets across multiple queues
> and set some MARK values for them. OvS is interested
> in getting this values with mbufs since they affect
> further lookups in software... So, OvS, knowing it
> wants these metadata DELIVERED (!), should invoke
> this metadata negotiate API to ensure that.
>
>>
>>
>> As I understand intention is to get hint from application if it will
>> require Rx metadata flow rules so that PMD can optimize better, but if
>
> No, nothing about flow rules. Just delivery of metadata with mbufs.
>
>> PMD doesn't enable Rx metadata flow rules when this API call is missing
>
> Again, PMD shall not make decisions on whether to enable or disable
> support for some FLOW primitives based on interactions via this API.
> This API exists solely to let PMD configure delivery of metadata,
> i.e. not the way it is generated in the first instance.
>
>> than it becomes a mandatory API to configure the device. But I think it
>> should be optional for optimization.
>>
>> Also if application not sure to use this flow rule or not, it will by
>> default enable all in any case, this will reduce the benefit. As done in
>> testpmd.
>
> Please see above. PMD does not need this API, I take it.
>
>>
>>
>> API works in both ways, it request to enable some Rx metadata flow rule,
>> but based on what PMD returns application can know what device supports,
>> this also inconsistent with how other flow rules work, we don't have API
>> to get capability but detect them via flow create/validate.
>> Can there be a case API returns a flow rule negotiated, but it fails
>> when tried to create the flow rule, isn't this confusing for application
>>
>>
>> I think if we continue this approach there can be multiple enable and
>> capability learning APIs for various flow rules or flow rule groups, and
>> this can make flow API much more harder to use for applications.
>
> See my explanations above. This API is not about flows. Period.
>
>>
>>
>>
>>>>
>>>>> Just to avoid back and forth. We will call off this patch and remove
>>>>> rte_eth_rx_metadata_negotiate()
>>>>> PMD callback from cnxk driver. Keep it as old behavior, so we don't
>>>>> need to care
>>>>> about rte_eth_rx_metadata_negotiate().
>>>>>
>>>>
>>>> When you remove 'rx_metadata_negotiate' callback, what will be the PMD
>>>> behavior? I assume PMD will do the required preparations as if all Rx
>>>> metadata is enabled.
>>>> And what is the performance impact, is removing callback improve the
>>>> performance?
>>>>
>>>>
>>>>> [1]
>>>>> The only reason is, typically testpmd will be used performance
>>>>> benchmarking as an industry standard. It is difficult to tell/educate
>>>>> the QA or customers
>>>>> that, "BTW if you need to get better performance add more flag to
>>>>> testpmd command line".
>>>>> To make that worst, only some PMD needs to give the additional
>>>>> parameter to get better number.
>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>>>>>>>> parameter to get better number.
>>>>>>>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>>>>>>>
>>>>>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>>>>>>>>>> situation with sfc driver?
>>>>>>>>>>>> Keeping it as negotiated and not use the feature, will impact
>>>>>>>>>>>> the per
>>>>>>>>>>>> core performance of sfc or
>>>>>>>>>>>> is it just PCI bandwidth thing which really dont show any
>>>>>>>>>>>> difference in testpmd?
>>>>>>>>>>>
>>>>>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>>>>>>>>>> it is better to negotiate nothing by default. But it is always
>>>>>>>>>>> painful to change defaults. You need to explain that now you
>>>>>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel
>>>>>>>>>>> offloads.
>>>>>>>>>>> Yes, it will be required on sfc and cnxk only.
>>>>>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>>>>>>>>>
>>>>>>>>>> If we change testpmd defaults to "do nothing",
>>>>>>>>>> then we should disable MBUF_FAST_FREE as well.
>>>>>>>>>
>>>>>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
>>>>>>>>> !MBUF_FAST_FREE is doing more work.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>
OK we are progressing on this topic :)
02/02/2023 11:48, Ivan Malov:
> I apologise, there was a typo in the previous mail: "PMD does
> not need this API". Should read as "TESTPMD does not need it".
testpmd needs all ethdev API,
because its purpose is to test the whole all ethdev API.
Maybe the use of this function is misplaced in testpmd.
It should be a specific command.
By the way, what is the driver default if negotiation is not done?
> On Thu, 2 Feb 2023, Ivan Malov wrote:
> > On Thu, 2 Feb 2023, Ferruh Yigit wrote:
> >
> >> On 2/2/2023 8:50 AM, Ivan Malov wrote:
> >>> On Thu, 2 Feb 2023, Ferruh Yigit wrote:
> >>>
> >>>> On 2/1/2023 3:22 PM, Jerin Jacob wrote:
> >>>>> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
> >>>>>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
> >>>>>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon
> >>>>>>>>> <thomas@monjalon.net> wrote:
> >>>>>>>>>>
> >>>>>>>>>> 01/02/2023 11:58, Andrew Rybchenko:
> >>>>>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
> >>>>>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
> >>>>>>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
> >>>>>>>>>>>>> Frankly speaking I don't understand why default value is so
> >>>>>>>>>>>>> important if we have a way to change it. Reasons should be
> >>>>>>>>>>>>> really strong to change existing defaults.
> >>>>>>>>>>>>
> >>>>>>>>>>>> The only reason is, typically testpmd will be used performance
> >>>>>>>>>>>> benchmarking as an industry standard. It is difficult to
> >>>>>>>>>>>> tell/educate
> >>>>>>>>>>>> the QA or customers
> >>>>>>>>>>>> that, "BTW if you need to get better performance add more flag to
> >>>>>>>>>>>> testpmd command line".
> >>>>>>>>>>
> >>>>>>>>>> I disagree.
> >>>>>>>>>> When you do performance benchmark, you tune settings accordingly.
> >>>>>>>>>
> >>>>>>>>> IMO, We tune the system resources like queue depth not the disabling
> >>>>>>>>> features for raw performance.
> >>>>>>>>> queue depth etc people know to tune so it is obvious. What is not
> >>>>>>>>> obvious is, testpmd only
> >>>>>>>>> negotiated some features by default.I am not using that feature,
> >>>>>>>>> hence
> >>>>>>>>> I need to explicitly
> >>>>>>>>> disable it.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
> >>>>>>>> believe that is the case for almost all applications since API is a
> >>>>>>>> relatively new one, PMD default behavior should be to enable Rx
> >>>>>>>> metadata
> >>>>>>>> flow rules, in case user requests them later.
> >>>>>>>>
> >>>>>>>> So, enabling all in application is same with not calling the API
> >>>>>>>> at all.
> >>>>>>>>
> >>>>>>>> In this perspective, disabling Rx metadata is additional
> >>>>>>>> optimization/tuning that application can do if it is sure that Rx
> >>>>>>>> metadata flow rules won't be used at all.
> >>>>>>>> And API is more meaningful when it is used to disable Rx metadata.
> >>>>>>>>
> >>>>>>>> I think it is reasonable to enable all Rx metadata by default in
> >>>>>>>> testpmd
> >>>>>>>> with a capability to disable it when wanted.
> >>>>>>>>
> >>>>>>>> OR
> >>>>>>>>
> >>>>>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by
> >>>>>>>> default in
> >>>>>>>> testpmd, it is only called when it is requested explicitly from user,
> >>>>>>>> enable or disable.
> >>>>>>>
> >>>>>>> Second option looks good to me.
> >>>>>>> When
> >>>>>>> 1) user request for action which is needed negotiate(),
> >>>>>>> AND
> >>>>>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
> >>>>>>> then, testpmd print a warning that need to enable
> >>>>>>> rte_eth_rx_metadata_negotiate().
> >>>>>>>
> >>>>>>
> >>>>>> We are not suggesting same thing.
> >>>>>>
> >>>>>> What you described above assumes PMD disabled Rx metadata flow rule
> >>>>>> support by default, and it needs to be enabled explicitly by
> >>>>>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
> >>>>>> functionality.
> >>>>>>
> >>>>>> As far as I understand PMD wants to disable this flow rule by default
> >>>>>> because of performance concerns. But this creates inconsistency between
> >>>>>> PMDs, because rest of them will enable this flow rule by default (if it
> >>>>>> is supported) and be ready to use it when proper flow rule created.
> >>>>>>
> >>>>>> With this approach some PMDs will need
> >>>>>> 'rte_eth_rx_metadata_negotiate()'
> >>>>>> to enable Rx metadata flow rules, some won't. This can be confusing for
> >>>>>> applications that *some* PMDs require double enabling with specific API
> >>>>>> call.
> >>>>>>
> >>>>>>
> >>>>>> Instead what I was trying to suggest is reverse,
> >>>>>> all PMDs enable the Rx metadata flow rule by default, and don't require
> >>>>>> double enabling.
> >>>>>> But if application knows that it won't use Rx metadata flow rule, it
> >>>>>> can
> >>>>>> disable it to optimize the performance.
> >>>>>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
> >>>>>> for testpmd context it can be called via a command on demand by user
> >>>>>> for
> >>>>>> optimization purpose.
> >>>>>
> >>>>> This won't solve concern I have outlined earlier[1].
> >>>>>
> >>>>
> >>>> Yes, it won't.
> >>>>
> >>>>> I think, The part of the problem there is no enough adaption of
> >>>>> rte_eth_rx_metadata_negotiate(),
> >>>>>
> >>>>> The view is total different from PMD maintainer PoV vs testpmd
> >>>>> application PoV.
> >>>>>
> >>>>
> >>>> Agree,
> >>>> and I assume it is different for user application too, which may
> >>>> prioritize consistency and portability.
> >>>>
> >>>> Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I
> >>>> think it is confusing.
> >>>
> >>> Forgive me, in which way is it confusing?
> >>>
> >>
> >> All other flow rules enabled by creating flow rule, but this one
> >
> > Hold on.. Did you just say "flow rules"? But this API is *not* about
> > flow rules. I suggest you please re-read description of the API.
> > It reads: "Negotiate the NIC's ability to deliver specific
> > kinds of metadata to the PMD". Nothing about flows there.
> >
> > Why is it decoupled from flow library this way? Because there is
> > a drastic difference between generating and plumbing metadata
> > INSIDE the NIC's flow engine, on the one hand, and delivering
> > these data from the NIC to the host driver, on the other.
> >
> > Let me explain. Say, one creates the following flow rules:
> >
> > (a) flow create 0 transfer group 0 pattern [A] / end \
> > actions mark id XXX jump group 1 / end
> > (b) flow create 0 transfer group 0 pattern [B] / end \
> > actions mark id YYY jump group 1 / end
> >
> > (c) flow create 0 transfer group 1 pattern mark id is XXX / end \
> > actions represented_port ethdev_id 1 / end
> > (d) flow create 0 transfer group 1 pattern mark id is YYY / end \
> > actions drop / end
> >
> > In this example, metadata of type "MARK" is used to partition
> > flow group (table) 1 into multiple lookup sections. So the
> > mark value is GENERATED by the flow engine and then it is
> > CONSUMED by this very flow engine. The application may
> > NOT necessarily want to receive the mark with mbufs...
> >
> > And it is only when the application wants these metadata
> > DELIVERED to it that it has to call the negotiate API.
> >
> > The short of it, nothing prevents the driver from accepting
> > flow create requests that leverage some meta items/actions.
> > Drivers do not need the negotiate API to *configure flow*.
> > They only need this API in order to let the application
> > choose whether metadata will be DELIVERED (!) or not.
> >
> >> requires an API to enable it, so I believe it is inconsistent in that way.
> >
> > Please see above. Everything is consistent as *flow library*
> > and *negotiate metadata delivery* API are totally decoupled.
> >
> >>
> >> From application perspective, assume that it doesn't know NIC details,
> >> should it call this API or not? Without API call should application
> >> assume Rx metadata flow rules are enabled or disabled?
> >
> > Frankly, applications like testpmd need never call this API.
> > Simply because seeing, for example, MARK values in mbufs is
> > useless to it. This API is needed by other applications.
> > For example, OvS has partial MARK+RSS offload. It adds
> > flows that distribute packets across multiple queues
> > and set some MARK values for them. OvS is interested
> > in getting this values with mbufs since they affect
> > further lookups in software... So, OvS, knowing it
> > wants these metadata DELIVERED (!), should invoke
> > this metadata negotiate API to ensure that.
> >
> >>
> >>
> >> As I understand intention is to get hint from application if it will
> >> require Rx metadata flow rules so that PMD can optimize better, but if
> >
> > No, nothing about flow rules. Just delivery of metadata with mbufs.
> >
> >> PMD doesn't enable Rx metadata flow rules when this API call is missing
> >
> > Again, PMD shall not make decisions on whether to enable or disable
> > support for some FLOW primitives based on interactions via this API.
> > This API exists solely to let PMD configure delivery of metadata,
> > i.e. not the way it is generated in the first instance.
> >
> >> than it becomes a mandatory API to configure the device. But I think it
> >> should be optional for optimization.
> >>
> >> Also if application not sure to use this flow rule or not, it will by
> >> default enable all in any case, this will reduce the benefit. As done in
> >> testpmd.
> >
> > Please see above. PMD does not need this API, I take it.
> >
> >>
> >>
> >> API works in both ways, it request to enable some Rx metadata flow rule,
> >> but based on what PMD returns application can know what device supports,
> >> this also inconsistent with how other flow rules work, we don't have API
> >> to get capability but detect them via flow create/validate.
> >> Can there be a case API returns a flow rule negotiated, but it fails
> >> when tried to create the flow rule, isn't this confusing for application
> >>
> >>
> >> I think if we continue this approach there can be multiple enable and
> >> capability learning APIs for various flow rules or flow rule groups, and
> >> this can make flow API much more harder to use for applications.
> >
> > See my explanations above. This API is not about flows. Period.
> >
> >>
> >>
> >>
> >>>>
> >>>>> Just to avoid back and forth. We will call off this patch and remove
> >>>>> rte_eth_rx_metadata_negotiate()
> >>>>> PMD callback from cnxk driver. Keep it as old behavior, so we don't
> >>>>> need to care
> >>>>> about rte_eth_rx_metadata_negotiate().
> >>>>>
> >>>>
> >>>> When you remove 'rx_metadata_negotiate' callback, what will be the PMD
> >>>> behavior? I assume PMD will do the required preparations as if all Rx
> >>>> metadata is enabled.
> >>>> And what is the performance impact, is removing callback improve the
> >>>> performance?
> >>>>
> >>>>
> >>>>> [1]
> >>>>> The only reason is, typically testpmd will be used performance
> >>>>> benchmarking as an industry standard. It is difficult to tell/educate
> >>>>> the QA or customers
> >>>>> that, "BTW if you need to get better performance add more flag to
> >>>>> testpmd command line".
> >>>>> To make that worst, only some PMD needs to give the additional
> >>>>> parameter to get better number.
> >>>>> And also, testpmd usage will be treated as application modeling.
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>> To make that worst, only some PMD needs to give the additional
> >>>>>>>>>>>> parameter to get better number.
> >>>>>>>>>>>> And also, testpmd usage will be treated as application modeling.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the
> >>>>>>>>>>>> situation with sfc driver?
> >>>>>>>>>>>> Keeping it as negotiated and not use the feature, will impact
> >>>>>>>>>>>> the per
> >>>>>>>>>>>> core performance of sfc or
> >>>>>>>>>>>> is it just PCI bandwidth thing which really dont show any
> >>>>>>>>>>>> difference in testpmd?
> >>>>>>>>>>>
> >>>>>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
> >>>>>>>>>>> it is better to negotiate nothing by default. But it is always
> >>>>>>>>>>> painful to change defaults. You need to explain that now you
> >>>>>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel
> >>>>>>>>>>> offloads.
> >>>>>>>>>>> Yes, it will be required on sfc and cnxk only.
> >>>>>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
> >>>>>>>>>>
> >>>>>>>>>> If we change testpmd defaults to "do nothing",
> >>>>>>>>>> then we should disable MBUF_FAST_FREE as well.
> >>>>>>>>>
> >>>>>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
> >>>>>>>>> !MBUF_FAST_FREE is doing more work.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>
> >>>>
> >>
> >>
> >
>
On Thu, 2 Feb 2023, Thomas Monjalon wrote:
> OK we are progressing on this topic :)
Indeed we are.
>
> 02/02/2023 11:48, Ivan Malov:
>> I apologise, there was a typo in the previous mail: "PMD does
>> not need this API". Should read as "TESTPMD does not need it".
>
> testpmd needs all ethdev API,
> because its purpose is to test the whole all ethdev API.
Touché.
>
> Maybe the use of this function is misplaced in testpmd.
> It should be a specific command.
So.. indeed. A specific command which, as I said, is
invoked explicitly by the person operating testpmd.
> By the way, what is the driver default if negotiation is not done?
The answer is in the question. It's the driver's default.
If the driver believes it shall NOT deliver metadata for
the sake of improved performance, default = all disabled.
If this delivery is a don't care to performance, then
the driver might want to enable everything by default.
This decision is made by a PMD maintainer, I take it.
>
>
>> On Thu, 2 Feb 2023, Ivan Malov wrote:
>>> On Thu, 2 Feb 2023, Ferruh Yigit wrote:
>>>
>>>> On 2/2/2023 8:50 AM, Ivan Malov wrote:
>>>>> On Thu, 2 Feb 2023, Ferruh Yigit wrote:
>>>>>
>>>>>> On 2/1/2023 3:22 PM, Jerin Jacob wrote:
>>>>>>> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 2/1/2023 1:48 PM, Jerin Jacob wrote:
>>>>>>>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yigit@amd.com>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote:
>>>>>>>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon
>>>>>>>>>>> <thomas@monjalon.net> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> 01/02/2023 11:58, Andrew Rybchenko:
>>>>>>>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote:
>>>>>>>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
>>>>>>>>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>>>>>>>> Frankly speaking I don't understand why default value is so
>>>>>>>>>>>>>>> important if we have a way to change it. Reasons should be
>>>>>>>>>>>>>>> really strong to change existing defaults.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The only reason is, typically testpmd will be used performance
>>>>>>>>>>>>>> benchmarking as an industry standard. It is difficult to
>>>>>>>>>>>>>> tell/educate
>>>>>>>>>>>>>> the QA or customers
>>>>>>>>>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>>>>>>>>>> testpmd command line".
>>>>>>>>>>>>
>>>>>>>>>>>> I disagree.
>>>>>>>>>>>> When you do performance benchmark, you tune settings accordingly.
>>>>>>>>>>>
>>>>>>>>>>> IMO, We tune the system resources like queue depth not the disabling
>>>>>>>>>>> features for raw performance.
>>>>>>>>>>> queue depth etc people know to tune so it is obvious. What is not
>>>>>>>>>>> obvious is, testpmd only
>>>>>>>>>>> negotiated some features by default.I am not using that feature,
>>>>>>>>>>> hence
>>>>>>>>>>> I need to explicitly
>>>>>>>>>>> disable it.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I
>>>>>>>>>> believe that is the case for almost all applications since API is a
>>>>>>>>>> relatively new one, PMD default behavior should be to enable Rx
>>>>>>>>>> metadata
>>>>>>>>>> flow rules, in case user requests them later.
>>>>>>>>>>
>>>>>>>>>> So, enabling all in application is same with not calling the API
>>>>>>>>>> at all.
>>>>>>>>>>
>>>>>>>>>> In this perspective, disabling Rx metadata is additional
>>>>>>>>>> optimization/tuning that application can do if it is sure that Rx
>>>>>>>>>> metadata flow rules won't be used at all.
>>>>>>>>>> And API is more meaningful when it is used to disable Rx metadata.
>>>>>>>>>>
>>>>>>>>>> I think it is reasonable to enable all Rx metadata by default in
>>>>>>>>>> testpmd
>>>>>>>>>> with a capability to disable it when wanted.
>>>>>>>>>>
>>>>>>>>>> OR
>>>>>>>>>>
>>>>>>>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by
>>>>>>>>>> default in
>>>>>>>>>> testpmd, it is only called when it is requested explicitly from user,
>>>>>>>>>> enable or disable.
>>>>>>>>>
>>>>>>>>> Second option looks good to me.
>>>>>>>>> When
>>>>>>>>> 1) user request for action which is needed negotiate(),
>>>>>>>>> AND
>>>>>>>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP
>>>>>>>>> then, testpmd print a warning that need to enable
>>>>>>>>> rte_eth_rx_metadata_negotiate().
>>>>>>>>>
>>>>>>>>
>>>>>>>> We are not suggesting same thing.
>>>>>>>>
>>>>>>>> What you described above assumes PMD disabled Rx metadata flow rule
>>>>>>>> support by default, and it needs to be enabled explicitly by
>>>>>>>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for
>>>>>>>> functionality.
>>>>>>>>
>>>>>>>> As far as I understand PMD wants to disable this flow rule by default
>>>>>>>> because of performance concerns. But this creates inconsistency between
>>>>>>>> PMDs, because rest of them will enable this flow rule by default (if it
>>>>>>>> is supported) and be ready to use it when proper flow rule created.
>>>>>>>>
>>>>>>>> With this approach some PMDs will need
>>>>>>>> 'rte_eth_rx_metadata_negotiate()'
>>>>>>>> to enable Rx metadata flow rules, some won't. This can be confusing for
>>>>>>>> applications that *some* PMDs require double enabling with specific API
>>>>>>>> call.
>>>>>>>>
>>>>>>>>
>>>>>>>> Instead what I was trying to suggest is reverse,
>>>>>>>> all PMDs enable the Rx metadata flow rule by default, and don't require
>>>>>>>> double enabling.
>>>>>>>> But if application knows that it won't use Rx metadata flow rule, it
>>>>>>>> can
>>>>>>>> disable it to optimize the performance.
>>>>>>>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and
>>>>>>>> for testpmd context it can be called via a command on demand by user
>>>>>>>> for
>>>>>>>> optimization purpose.
>>>>>>>
>>>>>>> This won't solve concern I have outlined earlier[1].
>>>>>>>
>>>>>>
>>>>>> Yes, it won't.
>>>>>>
>>>>>>> I think, The part of the problem there is no enough adaption of
>>>>>>> rte_eth_rx_metadata_negotiate(),
>>>>>>>
>>>>>>> The view is total different from PMD maintainer PoV vs testpmd
>>>>>>> application PoV.
>>>>>>>
>>>>>>
>>>>>> Agree,
>>>>>> and I assume it is different for user application too, which may
>>>>>> prioritize consistency and portability.
>>>>>>
>>>>>> Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I
>>>>>> think it is confusing.
>>>>>
>>>>> Forgive me, in which way is it confusing?
>>>>>
>>>>
>>>> All other flow rules enabled by creating flow rule, but this one
>>>
>>> Hold on.. Did you just say "flow rules"? But this API is *not* about
>>> flow rules. I suggest you please re-read description of the API.
>>> It reads: "Negotiate the NIC's ability to deliver specific
>>> kinds of metadata to the PMD". Nothing about flows there.
>>>
>>> Why is it decoupled from flow library this way? Because there is
>>> a drastic difference between generating and plumbing metadata
>>> INSIDE the NIC's flow engine, on the one hand, and delivering
>>> these data from the NIC to the host driver, on the other.
>>>
>>> Let me explain. Say, one creates the following flow rules:
>>>
>>> (a) flow create 0 transfer group 0 pattern [A] / end \
>>> actions mark id XXX jump group 1 / end
>>> (b) flow create 0 transfer group 0 pattern [B] / end \
>>> actions mark id YYY jump group 1 / end
>>>
>>> (c) flow create 0 transfer group 1 pattern mark id is XXX / end \
>>> actions represented_port ethdev_id 1 / end
>>> (d) flow create 0 transfer group 1 pattern mark id is YYY / end \
>>> actions drop / end
>>>
>>> In this example, metadata of type "MARK" is used to partition
>>> flow group (table) 1 into multiple lookup sections. So the
>>> mark value is GENERATED by the flow engine and then it is
>>> CONSUMED by this very flow engine. The application may
>>> NOT necessarily want to receive the mark with mbufs...
>>>
>>> And it is only when the application wants these metadata
>>> DELIVERED to it that it has to call the negotiate API.
>>>
>>> The short of it, nothing prevents the driver from accepting
>>> flow create requests that leverage some meta items/actions.
>>> Drivers do not need the negotiate API to *configure flow*.
>>> They only need this API in order to let the application
>>> choose whether metadata will be DELIVERED (!) or not.
>>>
>>>> requires an API to enable it, so I believe it is inconsistent in that way.
>>>
>>> Please see above. Everything is consistent as *flow library*
>>> and *negotiate metadata delivery* API are totally decoupled.
>>>
>>>>
>>>> From application perspective, assume that it doesn't know NIC details,
>>>> should it call this API or not? Without API call should application
>>>> assume Rx metadata flow rules are enabled or disabled?
>>>
>>> Frankly, applications like testpmd need never call this API.
>>> Simply because seeing, for example, MARK values in mbufs is
>>> useless to it. This API is needed by other applications.
>>> For example, OvS has partial MARK+RSS offload. It adds
>>> flows that distribute packets across multiple queues
>>> and set some MARK values for them. OvS is interested
>>> in getting this values with mbufs since they affect
>>> further lookups in software... So, OvS, knowing it
>>> wants these metadata DELIVERED (!), should invoke
>>> this metadata negotiate API to ensure that.
>>>
>>>>
>>>>
>>>> As I understand intention is to get hint from application if it will
>>>> require Rx metadata flow rules so that PMD can optimize better, but if
>>>
>>> No, nothing about flow rules. Just delivery of metadata with mbufs.
>>>
>>>> PMD doesn't enable Rx metadata flow rules when this API call is missing
>>>
>>> Again, PMD shall not make decisions on whether to enable or disable
>>> support for some FLOW primitives based on interactions via this API.
>>> This API exists solely to let PMD configure delivery of metadata,
>>> i.e. not the way it is generated in the first instance.
>>>
>>>> than it becomes a mandatory API to configure the device. But I think it
>>>> should be optional for optimization.
>>>>
>>>> Also if application not sure to use this flow rule or not, it will by
>>>> default enable all in any case, this will reduce the benefit. As done in
>>>> testpmd.
>>>
>>> Please see above. PMD does not need this API, I take it.
>>>
>>>>
>>>>
>>>> API works in both ways, it request to enable some Rx metadata flow rule,
>>>> but based on what PMD returns application can know what device supports,
>>>> this also inconsistent with how other flow rules work, we don't have API
>>>> to get capability but detect them via flow create/validate.
>>>> Can there be a case API returns a flow rule negotiated, but it fails
>>>> when tried to create the flow rule, isn't this confusing for application
>>>>
>>>>
>>>> I think if we continue this approach there can be multiple enable and
>>>> capability learning APIs for various flow rules or flow rule groups, and
>>>> this can make flow API much more harder to use for applications.
>>>
>>> See my explanations above. This API is not about flows. Period.
>>>
>>>>
>>>>
>>>>
>>>>>>
>>>>>>> Just to avoid back and forth. We will call off this patch and remove
>>>>>>> rte_eth_rx_metadata_negotiate()
>>>>>>> PMD callback from cnxk driver. Keep it as old behavior, so we don't
>>>>>>> need to care
>>>>>>> about rte_eth_rx_metadata_negotiate().
>>>>>>>
>>>>>>
>>>>>> When you remove 'rx_metadata_negotiate' callback, what will be the PMD
>>>>>> behavior? I assume PMD will do the required preparations as if all Rx
>>>>>> metadata is enabled.
>>>>>> And what is the performance impact, is removing callback improve the
>>>>>> performance?
>>>>>>
>>>>>>
>>>>>>> [1]
>>>>>>> The only reason is, typically testpmd will be used performance
>>>>>>> benchmarking as an industry standard. It is difficult to tell/educate
>>>>>>> the QA or customers
>>>>>>> that, "BTW if you need to get better performance add more flag to
>>>>>>> testpmd command line".
>>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>>> parameter to get better number.
>>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>> To make that worst, only some PMD needs to give the additional
>>>>>>>>>>>>>> parameter to get better number.
>>>>>>>>>>>>>> And also, testpmd usage will be treated as application modeling.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the
>>>>>>>>>>>>>> situation with sfc driver?
>>>>>>>>>>>>>> Keeping it as negotiated and not use the feature, will impact
>>>>>>>>>>>>>> the per
>>>>>>>>>>>>>> core performance of sfc or
>>>>>>>>>>>>>> is it just PCI bandwidth thing which really dont show any
>>>>>>>>>>>>>> difference in testpmd?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So,
>>>>>>>>>>>>> it is better to negotiate nothing by default. But it is always
>>>>>>>>>>>>> painful to change defaults. You need to explain that now you
>>>>>>>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel
>>>>>>>>>>>>> offloads.
>>>>>>>>>>>>> Yes, it will be required on sfc and cnxk only.
>>>>>>>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults.
>>>>>>>>>>>>
>>>>>>>>>>>> If we change testpmd defaults to "do nothing",
>>>>>>>>>>>> then we should disable MBUF_FAST_FREE as well.
>>>>>>>>>>>
>>>>>>>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually,
>>>>>>>>>>> !MBUF_FAST_FREE is doing more work.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>
>
>
>
>
>
02/02/2023 12:55, Ivan Malov:
> On Thu, 2 Feb 2023, Thomas Monjalon wrote:
>
> > OK we are progressing on this topic :)
>
> Indeed we are.
>
> >
> > 02/02/2023 11:48, Ivan Malov:
> >> I apologise, there was a typo in the previous mail: "PMD does
> >> not need this API". Should read as "TESTPMD does not need it".
> >
> > testpmd needs all ethdev API,
> > because its purpose is to test the whole all ethdev API.
>
> Touché.
>
> >
> > Maybe the use of this function is misplaced in testpmd.
> > It should be a specific command.
>
> So.. indeed. A specific command which, as I said, is
> invoked explicitly by the person operating testpmd.
>
> > By the way, what is the driver default if negotiation is not done?
>
> The answer is in the question. It's the driver's default.
> If the driver believes it shall NOT deliver metadata for
> the sake of improved performance, default = all disabled.
> If this delivery is a don't care to performance, then
> the driver might want to enable everything by default.
>
> This decision is made by a PMD maintainer, I take it.
I think it is better to have the same default for all drivers.
What others think?
On 2/2/23 15:03, Thomas Monjalon wrote:
> 02/02/2023 12:55, Ivan Malov:
>> On Thu, 2 Feb 2023, Thomas Monjalon wrote:
>>
>>> OK we are progressing on this topic :)
>>
>> Indeed we are.
>>
>>>
>>> 02/02/2023 11:48, Ivan Malov:
>>>> I apologise, there was a typo in the previous mail: "PMD does
>>>> not need this API". Should read as "TESTPMD does not need it".
>>>
>>> testpmd needs all ethdev API,
>>> because its purpose is to test the whole all ethdev API.
>>
>> Touché.
>>
>>>
>>> Maybe the use of this function is misplaced in testpmd.
>>> It should be a specific command.
>>
>> So.. indeed. A specific command which, as I said, is
>> invoked explicitly by the person operating testpmd.
>>
>>> By the way, what is the driver default if negotiation is not done?
>>
>> The answer is in the question. It's the driver's default.
>> If the driver believes it shall NOT deliver metadata for
>> the sake of improved performance, default = all disabled.
>> If this delivery is a don't care to performance, then
>> the driver might want to enable everything by default.
>>
>> This decision is made by a PMD maintainer, I take it.
>
> I think it is better to have the same default for all drivers.
> What others think?
Of course, it would be better since it would simplify
porting apps from one NIC to another.
The problem is that some NICs do not require negotiation
since corresponding data is always available.
If so, the same default should be to provide all Rx metadata
by default. However, it would decrease performance for NIC
NICs with default settings.
(Similar situation is with Rx checksum offload. Some NICs
always calculate Rx checksum and always provide results,
however, it is not a good reason to require it for all NICs.)
@@ -610,6 +610,9 @@ static void cmd_help_long_parsed(void *parsed_result,
"set port (port_id) fec_mode auto|off|rs|baser\n"
" set fec mode for a specific port\n\n"
+ "enable port <port_id> nic_to_pmd_rx_metadata"
+ " Allow nic to pmd Rx metadata negotiation\n\n"
+
, list_pkt_forwarding_modes()
);
}
@@ -12621,6 +12624,60 @@ static cmdline_parse_inst_t cmd_show_port_flow_transfer_proxy = {
}
};
+/* Allow negotiating Rx metadata between NIC and PMD */
+struct cmd_config_port_rx_metadata {
+ cmdline_fixed_string_t enable;
+ cmdline_fixed_string_t port;
+ uint16_t port_id;
+ cmdline_fixed_string_t nic_to_pmd_rx_metadata;
+};
+
+static void
+cmd_config_port_rx_metadata_parsed(void *parsed_result,
+ __rte_unused struct cmdline *cl,
+ __rte_unused void *data)
+{
+ struct cmd_config_port_rx_metadata *res = parsed_result;
+
+ if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+ return;
+ if (!port_is_stopped(res->port_id)) {
+ fprintf(stderr, "Please stop port %u first\n", res->port_id);
+ return;
+ }
+
+ ports[res->port_id].nic_to_pmd_rx_metadata = 1;
+
+ reset_port(res->port_id);
+}
+
+
+static cmdline_parse_token_string_t cmd_config_port_rx_metadata_enable =
+ TOKEN_STRING_INITIALIZER(struct cmd_config_port_rx_metadata, enable,
+ "enable");
+static cmdline_parse_token_string_t cmd_config_port_rx_metadata_port =
+ TOKEN_STRING_INITIALIZER(struct cmd_config_port_rx_metadata, port,
+ "port");
+static cmdline_parse_token_num_t cmd_config_port_rx_metadata_id =
+ TOKEN_NUM_INITIALIZER(struct cmd_config_port_rx_metadata, port_id,
+ RTE_UINT16);
+static cmdline_parse_token_string_t cmd_config_port_rx_metadata_nic_to_pmd_rx_metadata =
+ TOKEN_STRING_INITIALIZER(struct cmd_config_port_rx_metadata, nic_to_pmd_rx_metadata,
+ "nic_to_pmd_rx_metadata");
+
+static cmdline_parse_inst_t cmd_config_port_rx_metadata_parse = {
+ .f = cmd_config_port_rx_metadata_parsed,
+ .data = NULL,
+ .help_str = "enable port <port_id> nic_to_pmd_rx_metadata",
+ .tokens = {
+ (void *)&cmd_config_port_rx_metadata_enable,
+ (void *)&cmd_config_port_rx_metadata_port,
+ (void *)&cmd_config_port_rx_metadata_id,
+ (void *)&cmd_config_port_rx_metadata_nic_to_pmd_rx_metadata,
+ NULL,
+ },
+};
+
/* ******************************************************************************** */
/* list of instructions */
@@ -12851,6 +12908,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
(cmdline_parse_inst_t *)&cmd_show_capability,
(cmdline_parse_inst_t *)&cmd_set_flex_is_pattern,
(cmdline_parse_inst_t *)&cmd_set_flex_spec_pattern,
+ (cmdline_parse_inst_t *)&cmd_config_port_rx_metadata_parse,
NULL,
};
@@ -3249,6 +3249,15 @@ port_flow_create(portid_t port_id,
}
id = port->flow_list->id + 1;
}
+
+ if (port->nic_to_pmd_rx_metadata == 0 &&
+ (actions->type == RTE_FLOW_ACTION_TYPE_MARK ||
+ actions->type == RTE_FLOW_ACTION_TYPE_FLAG)) {
+ fprintf(stderr,
+ "rx metadata is not negotiated with PMD\n");
+ return -EINVAL;
+ }
+
if (tunnel_ops->enabled) {
pft = port_flow_tunnel_offload_cmd_prep(port_id, pattern,
actions, tunnel_ops);
@@ -1604,8 +1604,6 @@ init_config_port_offloads(portid_t pid, uint32_t socket_id)
int ret;
int i;
- eth_rx_metadata_negotiate_mp(pid);
-
port->dev_conf.txmode = tx_mode;
port->dev_conf.rxmode = rx_mode;
@@ -2946,6 +2944,9 @@ start_port(portid_t pid)
port->update_conf = 0;
}
+ if (port->nic_to_pmd_rx_metadata)
+ eth_rx_metadata_negotiate_mp(pi);
+
/* configure port */
diag = eth_dev_configure_mp(pi, nb_rxq + nb_hairpinq,
nb_txq + nb_hairpinq,
@@ -309,6 +309,7 @@ struct rte_port {
uint8_t need_setup; /**< port just attached */
uint8_t need_reconfig; /**< need reconfiguring port or not */
uint8_t need_reconfig_queues; /**< need reconfiguring queues or not */
+ uint8_t nic_to_pmd_rx_metadata; /**< send rx metadata to PMD. */
uint8_t rss_flag; /**< enable rss or not */
uint8_t dcb_flag; /**< enable dcb */
uint16_t nb_rx_desc[RTE_MAX_QUEUES_PER_PORT+1]; /**< per queue rx desc number */
@@ -1558,6 +1558,12 @@ Enable or disable a per port Rx offloading on all Rx queues of a port::
This command should be run when the port is stopped, or else it will fail.
+Enable Rx metadata negotiation
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Enable NIC to PMD Rx metadata negotiation::
+ testpmd> enable port <port_id> nic_to_pmd_rx_metadata
+
config per queue Rx offloading
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~