DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
@ 2021-04-01  8:28 dapengx.yu
  2021-04-07  2:30 ` Li, Xiaoyun
  2021-04-08 15:41 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
  0 siblings, 2 replies; 13+ messages in thread
From: dapengx.yu @ 2021-04-01  8:28 UTC (permalink / raw)
  To: xiaoyun.li, qi.z.zhang; +Cc: dev, Dapeng Yu, stable

From: Dapeng Yu <dapengx.yu@intel.com>

Configure per queue rx offloading and per queue tx offloading command
shouldn't trigger the rte_eth_dev_configure() to reconfigure device.

The patch sets the queue reconfiguration flag only, and does not set the
device reconfiguration flag. Therefore after port is restarted,
rte_eth_dev_configure() will not be called again.

Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")
Cc: stable@dpdk.org

Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
---
 app/test-pmd/cmdline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 14110eb2e..b49e9f52b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -15626,7 +15626,7 @@ cmd_config_per_queue_rx_offload_parsed(void *parsed_result,
 	else
 		port->rx_conf[queue_id].offloads &= ~single_offload;
 
-	cmd_reconfig_device_queue(port_id, 1, 1);
+	cmd_reconfig_device_queue(port_id, 0, 1);
 }
 
 cmdline_parse_inst_t cmd_config_per_queue_rx_offload = {
@@ -16044,7 +16044,7 @@ cmd_config_per_queue_tx_offload_parsed(void *parsed_result,
 	else
 		port->tx_conf[queue_id].offloads &= ~single_offload;
 
-	cmd_reconfig_device_queue(port_id, 1, 1);
+	cmd_reconfig_device_queue(port_id, 0, 1);
 }
 
 cmdline_parse_inst_t cmd_config_per_queue_tx_offload = {
-- 
2.27.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-01  8:28 [dpdk-dev] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd dapengx.yu
@ 2021-04-07  2:30 ` Li, Xiaoyun
  2021-04-08 15:41 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
  1 sibling, 0 replies; 13+ messages in thread
From: Li, Xiaoyun @ 2021-04-07  2:30 UTC (permalink / raw)
  To: Yu, DapengX, Zhang, Qi Z; +Cc: dev, stable

Hi

