diff mbox series

[v4] net/i40e: fix incorrect hash look up table

Message ID 20200724024712.10727-1-shougangx.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series [v4] net/i40e: fix incorrect hash look up table |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Shougang Wang July 24, 2020, 2:47 a.m. UTC 
  The hash look up table (LUT) is managed by global register but it is not
initialized when RSS is disabled. Once user wants to enable RSS during
runtime, the LUT will not be initialized.
This patch fixes the issue by initializing the LUT whether RSS enabled
or not.

Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS flow")
Cc: stable@dpdk.org

Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
---
v4:
-Updated code.
---
 drivers/net/i40e/i40e_ethdev.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

Comments

Guo, Jia July 24, 2020, 3:57 a.m. UTC | #1 
hi, shougang

On 7/24/2020 10:47 AM, Shougang Wang wrote:
> The hash look up table (LUT) is managed by global register but it is not
> initialized when RSS is disabled. Once user wants to enable RSS during
> runtime, the LUT will not be initialized.
> This patch fixes the issue by initializing the LUT whether RSS enabled
> or not.


"whatever" but not "whether"?


>
> Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS flow")
> Cc: stable@dpdk.org
>
> Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> ---
> v4:
> -Updated code.
> ---
>   drivers/net/i40e/i40e_ethdev.c | 15 ++++-----------
>   1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 05d5f2861..0a3f5e3c1 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -8985,6 +8985,7 @@ static int
>   i40e_pf_config_rss(struct i40e_pf *pf)
>   {
>   	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> +	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
>   	struct rte_eth_rss_conf rss_conf;
>   	uint32_t i, lut = 0;
>   	uint16_t j, num;
> @@ -9022,7 +9023,8 @@ i40e_pf_config_rss(struct i40e_pf *pf)
>   	}
>   
>   	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
> -	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) {
> +	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0 ||
> +	    !(mq_mode & ETH_MQ_RX_RSS_FLAG)) {
>   		i40e_pf_disable_rss(pf);
>   		return 0;
>   	}
> @@ -9198,16 +9200,7 @@ i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
>   static int
>   i40e_pf_config_mq_rx(struct i40e_pf *pf)


This function is still need or could it be replace by i40e_pf_config_rss?


>   {
> -	int ret = 0;
> -	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
> -
> -	/* RSS setup */
> -	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
> -		ret = i40e_pf_config_rss(pf);
> -	else
> -		i40e_pf_disable_rss(pf);
> -
> -	return ret;
> +	return i40e_pf_config_rss(pf);
>   }
>   
>   /* Get the symmetric hash enable configurations per port */
Shougang Wang July 24, 2020, 4:49 a.m. UTC | #2 
Hi, Jeff

> -----Original Message-----
> From: Guo, Jia <jia.guo@intel.com>
> Sent: Friday, July 24, 2020 11:58 AM
> To: Wang, ShougangX <shougangx.wang@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v4] net/i40e: fix incorrect hash look up table
> 
> hi, shougang
> 
> On 7/24/2020 10:47 AM, Shougang Wang wrote:
> > The hash look up table (LUT) is managed by global register but it is
> > not initialized when RSS is disabled. Once user wants to enable RSS
> > during runtime, the LUT will not be initialized.
> > This patch fixes the issue by initializing the LUT whether RSS enabled
> > or not.
> 
> 
> "whatever" but not "whether"?
"whatever", I will fix it.

