diff mbox series

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

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

Checks

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

Commit Message

Shougang Wang July 22, 2020, 8:15 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>
---
v3:
-Updated the time of initializing the look up table
---
 drivers/net/i40e/i40e_ethdev.c | 85 ++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 36 deletions(-)

Comments

Qiming Yang July 23, 2020, 1:57 a.m. UTC | #1 
I don't understand why you add new function and new function mostly do the same thing.
Why don't add fix in original code.

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
> Sent: Wednesday, July 22, 2020 16:16
> 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 v3] 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>
> ---
> v3:
> -Updated the time of initializing the look up table
> ---
>  drivers/net/i40e/i40e_ethdev.c | 85 ++++++++++++++++++++--------------
>  1 file changed, 49 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 05d5f2861..e35590d96 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -8984,42 +8984,7 @@ i40e_pf_calc_configured_queues_num(struct
> i40e_pf *pf)  static int  i40e_pf_config_rss(struct i40e_pf *pf)  {
> -	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
>  	struct rte_eth_rss_conf rss_conf;
> -	uint32_t i, lut = 0;
> -	uint16_t j, num;
> -
> -	/*
> -	 * If both VMDQ and RSS enabled, not all of PF queues are
> configured.
> -	 * It's necessary to calculate the actual PF queues that are configured.
> -	 */
> -	if (pf->dev_data->dev_conf.rxmode.mq_mode &
> ETH_MQ_RX_VMDQ_FLAG)
> -		num = i40e_pf_calc_configured_queues_num(pf);
> -	else
> -		num = pf->dev_data->nb_rx_queues;
> -
> -	num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
> -	PMD_INIT_LOG(INFO, "Max of contiguous %u PF queues are
> configured",
> -			num);
> -
> -	if (num == 0) {
> -		PMD_INIT_LOG(ERR,
> -			"No PF queues are configured to enable RSS for port
> %u",
> -			pf->dev_data->port_id);
> -		return -ENOTSUP;
> -	}
> -
> -	if (pf->adapter->rss_reta_updated == 0) {
> -		for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> -			if (j == num)
> -				j = 0;
> -			lut = (lut << 8) | (j & ((0x1 <<
> -				hw->func_caps.rss_table_entry_width) - 1));
> -			if ((i & 3) == 3)
> -				I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >>
> 2),
> -					       rte_bswap32(lut));
> -		}
> -	}
> 
>  	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
>  	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) { @@ -
> 9195,12 +9160,60 @@ i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
>  	return ret;
>  }
> 
> +/* Initialize the hash look up table */ static int
> +i40e_pf_init_rss_lut(struct i40e_pf *pf) {
> +	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> +	uint32_t lut = 0;
> +	uint16_t j, num;
> +	uint32_t i;
> +
> +	/* If both VMDQ and RSS enabled, not all of PF queues are
> configured.
> +	 * It's necessary to calculate the actual PF queues that are configured.
> +	 */
> +	if (pf->dev_data->dev_conf.rxmode.mq_mode &
> ETH_MQ_RX_VMDQ_FLAG)
> +		num = i40e_pf_calc_configured_queues_num(pf);
> +	else
> +		num = pf->dev_data->nb_rx_queues;
> +
> +	num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
> +	PMD_INIT_LOG(INFO, "Max of contiguous %u PF queues are
> configured",
> +		     num);
> +
> +	if (num == 0) {
> +		PMD_INIT_LOG(ERR,
> +			"No PF queues are configured to enable RSS for port
> %u",
> +			pf->dev_data->port_id);
> +		return -ENOTSUP;
> +	}
> +
> +	if (pf->adapter->rss_reta_updated == 0) {
> +		for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> +			if (j == num)
> +				j = 0;
> +			lut = (lut << 8) | (j & ((0x1 <<
> +				hw->func_caps.rss_table_entry_width) - 1));
> +			if ((i & 3) == 3)
> +				I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >>
> 2),
> +					       rte_bswap32(lut));
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  i40e_pf_config_mq_rx(struct i40e_pf *pf)  {
> -	int ret = 0;
> +	int ret;
>  	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data-
> >dev_conf.rxmode.mq_mode;
> 
> +	/* Initialize hash look up table */
> +	ret = i40e_pf_init_rss_lut(pf);
> +	if (ret)
> +		return ret;
> +
>  	/* RSS setup */
>  	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
>  		ret = i40e_pf_config_rss(pf);
> --
> 2.17.1
Qi Zhang July 23, 2020, 12:53 p.m. UTC | #2 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Yang, Qiming
> Sent: Thursday, July 23, 2020 9:57 AM
> 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 v3] net/i40e: fix incorrect hash look up table
> 
> I don't understand why you add new function and new function mostly do the
> same thing.
> Why don't add fix in original code.
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
> > Sent: Wednesday, July 22, 2020 16:16
> > 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 v3] 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>
> > ---
> > v3:
> > -Updated the time of initializing the look up table
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 85
> > ++++++++++++++++++++--------------
> >  1 file changed, 49 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 05d5f2861..e35590d96 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -8984,42 +8984,7 @@ i40e_pf_calc_configured_queues_num(struct
> > i40e_pf *pf)  static int  i40e_pf_config_rss(struct i40e_pf *pf)  {
> > -	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> >  	struct rte_eth_rss_conf rss_conf;
> > -	uint32_t i, lut = 0;
> > -	uint16_t j, num;
> > -
> > -	/*
> > -	 * If both VMDQ and RSS enabled, not all of PF queues are
> > configured.
> > -	 * It's necessary to calculate the actual PF queues that are configured.
> > -	 */
> > -	if (pf->dev_data->dev_conf.rxmode.mq_mode &
> > ETH_MQ_RX_VMDQ_FLAG)
> > -		num = i40e_pf_calc_configured_queues_num(pf);
> > -	else
> > -		num = pf->dev_data->nb_rx_queues;
> > -
> > -	num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
> > -	PMD_INIT_LOG(INFO, "Max of contiguous %u PF queues are
> > configured",
> > -			num);
> > -
> > -	if (num == 0) {
> > -		PMD_INIT_LOG(ERR,
> > -			"No PF queues are configured to enable RSS for port
> > %u",
> > -			pf->dev_data->port_id);
> > -		return -ENOTSUP;
> > -	}
> > -
> > -	if (pf->adapter->rss_reta_updated == 0) {
> > -		for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> > -			if (j == num)
> > -				j = 0;
> > -			lut = (lut << 8) | (j & ((0x1 <<
> > -				hw->func_caps.rss_table_entry_width) - 1));
> > -			if ((i & 3) == 3)
> > -				I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >>
> > 2),
> > -					       rte_bswap32(lut));
> > -		}
> > -	}
> >
> >  	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
> >  	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) { @@ -
> > 9195,12 +9160,60 @@ i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
> >  	return ret;
> >  }
> >
> > +/* Initialize the hash look up table */ static int
> > +i40e_pf_init_rss_lut(struct i40e_pf *pf) {
> > +	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> > +	uint32_t lut = 0;
> > +	uint16_t j, num;
> > +	uint32_t i;
> > +
> > +	/* If both VMDQ and RSS enabled, not all of PF queues are
> > configured.
> > +	 * It's necessary to calculate the actual PF queues that are configured.
> > +	 */
> > +	if (pf->dev_data->dev_conf.rxmode.mq_mode &
> > ETH_MQ_RX_VMDQ_FLAG)
> > +		num = i40e_pf_calc_configured_queues_num(pf);
> > +	else
> > +		num = pf->dev_data->nb_rx_queues;
> > +
> > +	num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
> > +	PMD_INIT_LOG(INFO, "Max of contiguous %u PF queues are
> > configured",
> > +		     num);
> > +
> > +	if (num == 0) {
> > +		PMD_INIT_LOG(ERR,
> > +			"No PF queues are configured to enable RSS for port
> > %u",
> > +			pf->dev_data->port_id);
> > +		return -ENOTSUP;
> > +	}
> > +
> > +	if (pf->adapter->rss_reta_updated == 0) {
> > +		for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> > +			if (j == num)
> > +				j = 0;
> > +			lut = (lut << 8) | (j & ((0x1 <<
> > +				hw->func_caps.rss_table_entry_width) - 1));
> > +			if ((i & 3) == 3)
> > +				I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >>
> > 2),
> > +					       rte_bswap32(lut));
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int
> >  i40e_pf_config_mq_rx(struct i40e_pf *pf)  {
> > -	int ret = 0;
> > +	int ret;
> >  	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data-
> > >dev_conf.rxmode.mq_mode;
> >
> > +	/* Initialize hash look up table */
> > +	ret = i40e_pf_init_rss_lut(pf);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* RSS setup */
> >  	if (mq_mode & ETH_MQ_RX_RSS_FLAG)

I agree with Qiming, if we move this check into i40e_pf_config_rss, dose that looks we don't need to create a new function?

> >  		ret = i40e_pf_config_rss(pf);
> > --
> > 2.17.1
Shougang Wang July 24, 2020, 2:34 a.m. UTC | #3 
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Thursday, July 23, 2020 8:53 PM
> To: Yang, Qiming <qiming.yang@intel.com>; 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 v3] net/i40e: fix incorrect hash look up table
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Yang, Qiming
> > Sent: Thursday, July 23, 2020 9:57 AM
> > 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 v3] net/i40e: fix incorrect hash look
> > up table
> >
> > I don't understand why you add new function and new function mostly do
> > the same thing.
> > Why don't add fix in original code.
> >
<snip>
> > >  static int
> > >  i40e_pf_config_mq_rx(struct i40e_pf *pf)  {
> > > -	int ret = 0;
> > > +	int ret;
> > >  	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data-
> > > >dev_conf.rxmode.mq_mode;
> > >
> > > +	/* Initialize hash look up table */
> > > +	ret = i40e_pf_init_rss_lut(pf);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	/* RSS setup */
> > >  	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
> 
> I agree with Qiming, if we move this check into i40e_pf_config_rss, dose that
> looks we don't need to create a new function?
Indeed, I will do like this.
> 
> > >  		ret = i40e_pf_config_rss(pf);
> > > --
> > > 2.17.1
>
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 05d5f2861..e35590d96 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -8984,42 +8984,7 @@  i40e_pf_calc_configured_queues_num(struct i40e_pf *pf)
 static int
 i40e_pf_config_rss(struct i40e_pf *pf)
 {
-	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
 	struct rte_eth_rss_conf rss_conf;
-	uint32_t i, lut = 0;
-	uint16_t j, num;
-
-	/*
-	 * If both VMDQ and RSS enabled, not all of PF queues are configured.
-	 * It's necessary to calculate the actual PF queues that are configured.
-	 */
-	if (pf->dev_data->dev_conf.rxmode.mq_mode & ETH_MQ_RX_VMDQ_FLAG)
-		num = i40e_pf_calc_configured_queues_num(pf);
-	else
-		num = pf->dev_data->nb_rx_queues;
-
-	num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
-	PMD_INIT_LOG(INFO, "Max of contiguous %u PF queues are configured",
-			num);
-
-	if (num == 0) {
-		PMD_INIT_LOG(ERR,
-			"No PF queues are configured to enable RSS for port %u",
-			pf->dev_data->port_id);
-		return -ENOTSUP;
-	}
-
-	if (pf->adapter->rss_reta_updated == 0) {
-		for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
-			if (j == num)
-				j = 0;
-			lut = (lut << 8) | (j & ((0x1 <<
-				hw->func_caps.rss_table_entry_width) - 1));
-			if ((i & 3) == 3)
-				I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2),
-					       rte_bswap32(lut));
-		}
-	}
 
 	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
 	if ((rss_conf.rss_hf & pf->adapter->flow_types_mask) == 0) {
@@ -9195,12 +9160,60 @@  i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
 	return ret;
 }
 