> -----Original Message-----
> From: Yu, DapengX <dapengx.yu@intel.com>
> Sent: Thursday, April 1, 2021 16:29
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> Subject: [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
> 
> From: Dapeng Yu <dapengx.yu@intel.com>
> 
> Configure per queue rx offloading and per queue tx offloading command
> shouldn't trigger the rte_eth_dev_configure() to reconfigure device.
> 
> The patch sets the queue reconfiguration flag only, and does not set the device
> reconfiguration flag. Therefore after port is restarted,
> rte_eth_dev_configure() will not be called again.
> 
> Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> ---
>  app/test-pmd/cmdline.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 14110eb2e..b49e9f52b 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15626,7 +15626,7 @@ cmd_config_per_queue_rx_offload_parsed(void
> *parsed_result,
>  	else
>  		port->rx_conf[queue_id].offloads &= ~single_offload;
> 
> -	cmd_reconfig_device_queue(port_id, 1, 1);
> +	cmd_reconfig_device_queue(port_id, 0, 1);
>  }
> 
>  cmdline_parse_inst_t cmd_config_per_queue_rx_offload = { @@ -16044,7
> +16044,7 @@ cmd_config_per_queue_tx_offload_parsed(void *parsed_result,
>  	else
>  		port->tx_conf[queue_id].offloads &= ~single_offload;
> 
> -	cmd_reconfig_device_queue(port_id, 1, 1);
> +	cmd_reconfig_device_queue(port_id, 0, 1);
>  }
> 
>  cmdline_parse_inst_t cmd_config_per_queue_tx_offload = {
> --
> 2.27.0

Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-01  8:28 [dpdk-dev] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd dapengx.yu
  2021-04-07  2:30 ` Li, Xiaoyun
@ 2021-04-08 15:41 ` Ferruh Yigit
  2021-04-09  5:25   ` Yu, DapengX
  1 sibling, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2021-04-08 15:41 UTC (permalink / raw)
  To: dapengx.yu, xiaoyun.li, qi.z.zhang; +Cc: dev, stable

On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> From: Dapeng Yu <dapengx.yu@intel.com>
> 
> Configure per queue rx offloading and per queue tx offloading command
> shouldn't trigger the rte_eth_dev_configure() to reconfigure device.
> 
> The patch sets the queue reconfiguration flag only, and does not set the
> device reconfiguration flag. Therefore after port is restarted,
> rte_eth_dev_configure() will not be called again.
> 

Just to clarify the impact, was calling 'rte_eth_dev_configure()' causing any 
problem, is this fixing any issue?
Or is this patch an optimization to eliminate an unnecessary call?

> Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> ---
>   app/test-pmd/cmdline.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 14110eb2e..b49e9f52b 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15626,7 +15626,7 @@ cmd_config_per_queue_rx_offload_parsed(void *parsed_result,
>   	else
>   		port->rx_conf[queue_id].offloads &= ~single_offload;
>   
> -	cmd_reconfig_device_queue(port_id, 1, 1);
> +	cmd_reconfig_device_queue(port_id, 0, 1);
>   }
>   
>   cmdline_parse_inst_t cmd_config_per_queue_rx_offload = {
> @@ -16044,7 +16044,7 @@ cmd_config_per_queue_tx_offload_parsed(void *parsed_result,
>   	else
>   		port->tx_conf[queue_id].offloads &= ~single_offload;
>   
> -	cmd_reconfig_device_queue(port_id, 1, 1);
> +	cmd_reconfig_device_queue(port_id, 0, 1);
>   }
>   
>   cmdline_parse_inst_t cmd_config_per_queue_tx_offload = {
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-08 15:41 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
@ 2021-04-09  5:25   ` Yu, DapengX
  2021-04-09  7:50     ` Li, Xiaoyun
  0 siblings, 1 reply; 13+ messages in thread
From: Yu, DapengX @ 2021-04-09  5:25 UTC (permalink / raw)
  To: Yigit, Ferruh, Li, Xiaoyun, Zhang, Qi Z; +Cc: dev, stable



> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday, April 8, 2021 11:42 PM
> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
> reconfig cmd
> 
> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> > From: Dapeng Yu <dapengx.yu@intel.com>
> >
> > Configure per queue rx offloading and per queue tx offloading command
> > shouldn't trigger the rte_eth_dev_configure() to reconfigure device.
> >
> > The patch sets the queue reconfiguration flag only, and does not set
> > the device reconfiguration flag. Therefore after port is restarted,
> > rte_eth_dev_configure() will not be called again.
> >
> 
> Just to clarify the impact, was calling 'rte_eth_dev_configure()' causing any
> problem, is this fixing any issue?
> Or is this patch an optimization to eliminate an unnecessary call?
> 
This patch does fix an issue, and it also eliminates an unnecessary call.

The issue is: 
per-queue configuration, for example: port 0 rxq 0 rx_offload jumbo_frame off
triggers the per-device configuration change: the RSS key is reconfigured and changes
after rte_eth_dev_configure() is called on ICE PMD driver, that cause a test case failure.

There is an unnecessary call in original implementation because both 
cmd_config_per_queue_rx_offload_parsed() and cmd_config_per_queue_tx_offload_parsed() 
does not update the "port->dev_conf" which hold the port configuration, therefore there is no
need to call rte_eth_dev_configure().


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-09  5:25   ` Yu, DapengX
@ 2021-04-09  7:50     ` Li, Xiaoyun
  2021-04-09 10:29       ` Yu, DapengX
  0 siblings, 1 reply; 13+ messages in thread
From: Li, Xiaoyun @ 2021-04-09  7:50 UTC (permalink / raw)
  To: Yu, DapengX, Yigit, Ferruh, Zhang, Qi Z; +Cc: dev, stable



> -----Original Message-----
> From: Yu, DapengX <dapengx.yu@intel.com>
> Sent: Friday, April 9, 2021 13:25
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
> reconfig cmd
> 
> 
> 
> > -----Original Message-----
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Sent: Thursday, April 8, 2021 11:42 PM
> > To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> > <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> > offload reconfig cmd
> >
> > On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> > > From: Dapeng Yu <dapengx.yu@intel.com>
> > >
> > > Configure per queue rx offloading and per queue tx offloading
> > > command shouldn't trigger the rte_eth_dev_configure() to reconfigure
> device.
> > >
> > > The patch sets the queue reconfiguration flag only, and does not set
> > > the device reconfiguration flag. Therefore after port is restarted,
> > > rte_eth_dev_configure() will not be called again.
> > >
> >
> > Just to clarify the impact, was calling 'rte_eth_dev_configure()'
> > causing any problem, is this fixing any issue?
> > Or is this patch an optimization to eliminate an unnecessary call?
> >
> This patch does fix an issue, and it also eliminates an unnecessary call.
> 
> The issue is:
> per-queue configuration, for example: port 0 rxq 0 rx_offload jumbo_frame off
> triggers the per-device configuration change: the RSS key is reconfigured and
> changes after rte_eth_dev_configure() is called on ICE PMD driver, that cause a
> test case failure.
> 

Hmmm. I agree on the following. It doesn't need dev_configure. That's why I give you ack.

But your issue. Shouldn't you fix the driver? The vsi->rss_key[] just updated itself as a random value when dev_conf doesn't contain one.
But your case is dev_conf doesn't contain new rss key, but vsi->rss_key is not null. In this case, it should keep the same not get a new random key.

> There is an unnecessary call in original implementation because both
> cmd_config_per_queue_rx_offload_parsed() and
> cmd_config_per_queue_tx_offload_parsed()
> does not update the "port->dev_conf" which hold the port configuration,
> therefore there is no need to call rte_eth_dev_configure().


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-09  7:50     ` Li, Xiaoyun
@ 2021-04-09 10:29       ` Yu, DapengX
  2021-04-12  2:21         ` Li, Xiaoyun
  0 siblings, 1 reply; 13+ messages in thread
From: Yu, DapengX @ 2021-04-09 10:29 UTC (permalink / raw)
  To: Li, Xiaoyun, Yigit, Ferruh, Zhang, Qi Z; +Cc: dev, stable



> -----Original Message-----
> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> Sent: Friday, April 9, 2021 3:50 PM
> To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
> reconfig cmd
> 
> 
> 
> > -----Original Message-----
> > From: Yu, DapengX <dapengx.yu@intel.com>
> > Sent: Friday, April 9, 2021 13:25
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> > <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> > offload reconfig cmd
> >
> >
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > > Sent: Thursday, April 8, 2021 11:42 PM
> > > To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> > > <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; stable@dpdk.org
> > > Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> > > offload reconfig cmd
> > >
> > > On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> > > > From: Dapeng Yu <dapengx.yu@intel.com>
> > > >
> > > > Configure per queue rx offloading and per queue tx offloading
> > > > command shouldn't trigger the rte_eth_dev_configure() to
> > > > reconfigure
> > device.
> > > >
> > > > The patch sets the queue reconfiguration flag only, and does not
> > > > set the device reconfiguration flag. Therefore after port is
> > > > restarted,
> > > > rte_eth_dev_configure() will not be called again.
> > > >
> > >
> > > Just to clarify the impact, was calling 'rte_eth_dev_configure()'
> > > causing any problem, is this fixing any issue?
> > > Or is this patch an optimization to eliminate an unnecessary call?
> > >
> > This patch does fix an issue, and it also eliminates an unnecessary call.
> >
> > The issue is:
> > per-queue configuration, for example: port 0 rxq 0 rx_offload
> > jumbo_frame off triggers the per-device configuration change: the RSS
> > key is reconfigured and changes after rte_eth_dev_configure() is
> > called on ICE PMD driver, that cause a test case failure.
> >
> 
> Hmmm. I agree on the following. It doesn't need dev_configure. That's why I
> give you ack.
> 
> But your issue. Shouldn't you fix the driver? The vsi->rss_key[] just updated
> itself as a random value when dev_conf doesn't contain one.
> But your case is dev_conf doesn't contain new rss key, but vsi->rss_key is not
> null. In this case, it should keep the same not get a new random key.
> 
In current implementation, dev->data->dev_conf.rx_adv_conf.rss_conf in PMD
does not hold the rss_key value if user does not set it in the input parameter of rte_eth_dev_configure(). 

Even if PMD generate one automatically, it will not be saved in dev->data->dev_conf.rx_adv_conf.rss_conf. 

When user want to get rss_key, it will be retrieved on the fly from hardware, but not from any variable
in PMD.

So PMD (ice and i40e) think only rss_key  which is set by user via rte_eth_dev_configure() can be reused
when port is reconfigured.

If it is not present, PMD will generate one by itself anyway even if it is present in vsi->rss_key.

I don’t think this behavior is wrong, so did not fix ice PMD.

> > There is an unnecessary call in original implementation because both
> > cmd_config_per_queue_rx_offload_parsed() and
> > cmd_config_per_queue_tx_offload_parsed()
> > does not update the "port->dev_conf" which hold the port
> > configuration, therefore there is no need to call rte_eth_dev_configure().


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-09 10:29       ` Yu, DapengX
@ 2021-04-12  2:21         ` Li, Xiaoyun
  2021-04-12 12:46           ` Ferruh Yigit
  0 siblings, 1 reply; 13+ messages in thread
From: Li, Xiaoyun @ 2021-04-12  2:21 UTC (permalink / raw)
  To: Yu, DapengX, Yigit, Ferruh, Zhang, Qi Z; +Cc: dev, stable



> -----Original Message-----
> From: Yu, DapengX <dapengx.yu@intel.com>
> Sent: Friday, April 9, 2021 18:29
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
> reconfig cmd
> 
> 
> 
> > -----Original Message-----
> > From: Li, Xiaoyun <xiaoyun.li@intel.com>
> > Sent: Friday, April 9, 2021 3:50 PM
> > To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> > offload reconfig cmd
> >
> >
> >
> > > -----Original Message-----
> > > From: Yu, DapengX <dapengx.yu@intel.com>
> > > Sent: Friday, April 9, 2021 13:25
> > > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> > > <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; stable@dpdk.org
> > > Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> > > offload reconfig cmd
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > > > Sent: Thursday, April 8, 2021 11:42 PM
> > > > To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> > > > <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > Cc: dev@dpdk.org; stable@dpdk.org
> > > > Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
> > > > Tx offload reconfig cmd
> > > >
> > > > On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> > > > > From: Dapeng Yu <dapengx.yu@intel.com>
> > > > >
> > > > > Configure per queue rx offloading and per queue tx offloading
> > > > > command shouldn't trigger the rte_eth_dev_configure() to
> > > > > reconfigure
> > > device.
> > > > >
> > > > > The patch sets the queue reconfiguration flag only, and does not
> > > > > set the device reconfiguration flag. Therefore after port is
> > > > > restarted,
> > > > > rte_eth_dev_configure() will not be called again.
> > > > >
> > > >
> > > > Just to clarify the impact, was calling 'rte_eth_dev_configure()'
> > > > causing any problem, is this fixing any issue?
> > > > Or is this patch an optimization to eliminate an unnecessary call?
> > > >
> > > This patch does fix an issue, and it also eliminates an unnecessary call.
> > >
> > > The issue is:
> > > per-queue configuration, for example: port 0 rxq 0 rx_offload
> > > jumbo_frame off triggers the per-device configuration change: the
> > > RSS key is reconfigured and changes after rte_eth_dev_configure() is
> > > called on ICE PMD driver, that cause a test case failure.
> > >
> >
> > Hmmm. I agree on the following. It doesn't need dev_configure. That's
> > why I give you ack.
> >
> > But your issue. Shouldn't you fix the driver? The vsi->rss_key[] just
> > updated itself as a random value when dev_conf doesn't contain one.
> > But your case is dev_conf doesn't contain new rss key, but
> > vsi->rss_key is not null. In this case, it should keep the same not get a new
> random key.
> >
> In current implementation, dev->data->dev_conf.rx_adv_conf.rss_conf in PMD
> does not hold the rss_key value if user does not set it in the input parameter of
> rte_eth_dev_configure().
> 
> Even if PMD generate one automatically, it will not be saved in dev->data-
> >dev_conf.rx_adv_conf.rss_conf.
> 
> When user want to get rss_key, it will be retrieved on the fly from hardware, but
> not from any variable in PMD.
> 
> So PMD (ice and i40e) think only rss_key  which is set by user via
> rte_eth_dev_configure() can be reused when port is reconfigured.
> 
> If it is not present, PMD will generate one by itself anyway even if it is present in
> vsi->rss_key.
> 
> I don’t think this behavior is wrong, so did not fix ice PMD.

If you want to recover the rss key. The driver should store the key in vsi->rss_key also in ice_rss_hash_update(). Then when user not config rss_key in rss_conf and vs->rss_key is not 0, you should set the original value not a random value to hw.

Otherwise, you are assuming the behavior rss to different queues is right since user want random key. It's not an issue.

Any scenario, it's not testpmd issue. Please withdraw your patch.

> 
> > > There is an unnecessary call in original implementation because both
> > > cmd_config_per_queue_rx_offload_parsed() and
> > > cmd_config_per_queue_tx_offload_parsed()
> > > does not update the "port->dev_conf" which hold the port
> > > configuration, therefore there is no need to call rte_eth_dev_configure().


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-12  2:21         ` Li, Xiaoyun
@ 2021-04-12 12:46           ` Ferruh Yigit
  2021-04-15  6:04             ` Yu, DapengX
  0 siblings, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2021-04-12 12:46 UTC (permalink / raw)
  To: Li, Xiaoyun, Yu, DapengX, Zhang, Qi Z; +Cc: dev, stable

On 4/12/2021 3:21 AM, Li, Xiaoyun wrote:
> 
> 
>> -----Original Message-----
>> From: Yu, DapengX <dapengx.yu@intel.com>
>> Sent: Friday, April 9, 2021 18:29
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
>> reconfig cmd
>>
>>
>>
>>> -----Original Message-----
>>> From: Li, Xiaoyun <xiaoyun.li@intel.com>
>>> Sent: Friday, April 9, 2021 3:50 PM
>>> To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>> Cc: dev@dpdk.org; stable@dpdk.org
>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
>>> offload reconfig cmd
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Yu, DapengX <dapengx.yu@intel.com>
>>>> Sent: Friday, April 9, 2021 13:25
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
>>>> offload reconfig cmd
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>> Sent: Thursday, April 8, 2021 11:42 PM
>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
>>>>> Tx offload reconfig cmd
>>>>>
>>>>> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
>>>>>> From: Dapeng Yu <dapengx.yu@intel.com>
>>>>>>
>>>>>> Configure per queue rx offloading and per queue tx offloading
>>>>>> command shouldn't trigger the rte_eth_dev_configure() to
>>>>>> reconfigure
>>>> device.
>>>>>>
>>>>>> The patch sets the queue reconfiguration flag only, and does not
>>>>>> set the device reconfiguration flag. Therefore after port is
>>>>>> restarted,
>>>>>> rte_eth_dev_configure() will not be called again.
>>>>>>
>>>>>
>>>>> Just to clarify the impact, was calling 'rte_eth_dev_configure()'
>>>>> causing any problem, is this fixing any issue?
>>>>> Or is this patch an optimization to eliminate an unnecessary call?
>>>>>
>>>> This patch does fix an issue, and it also eliminates an unnecessary call.
>>>>
>>>> The issue is:
>>>> per-queue configuration, for example: port 0 rxq 0 rx_offload
>>>> jumbo_frame off triggers the per-device configuration change: the
>>>> RSS key is reconfigured and changes after rte_eth_dev_configure() is
>>>> called on ICE PMD driver, that cause a test case failure.
>>>>
>>>
>>> Hmmm. I agree on the following. It doesn't need dev_configure. That's
>>> why I give you ack.
>>>
>>> But your issue. Shouldn't you fix the driver? The vsi->rss_key[] just
>>> updated itself as a random value when dev_conf doesn't contain one.
>>> But your case is dev_conf doesn't contain new rss key, but
>>> vsi->rss_key is not null. In this case, it should keep the same not get a new
>> random key.
>>>
>> In current implementation, dev->data->dev_conf.rx_adv_conf.rss_conf in PMD
>> does not hold the rss_key value if user does not set it in the input parameter of
>> rte_eth_dev_configure().
>>
>> Even if PMD generate one automatically, it will not be saved in dev->data-
>>> dev_conf.rx_adv_conf.rss_conf.
>>
>> When user want to get rss_key, it will be retrieved on the fly from hardware, but
>> not from any variable in PMD.
>>
>> So PMD (ice and i40e) think only rss_key  which is set by user via
>> rte_eth_dev_configure() can be reused when port is reconfigured.
>>
>> If it is not present, PMD will generate one by itself anyway even if it is present in
>> vsi->rss_key.
>>
>> I don’t think this behavior is wrong, so did not fix ice PMD.
> 
> If you want to recover the rss key. The driver should store the key in vsi->rss_key also in ice_rss_hash_update(). Then when user not config rss_key in rss_conf and vs->rss_key is not 0, you should set the original value not a random value to hw.
> 

+1

> Otherwise, you are assuming the behavior rss to different queues is right since user want random key. It's not an issue.
> 
> Any scenario, it's not testpmd issue. Please withdraw your patch.
> 
>>
>>>> There is an unnecessary call in original implementation because both
>>>> cmd_config_per_queue_rx_offload_parsed() and
>>>> cmd_config_per_queue_tx_offload_parsed()
>>>> does not update the "port->dev_conf" which hold the port
>>>> configuration, therefore there is no need to call rte_eth_dev_configure().
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-12 12:46           ` Ferruh Yigit
@ 2021-04-15  6:04             ` Yu, DapengX
  2021-04-19 15:46               ` Ferruh Yigit
  0 siblings, 1 reply; 13+ messages in thread
From: Yu, DapengX @ 2021-04-15  6:04 UTC (permalink / raw)
  To: Yigit, Ferruh, Li, Xiaoyun, Zhang, Qi Z; +Cc: dev, stable



> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, April 12, 2021 8:46 PM
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yu, DapengX
> <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
> reconfig cmd
> 
> On 4/12/2021 3:21 AM, Li, Xiaoyun wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yu, DapengX <dapengx.yu@intel.com>
> >> Sent: Friday, April 9, 2021 18:29
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> >> offload reconfig cmd
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> >>> Sent: Friday, April 9, 2021 3:50 PM
> >>> To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
> >>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>> Cc: dev@dpdk.org; stable@dpdk.org
> >>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> >>> offload reconfig cmd
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Yu, DapengX <dapengx.yu@intel.com>
> >>>> Sent: Friday, April 9, 2021 13:25
> >>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> >>>> offload reconfig cmd
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>> Sent: Thursday, April 8, 2021 11:42 PM
> >>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> >>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
> >>>>> Tx offload reconfig cmd
> >>>>>
> >>>>> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> >>>>>> From: Dapeng Yu <dapengx.yu@intel.com>
> >>>>>>
> >>>>>> Configure per queue rx offloading and per queue tx offloading
> >>>>>> command shouldn't trigger the rte_eth_dev_configure() to
> >>>>>> reconfigure
> >>>> device.
> >>>>>>
> >>>>>> The patch sets the queue reconfiguration flag only, and does not
> >>>>>> set the device reconfiguration flag. Therefore after port is
> >>>>>> restarted,
> >>>>>> rte_eth_dev_configure() will not be called again.
> >>>>>>
> >>>>>
> >>>>> Just to clarify the impact, was calling 'rte_eth_dev_configure()'
> >>>>> causing any problem, is this fixing any issue?
> >>>>> Or is this patch an optimization to eliminate an unnecessary call?
> >>>>>
> >>>> This patch does fix an issue, and it also eliminates an unnecessary call.
> >>>>
> >>>> The issue is:
> >>>> per-queue configuration, for example: port 0 rxq 0 rx_offload
> >>>> jumbo_frame off triggers the per-device configuration change: the
> >>>> RSS key is reconfigured and changes after rte_eth_dev_configure()
> >>>> is called on ICE PMD driver, that cause a test case failure.
> >>>>
> >>>
> >>> Hmmm. I agree on the following. It doesn't need dev_configure.
> >>> That's why I give you ack.
> >>>
> >>> But your issue. Shouldn't you fix the driver? The vsi->rss_key[]
> >>> just updated itself as a random value when dev_conf doesn't contain
> one.
> >>> But your case is dev_conf doesn't contain new rss key, but
> >>> vsi->rss_key is not null. In this case, it should keep the same not
> >>> vsi->get a new
> >> random key.
> >>>
> >> In current implementation, dev->data->dev_conf.rx_adv_conf.rss_conf
> >> in PMD does not hold the rss_key value if user does not set it in the
> >> input parameter of rte_eth_dev_configure().
> >>
> >> Even if PMD generate one automatically, it will not be saved in
> >> dev->data-
> >>> dev_conf.rx_adv_conf.rss_conf.
> >>
> >> When user want to get rss_key, it will be retrieved on the fly from
> >> hardware, but not from any variable in PMD.
> >>
> >> So PMD (ice and i40e) think only rss_key  which is set by user via
> >> rte_eth_dev_configure() can be reused when port is reconfigured.
> >>
> >> If it is not present, PMD will generate one by itself anyway even if
> >> it is present in
> >> vsi->rss_key.
> >>
> >> I don’t think this behavior is wrong, so did not fix ice PMD.
> >
> > If you want to recover the rss key. The driver should store the key in vsi-
> >rss_key also in ice_rss_hash_update(). Then when user not config rss_key
> in rss_conf and vs->rss_key is not 0, you should set the original value not a
> random value to hw.
> >
> 
> +1

The background of the patch is:
A test case expects that RSS feature makes the same packet flow into same queue,
after queue is reconfigured or device is reconfigured,  even if user does not use a 
RSS key to configure a device in advance.
The test case is actually over testing, and tester has agreed to modify test plan to 
make it pass. If we only care about the test case, testpmd and ice PMD does not
need any patch.

But tester found a hidden problem when run the above case:
Configure per queue rx offloading and per queue tx offloading command
trigger the rte_eth_dev_configure() to reconfigure device. 

The problem for the above test case can be resolved by setting RSS key in advance.
And looks like no imminent risk to deprecate the patch.
But it does not mean that the per queue command triggering device reconfiguration is correct.
So leave behind the test case, the patch is still reasonable. 

@Ferruh, if there is still concern for this patch, please comments on it, thanks!
If there is no objection, I will change the patch back to new state, and move on.

> 
> > Otherwise, you are assuming the behavior rss to different queues is right
> since user want random key. It's not an issue.
> >
> > Any scenario, it's not testpmd issue. Please withdraw your patch.
> >
> >>
> >>>> There is an unnecessary call in original implementation because
> >>>> both
> >>>> cmd_config_per_queue_rx_offload_parsed() and
> >>>> cmd_config_per_queue_tx_offload_parsed()
> >>>> does not update the "port->dev_conf" which hold the port
> >>>> configuration, therefore there is no need to call
> rte_eth_dev_configure().
> >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-15  6:04             ` Yu, DapengX
@ 2021-04-19 15:46               ` Ferruh Yigit
  2021-04-20  8:12                 ` Yu, DapengX
  0 siblings, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2021-04-19 15:46 UTC (permalink / raw)
  To: Yu, DapengX, Li, Xiaoyun, Zhang, Qi Z; +Cc: dev, stable

On 4/15/2021 7:04 AM, Yu, DapengX wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, April 12, 2021 8:46 PM
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yu, DapengX
>> <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
>> reconfig cmd
>>
>> On 4/12/2021 3:21 AM, Li, Xiaoyun wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Yu, DapengX <dapengx.yu@intel.com>
>>>> Sent: Friday, April 9, 2021 18:29
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
>>>> offload reconfig cmd
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>>> Sent: Friday, April 9, 2021 3:50 PM
>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
>>>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
>>>>> offload reconfig cmd
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yu, DapengX <dapengx.yu@intel.com>
>>>>>> Sent: Friday, April 9, 2021 13:25
>>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
>>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
>>>>>> offload reconfig cmd
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>>> Sent: Thursday, April 8, 2021 11:42 PM
>>>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
>>>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
>>>>>>> Tx offload reconfig cmd
>>>>>>>
>>>>>>> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
>>>>>>>> From: Dapeng Yu <dapengx.yu@intel.com>
>>>>>>>>
>>>>>>>> Configure per queue rx offloading and per queue tx offloading
>>>>>>>> command shouldn't trigger the rte_eth_dev_configure() to
>>>>>>>> reconfigure
>>>>>> device.
>>>>>>>>
>>>>>>>> The patch sets the queue reconfiguration flag only, and does not
>>>>>>>> set the device reconfiguration flag. Therefore after port is
>>>>>>>> restarted,
>>>>>>>> rte_eth_dev_configure() will not be called again.
>>>>>>>>
>>>>>>>
>>>>>>> Just to clarify the impact, was calling 'rte_eth_dev_configure()'
>>>>>>> causing any problem, is this fixing any issue?
>>>>>>> Or is this patch an optimization to eliminate an unnecessary call?
>>>>>>>
>>>>>> This patch does fix an issue, and it also eliminates an unnecessary call.
>>>>>>
>>>>>> The issue is:
>>>>>> per-queue configuration, for example: port 0 rxq 0 rx_offload
>>>>>> jumbo_frame off triggers the per-device configuration change: the
>>>>>> RSS key is reconfigured and changes after rte_eth_dev_configure()
>>>>>> is called on ICE PMD driver, that cause a test case failure.
>>>>>>
>>>>>
>>>>> Hmmm. I agree on the following. It doesn't need dev_configure.
>>>>> That's why I give you ack.
>>>>>
>>>>> But your issue. Shouldn't you fix the driver? The vsi->rss_key[]
>>>>> just updated itself as a random value when dev_conf doesn't contain
>> one.
>>>>> But your case is dev_conf doesn't contain new rss key, but
>>>>> vsi->rss_key is not null. In this case, it should keep the same not
>>>>> vsi->get a new
>>>> random key.
>>>>>
>>>> In current implementation, dev->data->dev_conf.rx_adv_conf.rss_conf
>>>> in PMD does not hold the rss_key value if user does not set it in the
>>>> input parameter of rte_eth_dev_configure().
>>>>
>>>> Even if PMD generate one automatically, it will not be saved in
>>>> dev->data-
>>>>> dev_conf.rx_adv_conf.rss_conf.
>>>>
>>>> When user want to get rss_key, it will be retrieved on the fly from
>>>> hardware, but not from any variable in PMD.
>>>>
>>>> So PMD (ice and i40e) think only rss_key  which is set by user via
>>>> rte_eth_dev_configure() can be reused when port is reconfigured.
>>>>
>>>> If it is not present, PMD will generate one by itself anyway even if
>>>> it is present in
>>>> vsi->rss_key.
>>>>
>>>> I don’t think this behavior is wrong, so did not fix ice PMD.
>>>
>>> If you want to recover the rss key. The driver should store the key in vsi-
>>> rss_key also in ice_rss_hash_update(). Then when user not config rss_key
>> in rss_conf and vs->rss_key is not 0, you should set the original value not a
>> random value to hw.
>>>
>>
>> +1
> 
> The background of the patch is:
> A test case expects that RSS feature makes the same packet flow into same queue,
> after queue is reconfigured or device is reconfigured,  even if user does not use a
> RSS key to configure a device in advance.
> The test case is actually over testing, and tester has agreed to modify test plan to
> make it pass. If we only care about the test case, testpmd and ice PMD does not
> need any patch.
> 
> But tester found a hidden problem when run the above case:
> Configure per queue rx offloading and per queue tx offloading command
> trigger the rte_eth_dev_configure() to reconfigure device.
> 
> The problem for the above test case can be resolved by setting RSS key in advance.
> And looks like no imminent risk to deprecate the patch.
> But it does not mean that the per queue command triggering device reconfiguration is correct.
> So leave behind the test case, the patch is still reasonable.
> 
> @Ferruh, if there is still concern for this patch, please comments on it, thanks!
> If there is no objection, I will change the patch back to new state, and move on.
> 

Hi Dapeng,

The patch is reasonable, that is why Xiaoyun ack'ed it at first place.

But later we have find out that the change has potential to hide some other 
driver error, so I think it make sense to wait for the driver fix fist, to be 
sure this change won't hide that issue, later we can proceed with this patch.

Calling an API redundantly on the control path is not so bad, we can live with 
it until other issue solved.

>>
>>> Otherwise, you are assuming the behavior rss to different queues is right
>> since user want random key. It's not an issue.
>>>
>>> Any scenario, it's not testpmd issue. Please withdraw your patch.
>>>
>>>>
>>>>>> There is an unnecessary call in original implementation because
>>>>>> both
>>>>>> cmd_config_per_queue_rx_offload_parsed() and
>>>>>> cmd_config_per_queue_tx_offload_parsed()
>>>>>> does not update the "port->dev_conf" which hold the port
>>>>>> configuration, therefore there is no need to call
>> rte_eth_dev_configure().
>>>
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-19 15:46               ` Ferruh Yigit
@ 2021-04-20  8:12                 ` Yu, DapengX
  2021-04-20  8:22                   ` Ferruh Yigit
  0 siblings, 1 reply; 13+ messages in thread
From: Yu, DapengX @ 2021-04-20  8:12 UTC (permalink / raw)
  To: Yigit, Ferruh, Li, Xiaoyun, Zhang, Qi Z; +Cc: dev, stable



> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Monday, April 19, 2021 11:46 PM
> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
> reconfig cmd
> 
> On 4/15/2021 7:04 AM, Yu, DapengX wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Monday, April 12, 2021 8:46 PM
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yu, DapengX
> >> <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> >> offload reconfig cmd
> >>
> >> On 4/12/2021 3:21 AM, Li, Xiaoyun wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Yu, DapengX <dapengx.yu@intel.com>
> >>>> Sent: Friday, April 9, 2021 18:29
> >>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> >>>> offload reconfig cmd
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> >>>>> Sent: Friday, April 9, 2021 3:50 PM
> >>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
> >>>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
> >>>>> Tx offload reconfig cmd
> >>>>>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yu, DapengX <dapengx.yu@intel.com>
> >>>>>> Sent: Friday, April 9, 2021 13:25
> >>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
> >>>>>> Tx offload reconfig cmd
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>>> Sent: Thursday, April 8, 2021 11:42 PM
> >>>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> >>>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
> >>>>>>> Tx offload reconfig cmd
> >>>>>>>
> >>>>>>> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> >>>>>>>> From: Dapeng Yu <dapengx.yu@intel.com>
> >>>>>>>>
> >>>>>>>> Configure per queue rx offloading and per queue tx offloading
> >>>>>>>> command shouldn't trigger the rte_eth_dev_configure() to
> >>>>>>>> reconfigure
> >>>>>> device.
> >>>>>>>>
> >>>>>>>> The patch sets the queue reconfiguration flag only, and does
> >>>>>>>> not set the device reconfiguration flag. Therefore after port
> >>>>>>>> is restarted,
> >>>>>>>> rte_eth_dev_configure() will not be called again.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Just to clarify the impact, was calling 'rte_eth_dev_configure()'
> >>>>>>> causing any problem, is this fixing any issue?
> >>>>>>> Or is this patch an optimization to eliminate an unnecessary call?
> >>>>>>>
> >>>>>> This patch does fix an issue, and it also eliminates an unnecessary call.
> >>>>>>
> >>>>>> The issue is:
> >>>>>> per-queue configuration, for example: port 0 rxq 0 rx_offload
> >>>>>> jumbo_frame off triggers the per-device configuration change: the
> >>>>>> RSS key is reconfigured and changes after rte_eth_dev_configure()
> >>>>>> is called on ICE PMD driver, that cause a test case failure.
> >>>>>>
> >>>>>
> >>>>> Hmmm. I agree on the following. It doesn't need dev_configure.
> >>>>> That's why I give you ack.
> >>>>>
> >>>>> But your issue. Shouldn't you fix the driver? The vsi->rss_key[]
> >>>>> just updated itself as a random value when dev_conf doesn't
> >>>>> contain
> >> one.
> >>>>> But your case is dev_conf doesn't contain new rss key, but
> >>>>> vsi->rss_key is not null. In this case, it should keep the same
> >>>>> vsi->not get a new
> >>>> random key.
> >>>>>
> >>>> In current implementation, dev->data-
> >dev_conf.rx_adv_conf.rss_conf
> >>>> in PMD does not hold the rss_key value if user does not set it in
> >>>> the input parameter of rte_eth_dev_configure().
> >>>>
> >>>> Even if PMD generate one automatically, it will not be saved in
> >>>> dev->data-
> >>>>> dev_conf.rx_adv_conf.rss_conf.
> >>>>
> >>>> When user want to get rss_key, it will be retrieved on the fly from
> >>>> hardware, but not from any variable in PMD.
> >>>>
> >>>> So PMD (ice and i40e) think only rss_key  which is set by user via
> >>>> rte_eth_dev_configure() can be reused when port is reconfigured.
> >>>>
> >>>> If it is not present, PMD will generate one by itself anyway even
> >>>> if it is present in
> >>>> vsi->rss_key.
> >>>>
> >>>> I don’t think this behavior is wrong, so did not fix ice PMD.
> >>>
> >>> If you want to recover the rss key. The driver should store the key
> >>> in vsi- rss_key also in ice_rss_hash_update(). Then when user not
> >>> config rss_key
> >> in rss_conf and vs->rss_key is not 0, you should set the original
> >> value not a random value to hw.
> >>>
> >>
> >> +1
> >
> > The background of the patch is:
> > A test case expects that RSS feature makes the same packet flow into
> > same queue, after queue is reconfigured or device is reconfigured,
> > even if user does not use a RSS key to configure a device in advance.
> > The test case is actually over testing, and tester has agreed to
> > modify test plan to make it pass. If we only care about the test case,
> > testpmd and ice PMD does not need any patch.
> >
> > But tester found a hidden problem when run the above case:
> > Configure per queue rx offloading and per queue tx offloading command
> > trigger the rte_eth_dev_configure() to reconfigure device.
> >
> > The problem for the above test case can be resolved by setting RSS key in
> advance.
> > And looks like no imminent risk to deprecate the patch.
> > But it does not mean that the per queue command triggering device
> reconfiguration is correct.
> > So leave behind the test case, the patch is still reasonable.
> >
> > @Ferruh, if there is still concern for this patch, please comments on it,
> thanks!
> > If there is no objection, I will change the patch back to new state, and move
> on.
> >
> 
> Hi Dapeng,
> 
> The patch is reasonable, that is why Xiaoyun ack'ed it at first place.
> 
> But later we have find out that the change has potential to hide some other
> driver error, so I think it make sense to wait for the driver fix fist, to be sure
> this change won't hide that issue, later we can proceed with this patch.
> 
> Calling an API redundantly on the control path is not so bad, we can live with
> it until other issue solved.
> 

@Ferruh, thank you for sparing time to answer.
The previous comments mentioned a controversy that there is error in ICE PMD driver(Random RSS key generation on device reconfiguration), 
Actually this is not bug, and it has been confirmed with maintainer(Zhang Qi), there is no need to fix it.
Are there other driver errors which this patch may hide? If not, I think we can proceed with this patch.

> >>
> >>> Otherwise, you are assuming the behavior rss to different queues is
> >>> right
> >> since user want random key. It's not an issue.
> >>>
> >>> Any scenario, it's not testpmd issue. Please withdraw your patch.
> >>>
> >>>>
> >>>>>> There is an unnecessary call in original implementation because
> >>>>>> both
> >>>>>> cmd_config_per_queue_rx_offload_parsed() and
> >>>>>> cmd_config_per_queue_tx_offload_parsed()
> >>>>>> does not update the "port->dev_conf" which hold the port
> >>>>>> configuration, therefore there is no need to call
> >> rte_eth_dev_configure().
> >>>
> >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-20  8:12                 ` Yu, DapengX
@ 2021-04-20  8:22                   ` Ferruh Yigit
  2021-04-20  8:40                     ` Yu, DapengX
  0 siblings, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2021-04-20  8:22 UTC (permalink / raw)
  To: Yu, DapengX, Li, Xiaoyun, Zhang, Qi Z; +Cc: dev, stable

On 4/20/2021 9:12 AM, Yu, DapengX wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Monday, April 19, 2021 11:46 PM
>> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
>> reconfig cmd
>>
>> On 4/15/2021 7:04 AM, Yu, DapengX wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: Monday, April 12, 2021 8:46 PM
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yu, DapengX
>>>> <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
>>>> offload reconfig cmd
>>>>
>>>> On 4/12/2021 3:21 AM, Li, Xiaoyun wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yu, DapengX <dapengx.yu@intel.com>
>>>>>> Sent: Friday, April 9, 2021 18:29
>>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
>>>>>> offload reconfig cmd
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>>>>> Sent: Friday, April 9, 2021 3:50 PM
>>>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
>>>>>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
>>>>>>> Tx offload reconfig cmd
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Yu, DapengX <dapengx.yu@intel.com>
>>>>>>>> Sent: Friday, April 9, 2021 13:25
>>>>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
>>>>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
>>>>>>>> Tx offload reconfig cmd
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>>>>> Sent: Thursday, April 8, 2021 11:42 PM
>>>>>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
>>>>>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
>>>>>>>>> Tx offload reconfig cmd
>>>>>>>>>
>>>>>>>>> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
>>>>>>>>>> From: Dapeng Yu <dapengx.yu@intel.com>
>>>>>>>>>>
>>>>>>>>>> Configure per queue rx offloading and per queue tx offloading
>>>>>>>>>> command shouldn't trigger the rte_eth_dev_configure() to
>>>>>>>>>> reconfigure
>>>>>>>> device.
>>>>>>>>>>
>>>>>>>>>> The patch sets the queue reconfiguration flag only, and does
>>>>>>>>>> not set the device reconfiguration flag. Therefore after port
>>>>>>>>>> is restarted,
>>>>>>>>>> rte_eth_dev_configure() will not be called again.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Just to clarify the impact, was calling 'rte_eth_dev_configure()'
>>>>>>>>> causing any problem, is this fixing any issue?
>>>>>>>>> Or is this patch an optimization to eliminate an unnecessary call?
>>>>>>>>>
>>>>>>>> This patch does fix an issue, and it also eliminates an unnecessary call.
>>>>>>>>
>>>>>>>> The issue is:
>>>>>>>> per-queue configuration, for example: port 0 rxq 0 rx_offload
>>>>>>>> jumbo_frame off triggers the per-device configuration change: the
>>>>>>>> RSS key is reconfigured and changes after rte_eth_dev_configure()
>>>>>>>> is called on ICE PMD driver, that cause a test case failure.
>>>>>>>>
>>>>>>>
>>>>>>> Hmmm. I agree on the following. It doesn't need dev_configure.
>>>>>>> That's why I give you ack.
>>>>>>>
>>>>>>> But your issue. Shouldn't you fix the driver? The vsi->rss_key[]
>>>>>>> just updated itself as a random value when dev_conf doesn't
>>>>>>> contain
>>>> one.
>>>>>>> But your case is dev_conf doesn't contain new rss key, but
>>>>>>> vsi->rss_key is not null. In this case, it should keep the same
>>>>>>> vsi->not get a new
>>>>>> random key.
>>>>>>>
>>>>>> In current implementation, dev->data-
>>> dev_conf.rx_adv_conf.rss_conf
>>>>>> in PMD does not hold the rss_key value if user does not set it in
>>>>>> the input parameter of rte_eth_dev_configure().
>>>>>>
>>>>>> Even if PMD generate one automatically, it will not be saved in
>>>>>> dev->data-
>>>>>>> dev_conf.rx_adv_conf.rss_conf.
>>>>>>
>>>>>> When user want to get rss_key, it will be retrieved on the fly from
>>>>>> hardware, but not from any variable in PMD.
>>>>>>
>>>>>> So PMD (ice and i40e) think only rss_key  which is set by user via
>>>>>> rte_eth_dev_configure() can be reused when port is reconfigured.
>>>>>>
>>>>>> If it is not present, PMD will generate one by itself anyway even
>>>>>> if it is present in
>>>>>> vsi->rss_key.
>>>>>>
>>>>>> I don’t think this behavior is wrong, so did not fix ice PMD.
>>>>>
>>>>> If you want to recover the rss key. The driver should store the key
>>>>> in vsi- rss_key also in ice_rss_hash_update(). Then when user not
>>>>> config rss_key
>>>> in rss_conf and vs->rss_key is not 0, you should set the original
>>>> value not a random value to hw.
>>>>>
>>>>
>>>> +1
>>>
>>> The background of the patch is:
>>> A test case expects that RSS feature makes the same packet flow into
>>> same queue, after queue is reconfigured or device is reconfigured,
>>> even if user does not use a RSS key to configure a device in advance.
>>> The test case is actually over testing, and tester has agreed to
>>> modify test plan to make it pass. If we only care about the test case,
>>> testpmd and ice PMD does not need any patch.
>>>
>>> But tester found a hidden problem when run the above case:
>>> Configure per queue rx offloading and per queue tx offloading command
>>> trigger the rte_eth_dev_configure() to reconfigure device.
>>>
>>> The problem for the above test case can be resolved by setting RSS key in
>> advance.
>>> And looks like no imminent risk to deprecate the patch.
>>> But it does not mean that the per queue command triggering device
>> reconfiguration is correct.
>>> So leave behind the test case, the patch is still reasonable.
>>>
>>> @Ferruh, if there is still concern for this patch, please comments on it,
>> thanks!
>>> If there is no objection, I will change the patch back to new state, and move
>> on.
>>>
>>
>> Hi Dapeng,
>>
>> The patch is reasonable, that is why Xiaoyun ack'ed it at first place.
>>
>> But later we have find out that the change has potential to hide some other
>> driver error, so I think it make sense to wait for the driver fix fist, to be sure
>> this change won't hide that issue, later we can proceed with this patch.
>>
>> Calling an API redundantly on the control path is not so bad, we can live with
>> it until other issue solved.
>>
> 
> @Ferruh, thank you for sparing time to answer.
> The previous comments mentioned a controversy that there is error in ICE PMD driver(Random RSS key generation on device reconfiguration),
> Actually this is not bug, and it has been confirmed with maintainer(Zhang Qi), there is no need to fix it.

Wasn't the problem that device get a new random RSS key on each call of the 
configure API, is this expected behavior?

And will the RSS key change even if the user set the RSS key between two 
configure calls?

> Are there other driver errors which this patch may hide? If not, I think we can proceed with this patch.
> 
>>>>
>>>>> Otherwise, you are assuming the behavior rss to different queues is
>>>>> right
>>>> since user want random key. It's not an issue.
>>>>>
>>>>> Any scenario, it's not testpmd issue. Please withdraw your patch.
>>>>>
>>>>>>
>>>>>>>> There is an unnecessary call in original implementation because
>>>>>>>> both
>>>>>>>> cmd_config_per_queue_rx_offload_parsed() and
>>>>>>>> cmd_config_per_queue_tx_offload_parsed()
>>>>>>>> does not update the "port->dev_conf" which hold the port
>>>>>>>> configuration, therefore there is no need to call
>>>> rte_eth_dev_configure().
>>>>>
>>>
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd
  2021-04-20  8:22                   ` Ferruh Yigit
@ 2021-04-20  8:40                     ` Yu, DapengX
  0 siblings, 0 replies; 13+ messages in thread
From: Yu, DapengX @ 2021-04-20  8:40 UTC (permalink / raw)
  To: Yigit, Ferruh, Li, Xiaoyun, Zhang, Qi Z; +Cc: dev, stable



> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Tuesday, April 20, 2021 4:22 PM
> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload
> reconfig cmd
> 
> On 4/20/2021 9:12 AM, Yu, DapengX wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Sent: Monday, April 19, 2021 11:46 PM
> >> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> >> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> >> offload reconfig cmd
> >>
> >> On 4/15/2021 7:04 AM, Yu, DapengX wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> Sent: Monday, April 12, 2021 8:46 PM
> >>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yu, DapengX
> >>>> <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
> >>>> offload reconfig cmd
> >>>>
> >>>> On 4/12/2021 3:21 AM, Li, Xiaoyun wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yu, DapengX <dapengx.yu@intel.com>
> >>>>>> Sent: Friday, April 9, 2021 18:29
> >>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >>>>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
> >>>>>> Tx offload reconfig cmd
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> >>>>>>> Sent: Friday, April 9, 2021 3:50 PM
> >>>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Yigit, Ferruh
> >>>>>>> <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
> >>>>>>> Tx offload reconfig cmd
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Yu, DapengX <dapengx.yu@intel.com>
> >>>>>>>> Sent: Friday, April 9, 2021 13:25
> >>>>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >>>>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx
> >>>>>>>> and Tx offload reconfig cmd
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>>>>> Sent: Thursday, April 8, 2021 11:42 PM
> >>>>>>>>> To: Yu, DapengX <dapengx.yu@intel.com>; Li, Xiaoyun
> >>>>>>>>> <xiaoyun.li@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx
> >>>>>>>>> and Tx offload reconfig cmd
> >>>>>>>>>
> >>>>>>>>> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote:
> >>>>>>>>>> From: Dapeng Yu <dapengx.yu@intel.com>
> >>>>>>>>>>
> >>>>>>>>>> Configure per queue rx offloading and per queue tx offloading
> >>>>>>>>>> command shouldn't trigger the rte_eth_dev_configure() to
> >>>>>>>>>> reconfigure
> >>>>>>>> device.
> >>>>>>>>>>
> >>>>>>>>>> The patch sets the queue reconfiguration flag only, and does
> >>>>>>>>>> not set the device reconfiguration flag. Therefore after port
> >>>>>>>>>> is restarted,
> >>>>>>>>>> rte_eth_dev_configure() will not be called again.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Just to clarify the impact, was calling 'rte_eth_dev_configure()'
> >>>>>>>>> causing any problem, is this fixing any issue?
> >>>>>>>>> Or is this patch an optimization to eliminate an unnecessary call?
> >>>>>>>>>
> >>>>>>>> This patch does fix an issue, and it also eliminates an unnecessary
> call.
> >>>>>>>>
> >>>>>>>> The issue is:
> >>>>>>>> per-queue configuration, for example: port 0 rxq 0 rx_offload
> >>>>>>>> jumbo_frame off triggers the per-device configuration change:
> >>>>>>>> the RSS key is reconfigured and changes after
> >>>>>>>> rte_eth_dev_configure() is called on ICE PMD driver, that cause a
> test case failure.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Hmmm. I agree on the following. It doesn't need dev_configure.
> >>>>>>> That's why I give you ack.
> >>>>>>>
> >>>>>>> But your issue. Shouldn't you fix the driver? The vsi->rss_key[]
> >>>>>>> just updated itself as a random value when dev_conf doesn't
> >>>>>>> contain
> >>>> one.
> >>>>>>> But your case is dev_conf doesn't contain new rss key, but
> >>>>>>> vsi->rss_key is not null. In this case, it should keep the same
> >>>>>>> vsi->not get a new
> >>>>>> random key.
> >>>>>>>
> >>>>>> In current implementation, dev->data-
> >>> dev_conf.rx_adv_conf.rss_conf
> >>>>>> in PMD does not hold the rss_key value if user does not set it in
> >>>>>> the input parameter of rte_eth_dev_configure().
> >>>>>>
> >>>>>> Even if PMD generate one automatically, it will not be saved in
> >>>>>> dev->data-
> >>>>>>> dev_conf.rx_adv_conf.rss_conf.
> >>>>>>
> >>>>>> When user want to get rss_key, it will be retrieved on the fly
> >>>>>> from hardware, but not from any variable in PMD.
> >>>>>>
> >>>>>> So PMD (ice and i40e) think only rss_key  which is set by user
> >>>>>> via
> >>>>>> rte_eth_dev_configure() can be reused when port is reconfigured.
> >>>>>>
> >>>>>> If it is not present, PMD will generate one by itself anyway even
> >>>>>> if it is present in
> >>>>>> vsi->rss_key.
> >>>>>>
> >>>>>> I don’t think this behavior is wrong, so did not fix ice PMD.
> >>>>>
> >>>>> If you want to recover the rss key. The driver should store the
> >>>>> key in vsi- rss_key also in ice_rss_hash_update(). Then when user
> >>>>> not config rss_key
> >>>> in rss_conf and vs->rss_key is not 0, you should set the original
> >>>> value not a random value to hw.
> >>>>>
> >>>>
> >>>> +1
> >>>
> >>> The background of the patch is:
> >>> A test case expects that RSS feature makes the same packet flow into
> >>> same queue, after queue is reconfigured or device is reconfigured,
> >>> even if user does not use a RSS key to configure a device in advance.
> >>> The test case is actually over testing, and tester has agreed to
> >>> modify test plan to make it pass. If we only care about the test
> >>> case, testpmd and ice PMD does not need any patch.
> >>>
> >>> But tester found a hidden problem when run the above case:
> >>> Configure per queue rx offloading and per queue tx offloading
> >>> command trigger the rte_eth_dev_configure() to reconfigure device.
> >>>
> >>> The problem for the above test case can be resolved by setting RSS
> >>> key in
> >> advance.
> >>> And looks like no imminent risk to deprecate the patch.
> >>> But it does not mean that the per queue command triggering device
> >> reconfiguration is correct.
> >>> So leave behind the test case, the patch is still reasonable.
> >>>
> >>> @Ferruh, if there is still concern for this patch, please comments
> >>> on it,
> >> thanks!
> >>> If there is no objection, I will change the patch back to new state,
> >>> and move
> >> on.
> >>>
> >>
> >> Hi Dapeng,
> >>
> >> The patch is reasonable, that is why Xiaoyun ack'ed it at first place.
> >>
> >> But later we have find out that the change has potential to hide some
> >> other driver error, so I think it make sense to wait for the driver
> >> fix fist, to be sure this change won't hide that issue, later we can proceed
> with this patch.
> >>
> >> Calling an API redundantly on the control path is not so bad, we can
> >> live with it until other issue solved.
> >>
> >
> > @Ferruh, thank you for sparing time to answer.
> > The previous comments mentioned a controversy that there is error in
> > ICE PMD driver(Random RSS key generation on device reconfiguration),
> Actually this is not bug, and it has been confirmed with maintainer(Zhang Qi),
> there is no need to fix it.
> 
> Wasn't the problem that device get a new random RSS key on each call of the
> configure API, is this expected behavior?

The behavior is expected for ICE PMD, since user did not give a RSS key on device configuration.
If user did not set RSS key, it implies that user did not care about what is RSS key value, and how it will affect packet flow.
So any RSS key value should be acceptable.

> 
> And will the RSS key change even if the user set the RSS key between two
> configure calls?

Actually if user set the RSS key between two configure call, The RSS key will be changed to the one that user set. No random RSS key will be generated automatically.

> 
> > Are there other driver errors which this patch may hide? If not, I think we
> can proceed with this patch.
> >
> >>>>
> >>>>> Otherwise, you are assuming the behavior rss to different queues
> >>>>> is right
> >>>> since user want random key. It's not an issue.
> >>>>>
> >>>>> Any scenario, it's not testpmd issue. Please withdraw your patch.
> >>>>>
> >>>>>>
> >>>>>>>> There is an unnecessary call in original implementation because
> >>>>>>>> both
> >>>>>>>> cmd_config_per_queue_rx_offload_parsed() and
> >>>>>>>> cmd_config_per_queue_tx_offload_parsed()
> >>>>>>>> does not update the "port->dev_conf" which hold the port
> >>>>>>>> configuration, therefore there is no need to call
> >>>> rte_eth_dev_configure().
> >>>>>
> >>>
> >


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-04-20  8:40 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01  8:28 [dpdk-dev] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd dapengx.yu
2021-04-07  2:30 ` Li, Xiaoyun
2021-04-08 15:41 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2021-04-09  5:25   ` Yu, DapengX
2021-04-09  7:50     ` Li, Xiaoyun
2021-04-09 10:29       ` Yu, DapengX
2021-04-12  2:21         ` Li, Xiaoyun
2021-04-12 12:46           ` Ferruh Yigit
2021-04-15  6:04             ` Yu, DapengX
2021-04-19 15:46               ` Ferruh Yigit
2021-04-20  8:12                 ` Yu, DapengX
2021-04-20  8:22                   ` Ferruh Yigit
2021-04-20  8:40                     ` Yu, DapengX

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).