diff mbox series

[v2] app/testpmd: fix set Rx offloads RSS hash off

Message ID 20211104092947.244962-1-jie1x.wang@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] app/testpmd: fix set Rx offloads RSS hash off |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues

Commit Message

Jie Wang Nov. 4, 2021, 9:29 a.m. UTC 
  When we set Rx offloads RSS hash off in cmdline, then we start
port, if dev->data->dev_conf.rxmode.mq_mode RSS enable(multiple
queues mode), it will load RSS hash in driver and sync to port.

After setting Rx offloads RSS hash off, this patch removes
RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode to fix this issue.

And after setting Rx offloads RSS hash on, this patch also adds
RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode.

Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
 app/test-pmd/cmdline.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Ferruh Yigit Nov. 5, 2021, 4:24 p.m. UTC | #1 
On 11/4/2021 9:29 AM, Jie Wang wrote:
> When we set Rx offloads RSS hash off in cmdline, then we start
> port, if dev->data->dev_conf.rxmode.mq_mode RSS enable(multiple
> queues mode), it will load RSS hash in driver and sync to port.
> 
> After setting Rx offloads RSS hash off, this patch removes
> RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode to fix this issue.
> 
> And after setting Rx offloads RSS hash on, this patch also adds
> RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode.
> 
> Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")
> 
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
>   app/test-pmd/cmdline.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index d08a1c9777..f8277e4443 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -16176,10 +16176,16 @@ cmd_config_per_port_rx_offload_parsed(void *parsed_result,
>   		port->dev_conf.rxmode.offloads |= single_offload;
>   		for (q = 0; q < nb_rx_queues; q++)
>   			port->rx_conf[q].offloads |= single_offload;
> +
> +		if (single_offload & 0x80000)

I don't why you used immediate value, but you mean 'RTE_ETH_RX_OFFLOAD_RSS_HASH' here.

> +			port->dev_conf.rxmode.mq_mode |= RTE_ETH_MQ_RX_RSS;

Why enabling RSS when 'RSS HASH' offload is enabled? In this case if RSS is disabled
I think it should fail.

Can you please describe again what problem you are solving?

>   	} else {
>   		port->dev_conf.rxmode.offloads &= ~single_offload;
>   		for (q = 0; q < nb_rx_queues; q++)
>   			port->rx_conf[q].offloads &= ~single_offload;
> +
> +		if (single_offload & 0x80000)
> +			port->dev_conf.rxmode.mq_mode &= ~RTE_ETH_MQ_RX_RSS;
>   	}
>   
>   	cmd_reconfig_device_queue(port_id, 1, 1);
>
Jie Wang Nov. 8, 2021, 7:24 a.m. UTC | #2 
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Saturday, November 6, 2021 12:24 AM
> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> Cc: Zhang, Yuying <yuying.zhang@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Yang, SteveX <stevex.yang@intel.com>; Guo, Junfeng
> <junfeng.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix set Rx offloads RSS hash off
> 
> On 11/4/2021 9:29 AM, Jie Wang wrote:
> > When we set Rx offloads RSS hash off in cmdline, then we start port,
> > if dev->data->dev_conf.rxmode.mq_mode RSS enable(multiple queues
> > mode), it will load RSS hash in driver and sync to port.
> >
> > After setting Rx offloads RSS hash off, this patch removes
> > RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode to fix this issue.
> >
> > And after setting Rx offloads RSS hash on, this patch also adds
> > RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode.
> >
> > Fixes: c73a9071877a ("app/testpmd: add commands to test new offload
> > API")
> >
> > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> > ---
> >   app/test-pmd/cmdline.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > d08a1c9777..f8277e4443 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -16176,10 +16176,16 @@ cmd_config_per_port_rx_offload_parsed(void
> *parsed_result,
> >   		port->dev_conf.rxmode.offloads |= single_offload;
> >   		for (q = 0; q < nb_rx_queues; q++)
> >   			port->rx_conf[q].offloads |= single_offload;
> > +
> > +		if (single_offload & 0x80000)
> 
> I don't why you used immediate value, but you mean
> 'RTE_ETH_RX_OFFLOAD_RSS_HASH' here.
> 

I will update it in next version.

> > +			port->dev_conf.rxmode.mq_mode |=
> RTE_ETH_MQ_RX_RSS;
> 
> Why enabling RSS when 'RSS HASH' offload is enabled? In this case if RSS is
> disabled I think it should fail.
> 
> Can you please describe again what problem you are solving?
> 

If 'RSS HASH' offload is disabled in the first, the mq_mode RSS is disabled.
Then 'RSS HASH' offload is enabled in the second, so that we should enable RSS here.

> >   	} else {
> >   		port->dev_conf.rxmode.offloads &= ~single_offload;
> >   		for (q = 0; q < nb_rx_queues; q++)
> >   			port->rx_conf[q].offloads &= ~single_offload;
> > +
> > +		if (single_offload & 0x80000)
> > +			port->dev_conf.rxmode.mq_mode &=
> ~RTE_ETH_MQ_RX_RSS;

When 'RSS HASH' offload is disabled, that mq_mode RSS should be disabled too. So that it will not enable 'RSS HASH' offload in driver.

> >   	}
> >
> >   	cmd_reconfig_device_queue(port_id, 1, 1);
> >
Ferruh Yigit Nov. 8, 2021, 9:02 a.m. UTC | #3 
On 11/8/2021 7:24 AM, Wang, Jie1X wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Saturday, November 6, 2021 12:24 AM
>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
>> Cc: Zhang, Yuying <yuying.zhang@intel.com>; Li, Xiaoyun
>> <xiaoyun.li@intel.com>; Yang, SteveX <stevex.yang@intel.com>; Guo, Junfeng
>> <junfeng.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix set Rx offloads RSS hash off
>>
>> On 11/4/2021 9:29 AM, Jie Wang wrote:
>>> When we set Rx offloads RSS hash off in cmdline, then we start port,
>>> if dev->data->dev_conf.rxmode.mq_mode RSS enable(multiple queues
>>> mode), it will load RSS hash in driver and sync to port.
>>>
>>> After setting Rx offloads RSS hash off, this patch removes
>>> RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode to fix this issue.
>>>
>>> And after setting Rx offloads RSS hash on, this patch also adds
>>> RTE_ETH_MQ_RX_RSS in port->dev_conf.rxmode.mq_mode.
>>>
>>> Fixes: c73a9071877a ("app/testpmd: add commands to test new offload
>>> API")
>>>
>>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>>> ---
>>>    app/test-pmd/cmdline.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>> d08a1c9777..f8277e4443 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -16176,10 +16176,16 @@ cmd_config_per_port_rx_offload_parsed(void
>> *parsed_result,
>>>    		port->dev_conf.rxmode.offloads |= single_offload;
>>>    		for (q = 0; q < nb_rx_queues; q++)
>>>    			port->rx_conf[q].offloads |= single_offload;
>>> +
>>> +		if (single_offload & 0x80000)
>>
>> I don't why you used immediate value, but you mean
>> 'RTE_ETH_RX_OFFLOAD_RSS_HASH' here.
>>
> 
> I will update it in next version.
> 
>>> +			port->dev_conf.rxmode.mq_mode |=
>> RTE_ETH_MQ_RX_RSS;
>>
>> Why enabling RSS when 'RSS HASH' offload is enabled? In this case if RSS is
>> disabled I think it should fail.
>>
>> Can you please describe again what problem you are solving?
>>
> 
> If 'RSS HASH' offload is disabled in the first, the mq_mode RSS is disabled.
> Then 'RSS HASH' offload is enabled in the second, so that we should enable RSS here.
> 
>>>    	} else {
>>>    		port->dev_conf.rxmode.offloads &= ~single_offload;
>>>    		for (q = 0; q < nb_rx_queues; q++)
>>>    			port->rx_conf[q].offloads &= ~single_offload;
>>> +
>>> +		if (single_offload & 0x80000)
>>> +			port->dev_conf.rxmode.mq_mode &=
>> ~RTE_ETH_MQ_RX_RSS;
> 
> When 'RSS HASH' offload is disabled, that mq_mode RSS should be disabled too. So that it will not enable 'RSS HASH' offload in driver.
> 

I am not clear with this statement.

"RSS HASH" can be disabled without disabling 'RSS', indeed that is the
reason to have this offload.
And most of the Intel NICs don't support disabling "RSS HASH" anyway.

Again, can you please clarify the real problem you are solving?
diff mbox series

Patch

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index d08a1c9777..f8277e4443 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -16176,10 +16176,16 @@  cmd_config_per_port_rx_offload_parsed(void *parsed_result,
 		port->dev_conf.rxmode.offloads |= single_offload;
 		for (q = 0; q < nb_rx_queues; q++)
 			port->rx_conf[q].offloads |= single_offload;
+
+		if (single_offload & 0x80000)
+			port->dev_conf.rxmode.mq_mode |= RTE_ETH_MQ_RX_RSS;
 	} else {
 		port->dev_conf.rxmode.offloads &= ~single_offload;
 		for (q = 0; q < nb_rx_queues; q++)
 			port->rx_conf[q].offloads &= ~single_offload;
+
+		if (single_offload & 0x80000)
+			port->dev_conf.rxmode.mq_mode &= ~RTE_ETH_MQ_RX_RSS;
 	}
 
 	cmd_reconfig_device_queue(port_id, 1, 1);