> 
> 
> >
> > Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS
> > flow")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> > ---
> > v4:
> > -Updated code.
> > ---
> >   drivers/net/i40e/i40e_ethdev.c | 15 ++++-----------
> >   1 file changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 05d5f2861..0a3f5e3c1 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -8985,6 +8985,7 @@ static int
> >   i40e_pf_config_rss(struct i40e_pf *pf)
> >   {
> >   	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> > +	enum rte_eth_rx_mq_mode mq_mode =
> > +pf->dev_data->dev_conf.rxmode.mq_mode;
> >   	struct rte_eth_rss_conf rss_conf;
> >   	uint32_t i, lut = 0;
> >   	uint16_t j, num;
> > @@ -9022,7 +9023,8 @@ i40e_pf_config_rss(struct i40e_pf *pf)
> >   	}
> >
> >   	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
> > -	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) {
> > +	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0 ||
> > +	    !(mq_mode & ETH_MQ_RX_RSS_FLAG)) {
> >   		i40e_pf_disable_rss(pf);
> >   		return 0;
> >   	}
> > @@ -9198,16 +9200,7 @@ i40e_tunnel_filter_handle(struct rte_eth_dev
> *dev,
> >   static int
> >   i40e_pf_config_mq_rx(struct i40e_pf *pf)
> 
> 
> This function is still need or could it be replace by i40e_pf_config_rss?
It can be replaced by i40e_pf_config_rss, thanks for your review.
Qiming Yang July 24, 2020, 5:07 a.m. UTC | #3 
Hi, Shougang
This version looks better, only two small questions.
Once somebody gave comments to your patch, you should reply the comments and CC when you send patch next version.
You don't include me in this version, don't forget this next time.

Qiming
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
> Sent: Friday, July 24, 2020 10:47
> To: dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang,
> ShougangX <shougangx.wang@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up table
> 
> The hash look up table (LUT) is managed by global register but it is not
> initialized when RSS is disabled. Once user wants to enable RSS during
> runtime, the LUT will not be initialized.
> This patch fixes the issue by initializing the LUT whether RSS enabled or not.
> 
> Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS flow")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> ---
> v4:
> -Updated code.
> ---
>  drivers/net/i40e/i40e_ethdev.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 05d5f2861..0a3f5e3c1 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -8985,6 +8985,7 @@ static int
>  i40e_pf_config_rss(struct i40e_pf *pf)
>  {
>  	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> +	enum rte_eth_rx_mq_mode mq_mode =
> +pf->dev_data->dev_conf.rxmode.mq_mode;

No tab?

>  	struct rte_eth_rss_conf rss_conf;
>  	uint32_t i, lut = 0;
>  	uint16_t j, num;
> @@ -9022,7 +9023,8 @@ i40e_pf_config_rss(struct i40e_pf *pf)
>  	}
> 
>  	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
> -	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) {
> +	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0 ||
> +	    !(mq_mode & ETH_MQ_RX_RSS_FLAG)) {
>  		i40e_pf_disable_rss(pf);
>  		return 0;
>  	}
> @@ -9198,16 +9200,7 @@ i40e_tunnel_filter_handle(struct rte_eth_dev
> *dev,  static int  i40e_pf_config_mq_rx(struct i40e_pf *pf)  {
> -	int ret = 0;
> -	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data-
> >dev_conf.rxmode.mq_mode;
> -
> -	/* RSS setup */
> -	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
> -		ret = i40e_pf_config_rss(pf);
> -	else
> -		i40e_pf_disable_rss(pf);
> -
> -	return ret;
> +	return i40e_pf_config_rss(pf);

If only have one function call in this function, we can delete it, and just use
i40e_pf_config_rss(pf) instead of i40e_pf_config_mq_rx.

>  }
> 
>  /* Get the symmetric hash enable configurations per port */
> --
> 2.17.1
Shougang Wang July 24, 2020, 5:42 a.m. UTC | #4 
Hi, Qiming

> -----Original Message-----
> From: Yang, Qiming <qiming.yang@intel.com>
> Sent: Friday, July 24, 2020 1:08 PM
> To: Wang, ShougangX <shougangx.wang@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang,
> ShougangX <shougangx.wang@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up table
> 
> Hi, Shougang
> This version looks better, only two small questions.
> Once somebody gave comments to your patch, you should reply the
> comments and CC when you send patch next version.
> You don't include me in this version, don't forget this next time.
Got it, I will keep in mind.

> Qiming
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
> > Sent: Friday, July 24, 2020 10:47
> > To: dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> > <jia.guo@intel.com>; Wang, ShougangX <shougangx.wang@intel.com>;
> > stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up
> > table
> >
> > The hash look up table (LUT) is managed by global register but it is
> > not initialized when RSS is disabled. Once user wants to enable RSS
> > during runtime, the LUT will not be initialized.
> > This patch fixes the issue by initializing the LUT whether RSS enabled or not.
> >
> > Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS
> > flow")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> > ---
> > v4:
> > -Updated code.
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 15 ++++-----------
> >  1 file changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 05d5f2861..0a3f5e3c1 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -8985,6 +8985,7 @@ static int
> >  i40e_pf_config_rss(struct i40e_pf *pf)  {
> >  	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> > +	enum rte_eth_rx_mq_mode mq_mode =
> > +pf->dev_data->dev_conf.rxmode.mq_mode;
> 
> No tab?
Actually, this line is on the same line as the code above, but it looks like two lines here.
I will adjust the position of this definition to follow the "Christmas tree" in next version.

> 
> >  	struct rte_eth_rss_conf rss_conf;
> >  	uint32_t i, lut = 0;
> >  	uint16_t j, num;
> > @@ -9022,7 +9023,8 @@ i40e_pf_config_rss(struct i40e_pf *pf)
> >  	}
> >
> >  	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
> > -	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) {
> > +	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0 ||
> > +	    !(mq_mode & ETH_MQ_RX_RSS_FLAG)) {
> >  		i40e_pf_disable_rss(pf);
> >  		return 0;
> >  	}
> > @@ -9198,16 +9200,7 @@ i40e_tunnel_filter_handle(struct rte_eth_dev
> > *dev,  static int  i40e_pf_config_mq_rx(struct i40e_pf *pf)  {
> > -	int ret = 0;
> > -	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data-
> > >dev_conf.rxmode.mq_mode;
> > -
> > -	/* RSS setup */
> > -	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
> > -		ret = i40e_pf_config_rss(pf);
> > -	else
> > -		i40e_pf_disable_rss(pf);
> > -
> > -	return ret;
> > +	return i40e_pf_config_rss(pf);
> 
> If only have one function call in this function, we can delete it, and just use
> i40e_pf_config_rss(pf) instead of i40e_pf_config_mq_rx.
Got it. Thanks for your review.

Thanks.
Shougang
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 05d5f2861..0a3f5e3c1 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -8985,6 +8985,7 @@  static int
 i40e_pf_config_rss(struct i40e_pf *pf)
 {
 	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
 	struct rte_eth_rss_conf rss_conf;
 	uint32_t i, lut = 0;
 	uint16_t j, num;
@@ -9022,7 +9023,8 @@  i40e_pf_config_rss(struct i40e_pf *pf)
 	}
 
 	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
-	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) {
+	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0 ||
+	    !(mq_mode & ETH_MQ_RX_RSS_FLAG)) {
 		i40e_pf_disable_rss(pf);
 		return 0;
 	}
@@ -9198,16 +9200,7 @@  i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
 static int
 i40e_pf_config_mq_rx(struct i40e_pf *pf)
 {
-	int ret = 0;
-	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
-
-	/* RSS setup */
-	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
-		ret = i40e_pf_config_rss(pf);
-	else
-		i40e_pf_disable_rss(pf);
-
-	return ret;
+	return i40e_pf_config_rss(pf);
 }
 
 /* Get the symmetric hash enable configurations per port */