-----Original Message-----
From: Yigit, Ferruh <ferruh.yi...@intel.com>
Sent: Monday, April 19, 2021 11:46 PM
To: Yu, DapengX <dapengx...@intel.com>; Li, Xiaoyun
<xiaoyun...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
Cc: dev@dpdk.org; sta...@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.yi...@intel.com>
Sent: Monday, April 12, 2021 8:46 PM
To: Li, Xiaoyun <xiaoyun...@intel.com>; Yu, DapengX
<dapengx...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
Cc: dev@dpdk.org; sta...@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...@intel.com>
Sent: Friday, April 9, 2021 18:29
To: Li, Xiaoyun <xiaoyun...@intel.com>; Yigit, Ferruh
<ferruh.yi...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
Cc: dev@dpdk.org; sta...@dpdk.org
Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx
offload reconfig cmd
-----Original Message-----
From: Li, Xiaoyun <xiaoyun...@intel.com>
Sent: Friday, April 9, 2021 3:50 PM
To: Yu, DapengX <dapengx...@intel.com>; Yigit, Ferruh
<ferruh.yi...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
Cc: dev@dpdk.org; sta...@dpdk.org
Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
Tx offload reconfig cmd
-----Original Message-----
From: Yu, DapengX <dapengx...@intel.com>
Sent: Friday, April 9, 2021 13:25
To: Yigit, Ferruh <ferruh.yi...@intel.com>; Li, Xiaoyun
<xiaoyun...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
Cc: dev@dpdk.org; sta...@dpdk.org
Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and
Tx offload reconfig cmd
-----Original Message-----
From: Yigit, Ferruh <ferruh.yi...@intel.com>
Sent: Thursday, April 8, 2021 11:42 PM
To: Yu, DapengX <dapengx...@intel.com>; Li, Xiaoyun
<xiaoyun...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
Cc: dev@dpdk.org; sta...@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...@intel.com wrote:
From: Dapeng Yu <dapengx...@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.