+/* Initialize the hash look up table */
+static int
+i40e_pf_init_rss_lut(struct i40e_pf *pf)
+{
+	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	uint32_t lut = 0;
+	uint16_t j, num;
+	uint32_t i;
+
+	/* If both VMDQ and RSS enabled, not all of PF queues are configured.
+	 * It's necessary to calculate the actual PF queues that are configured.
+	 */
+	if (pf->dev_data->dev_conf.rxmode.mq_mode & ETH_MQ_RX_VMDQ_FLAG)
+		num = i40e_pf_calc_configured_queues_num(pf);
+	else
+		num = pf->dev_data->nb_rx_queues;
+
+	num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
+	PMD_INIT_LOG(INFO, "Max of contiguous %u PF queues are configured",
+		     num);
+
+	if (num == 0) {
+		PMD_INIT_LOG(ERR,
+			"No PF queues are configured to enable RSS for port %u",
+			pf->dev_data->port_id);
+		return -ENOTSUP;
+	}
+
+	if (pf->adapter->rss_reta_updated == 0) {
+		for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
+			if (j == num)
+				j = 0;
+			lut = (lut << 8) | (j & ((0x1 <<
+				hw->func_caps.rss_table_entry_width) - 1));
+			if ((i & 3) == 3)
+				I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2),
+					       rte_bswap32(lut));
+		}
+	}
+
+	return 0;
+}
+
 static int
 i40e_pf_config_mq_rx(struct i40e_pf *pf)
 {
-	int ret = 0;
+	int ret;
 	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
 
+	/* Initialize hash look up table */
+	ret = i40e_pf_init_rss_lut(pf);
+	if (ret)
+		return ret;
+
 	/* RSS setup */
 	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
 		ret = i40e_pf_config_rss(pf);