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

  The hash look up table(LUT) will not be initializing when starting
testpmd with --disable-rss. So that some invalid queue indexes may still
in the LUT. When enable RSS by creating RSS rule, some packets will not
be into the valid queues.
This patch fixes this issue by initializing the LUT when creating an RSS
rule.

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

Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 134 ++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 71 deletions(-)
  

Tested-by: Zhang, XiX <xix.zhang@intel.com>

Regards,
Xie Wei


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shougang Wang
Sent: Tuesday, July 21, 2020 1:49 PM
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 v2] net/i40e: fix incorrect hash look up table

The hash look up table(LUT) will not be initializing when starting testpmd with --disable-rss. So that some invalid queue indexes may still in the LUT. When enable RSS by creating RSS rule, some packets will not be into the valid queues.
This patch fixes this issue by initializing the LUT when creating an RSS rule.

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

Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 134 ++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 71 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 393b5320f..e56543393 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -13070,6 +13070,55 @@ i40e_rss_conf_init(struct i40e_rte_flow_rss_conf *out,
 	return 0;
 }
 
+/* If conf is NULL, function will init hash LUT with default 
+configration*/ static int i40e_rss_set_lut(struct i40e_pf *pf,
+		 struct i40e_rte_flow_rss_conf *conf) {
+	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;
+
+	if (conf == NULL)
+		num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
+	else
+		num = RTE_MIN(num, conf->conf.queue_num);
+	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
+			num);
+
+	if (num == 0) {
+		PMD_DRV_LOG(ERR,
+			"No PF queues are configured to enable RSS for port %u",
+			pf->dev_data->port_id);
+		return -ENOTSUP;
+	}
+
+	/* Fill in redirection table */
+	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
+		if (j == num)
+			j = 0;
+		if (conf == NULL)
+			lut = (lut << 8) | (j & ((0x1 <<
+				hw->func_caps.rss_table_entry_width) - 1));
+		else
+			lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
+			hw->func_caps.rss_table_entry_width) - 1));
+		if ((i & 3) == 3)
+			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
+	}
+
+	return 0;
+}
+
 /* Write HENA register to enable hash */  static int  i40e_rss_hash_set(struct i40e_pf *pf, struct i40e_rte_flow_rss_conf *rss_conf) @@ -13318,12 +13367,24 @@ static int  i40e_rss_enable_hash(struct i40e_pf *pf,
 		struct i40e_rte_flow_rss_conf *conf)
 {
+	enum rte_eth_rx_mq_mode mq_mode = 
+pf->dev_data->dev_conf.rxmode.mq_mode;
 	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
 	struct i40e_rte_flow_rss_conf rss_conf;
+	int ret;
 
 	if (!(conf->conf.types & pf->adapter->flow_types_mask))
 		return -ENOTSUP;
 
+	/* If the RSS is disabled before this, the LUT is uninitialized. 
+	 * So it is necessary to initialize it here.
+	 */
+	if (!(mq_mode & ETH_MQ_RX_RSS_FLAG) && !pf->rss_info.conf.queue_num &&
+	    !pf->adapter->rss_reta_updated) {
+		ret = i40e_rss_set_lut(pf, NULL);
+		if (ret)
+			return ret;
+	}
+
 	memset(&rss_conf, 0, sizeof(rss_conf));
 	rte_memcpy(&rss_conf, conf, sizeof(rss_conf));
 
@@ -13362,39 +13423,7 @@ static int
 i40e_rss_config_queue_region(struct i40e_pf *pf,
 		struct i40e_rte_flow_rss_conf *conf)
 {
-	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, conf->conf.queue_num);
-	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
-			num);
-
-	if (num == 0) {
-		PMD_DRV_LOG(ERR,
-			"No PF queues are configured to enable RSS for port %u",
-			pf->dev_data->port_id);
-		return -ENOTSUP;
-	}
-
-	/* Fill in redirection table */
-	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
-		if (j == num)
-			j = 0;
-		lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
-			hw->func_caps.rss_table_entry_width) - 1));
-		if ((i & 3) == 3)
-			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
-	}
+	i40e_rss_set_lut(pf, conf);
 
 	i40e_rss_mark_invalid_rule(pf, conf);
 
@@ -13491,46 +13520,9 @@ i40e_rss_disable_hash(struct i40e_pf *pf,  static int  i40e_rss_clear_queue_region(struct i40e_pf *pf)  {
-	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
 	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
-	uint16_t queue[I40E_MAX_Q_PER_TC];
-	uint32_t num_rxq, i;
-	uint32_t lut = 0;
-	uint16_t j, num;
-
-	num_rxq = RTE_MIN(pf->dev_data->nb_rx_queues, I40E_MAX_Q_PER_TC);
 
-	for (j = 0; j < num_rxq; j++)
-		queue[j] = j;
-
-	/* 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, num_rxq);
-	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
-			num);
-
-	if (num == 0) {
-		PMD_DRV_LOG(ERR,
-			"No PF queues are configured to enable RSS for port %u",
-			pf->dev_data->port_id);
-		return -ENOTSUP;
-	}
-
-	/* Fill in redirection table */
-	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
-		if (j == num)
-			j = 0;
-		lut = (lut << 8) | (queue[j] & ((0x1 <<
-			hw->func_caps.rss_table_entry_width) - 1));
-		if ((i & 3) == 3)
-			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
-	}
+	i40e_rss_set_lut(pf, NULL);
 
 	rss_info->conf.queue_num = 0;
 	memset(&rss_info->conf.queue, 0, sizeof(uint16_t));
--
2.17.1
  

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
> Sent: Tuesday, July 21, 2020 1:49 PM
> 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 v2] net/i40e: fix incorrect hash look up table
> 
> The hash look up table(LUT) will not be initializing when starting testpmd with
> --disable-rss. So that some invalid queue indexes may still in the LUT. When
> enable RSS by creating RSS rule, some packets will not be into the valid queues.
> This patch fixes this issue by initializing the LUT when creating an RSS rule.

Could you explain why you only initialize the LUT when creating an RSS rule but not at dev_init or dev_start?
What if user configure LUT table before create a RSS rule? Does that mean the LUT table will be flushed?

> 
> Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS flow")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 134 ++++++++++++++++-----------------
>  1 file changed, 63 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 393b5320f..e56543393 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -13070,6 +13070,55 @@ i40e_rss_conf_init(struct i40e_rte_flow_rss_conf
> *out,
>  	return 0;
>  }
> 
> +/* If conf is NULL, function will init hash LUT with default
> +configration*/ static int i40e_rss_set_lut(struct i40e_pf *pf,
> +		 struct i40e_rte_flow_rss_conf *conf) {
> +	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;
> +
> +	if (conf == NULL)
> +		num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
> +	else
> +		num = RTE_MIN(num, conf->conf.queue_num);
> +	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
> +			num);
> +
> +	if (num == 0) {
> +		PMD_DRV_LOG(ERR,
> +			"No PF queues are configured to enable RSS for port %u",
> +			pf->dev_data->port_id);
> +		return -ENOTSUP;
> +	}
> +
> +	/* Fill in redirection table */
> +	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> +		if (j == num)
> +			j = 0;
> +		if (conf == NULL)
> +			lut = (lut << 8) | (j & ((0x1 <<
> +				hw->func_caps.rss_table_entry_width) - 1));
> +		else
> +			lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
> +			hw->func_caps.rss_table_entry_width) - 1));
> +		if ((i & 3) == 3)
> +			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> +	}
> +
> +	return 0;
> +}
> +
>  /* Write HENA register to enable hash */  static int
> i40e_rss_hash_set(struct i40e_pf *pf, struct i40e_rte_flow_rss_conf *rss_conf)
> @@ -13318,12 +13367,24 @@ static int  i40e_rss_enable_hash(struct i40e_pf
> *pf,
>  		struct i40e_rte_flow_rss_conf *conf)
>  {
> +	enum rte_eth_rx_mq_mode mq_mode =
> +pf->dev_data->dev_conf.rxmode.mq_mode;
>  	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
>  	struct i40e_rte_flow_rss_conf rss_conf;
> +	int ret;
> 
>  	if (!(conf->conf.types & pf->adapter->flow_types_mask))
>  		return -ENOTSUP;
> 
> +	/* If the RSS is disabled before this, the LUT is uninitialized.
> +	 * So it is necessary to initialize it here.
> +	 */
> +	if (!(mq_mode & ETH_MQ_RX_RSS_FLAG)
> && !pf->rss_info.conf.queue_num &&
> +	    !pf->adapter->rss_reta_updated) {
> +		ret = i40e_rss_set_lut(pf, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	memset(&rss_conf, 0, sizeof(rss_conf));
>  	rte_memcpy(&rss_conf, conf, sizeof(rss_conf));
> 
> @@ -13362,39 +13423,7 @@ static int
>  i40e_rss_config_queue_region(struct i40e_pf *pf,
>  		struct i40e_rte_flow_rss_conf *conf)
>  {
> -	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, conf->conf.queue_num);
> -	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
> -			num);
> -
> -	if (num == 0) {
> -		PMD_DRV_LOG(ERR,
> -			"No PF queues are configured to enable RSS for port %u",
> -			pf->dev_data->port_id);
> -		return -ENOTSUP;
> -	}
> -
> -	/* Fill in redirection table */
> -	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> -		if (j == num)
> -			j = 0;
> -		lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
> -			hw->func_caps.rss_table_entry_width) - 1));
> -		if ((i & 3) == 3)
> -			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> -	}
> +	i40e_rss_set_lut(pf, conf);
> 
>  	i40e_rss_mark_invalid_rule(pf, conf);
> 
> @@ -13491,46 +13520,9 @@ i40e_rss_disable_hash(struct i40e_pf *pf,  static
> int  i40e_rss_clear_queue_region(struct i40e_pf *pf)  {
> -	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
>  	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
> -	uint16_t queue[I40E_MAX_Q_PER_TC];
> -	uint32_t num_rxq, i;
> -	uint32_t lut = 0;
> -	uint16_t j, num;
> -
> -	num_rxq = RTE_MIN(pf->dev_data->nb_rx_queues,
> I40E_MAX_Q_PER_TC);
> 
> -	for (j = 0; j < num_rxq; j++)
> -		queue[j] = j;
> -
> -	/* 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, num_rxq);
> -	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
> -			num);
> -
> -	if (num == 0) {
> -		PMD_DRV_LOG(ERR,
> -			"No PF queues are configured to enable RSS for port %u",
> -			pf->dev_data->port_id);
> -		return -ENOTSUP;
> -	}
> -
> -	/* Fill in redirection table */
> -	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> -		if (j == num)
> -			j = 0;
> -		lut = (lut << 8) | (queue[j] & ((0x1 <<
> -			hw->func_caps.rss_table_entry_width) - 1));
> -		if ((i & 3) == 3)
> -			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> -	}
> +	i40e_rss_set_lut(pf, NULL);
> 
>  	rss_info->conf.queue_num = 0;
>  	memset(&rss_info->conf.queue, 0, sizeof(uint16_t));
> --
> 2.17.1
  

hi, shougang

On 7/21/2020 2:41 PM, Xie, WeiX wrote:
> Tested-by: Zhang, XiX <xix.zhang@intel.com>
>
> Regards,
> Xie Wei
>
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shougang Wang
> Sent: Tuesday, July 21, 2020 1:49 PM
> 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 v2] net/i40e: fix incorrect hash look up table
>
> The hash look up table(LUT) will not be initializing when starting testpmd with --disable-rss. So that some invalid queue indexes may still in the LUT. When enable RSS by creating RSS rule, some packets will not be into the valid queues.
> This patch fixes this issue by initializing the LUT when creating an RSS rule.
>
> Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS flow")
> Cc: stable@dpdk.org
>
> Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> ---
>   drivers/net/i40e/i40e_ethdev.c | 134 ++++++++++++++++-----------------
>   1 file changed, 63 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 393b5320f..e56543393 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -13070,6 +13070,55 @@ i40e_rss_conf_init(struct i40e_rte_flow_rss_conf *out,
>   	return 0;
>   }
>   
> +/* If conf is NULL, function will init hash LUT with default
> +configration*/


Please fix the checkpatch issue here.


> static int i40e_rss_set_lut(struct i40e_pf *pf,


In order to eliminate any confuse with current i40e_set_rss_lut, please 
change the name, such as "i40e_rss_lut_init" or other better naming.


> +		 struct i40e_rte_flow_rss_conf *conf) {
> +	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;
> +
> +	if (conf == NULL)
> +		num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
> +	else
> +		num = RTE_MIN(num, conf->conf.queue_num);
> +	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
> +			num);


Alignment should match open parenthesis.


> +
> +	if (num == 0) {
> +		PMD_DRV_LOG(ERR,
> +			"No PF queues are configured to enable RSS for port %u",
> +			pf->dev_data->port_id);
> +		return -ENOTSUP;
> +	}
> +
> +	/* Fill in redirection table */
> +	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> +		if (j == num)
> +			j = 0;
> +		if (conf == NULL)
> +			lut = (lut << 8) | (j & ((0x1 <<
> +				hw->func_caps.rss_table_entry_width) - 1));
> +		else
> +			lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
> +			hw->func_caps.rss_table_entry_width) - 1));
> +		if ((i & 3) == 3)
> +			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> +	}
> +
> +	return 0;
> +}
> +
>   /* Write HENA register to enable hash */  static int  i40e_rss_hash_set(struct i40e_pf *pf, struct i40e_rte_flow_rss_conf *rss_conf) @@ -13318,12 +13367,24 @@ static int  i40e_rss_enable_hash(struct i40e_pf *pf,
>   		struct i40e_rte_flow_rss_conf *conf)
>   {
> +	enum rte_eth_rx_mq_mode mq_mode =
> +pf->dev_data->dev_conf.rxmode.mq_mode;
>   	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
>   	struct i40e_rte_flow_rss_conf rss_conf;
> +	int ret;


Suggest set the 0 to ret and return ret at the end of this function, so 
ret could be use in all part.


>   
>   	if (!(conf->conf.types & pf->adapter->flow_types_mask))
>   		return -ENOTSUP;
>   
> +	/* If the RSS is disabled before this, the LUT is uninitialized.
> +	 * So it is necessary to initialize it here.
> +	 */
> +	if (!(mq_mode & ETH_MQ_RX_RSS_FLAG) && !pf->rss_info.conf.queue_num &&
> +	    !pf->adapter->rss_reta_updated) {
> +		ret = i40e_rss_set_lut(pf, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	memset(&rss_conf, 0, sizeof(rss_conf));
>   	rte_memcpy(&rss_conf, conf, sizeof(rss_conf));
>   
> @@ -13362,39 +13423,7 @@ static int
>   i40e_rss_config_queue_region(struct i40e_pf *pf,
>   		struct i40e_rte_flow_rss_conf *conf)
>   {
> -	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, conf->conf.queue_num);
> -	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
> -			num);
> -
> -	if (num == 0) {
> -		PMD_DRV_LOG(ERR,
> -			"No PF queues are configured to enable RSS for port %u",
> -			pf->dev_data->port_id);
> -		return -ENOTSUP;
> -	}
> -
> -	/* Fill in redirection table */
> -	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> -		if (j == num)
> -			j = 0;
> -		lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
> -			hw->func_caps.rss_table_entry_width) - 1));
> -		if ((i & 3) == 3)
> -			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> -	}
> +	i40e_rss_set_lut(pf, conf);
>   
>   	i40e_rss_mark_invalid_rule(pf, conf);
>   
> @@ -13491,46 +13520,9 @@ i40e_rss_disable_hash(struct i40e_pf *pf,  static int  i40e_rss_clear_queue_region(struct i40e_pf *pf)  {
> -	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
>   	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
> -	uint16_t queue[I40E_MAX_Q_PER_TC];
> -	uint32_t num_rxq, i;
> -	uint32_t lut = 0;
> -	uint16_t j, num;
> -
> -	num_rxq = RTE_MIN(pf->dev_data->nb_rx_queues, I40E_MAX_Q_PER_TC);
>   
> -	for (j = 0; j < num_rxq; j++)
> -		queue[j] = j;
> -
> -	/* 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, num_rxq);
> -	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are configured",
> -			num);
> -
> -	if (num == 0) {
> -		PMD_DRV_LOG(ERR,
> -			"No PF queues are configured to enable RSS for port %u",
> -			pf->dev_data->port_id);
> -		return -ENOTSUP;
> -	}
> -
> -	/* Fill in redirection table */
> -	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> -		if (j == num)
> -			j = 0;
> -		lut = (lut << 8) | (queue[j] & ((0x1 <<
> -			hw->func_caps.rss_table_entry_width) - 1));
> -		if ((i & 3) == 3)
> -			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> -	}
> +	i40e_rss_set_lut(pf, NULL);


Need to check return value.


>   
>   	rss_info->conf.queue_num = 0;
>   	memset(&rss_info->conf.queue, 0, sizeof(uint16_t));
> --
> 2.17.1
>
  

Hi, Qi

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Wednesday, July 22, 2020 1:31 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 v2] net/i40e: fix incorrect hash look up table
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
> > Sent: Tuesday, July 21, 2020 1:49 PM
> > 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 v2] net/i40e: fix incorrect hash look up
> > table
> >
> > The hash look up table(LUT) will not be initializing when starting
> > testpmd with --disable-rss. So that some invalid queue indexes may
> > still in the LUT. When enable RSS by creating RSS rule, some packets will not
> be into the valid queues.
> > This patch fixes this issue by initializing the LUT when creating an RSS rule.
> 
> Could you explain why you only initialize the LUT when creating an RSS rule
> but not at dev_init or dev_start?
It is good to initialize the LUT at dev_start, I will fix it.

> What if user configure LUT table before create a RSS rule? Does that mean
> the LUT table will be flushed?
Yes.

Thanks.
Shougang
  

Hi, Jeff

> -----Original Message-----
> From: Guo, Jia <jia.guo@intel.com>
> Sent: Wednesday, July 22, 2020 1:51 PM
> To: Xie, WeiX <weix.xie@intel.com>; Wang, ShougangX
> <shougangx.wang@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect hash look up table
> 
> hi, shougang
> 
> On 7/21/2020 2:41 PM, Xie, WeiX wrote:
> > Tested-by: Zhang, XiX <xix.zhang@intel.com>
> >
> > Regards,
> > Xie Wei
> >
> >
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shougang Wang
> > Sent: Tuesday, July 21, 2020 1:49 PM
> > 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 v2] net/i40e: fix incorrect hash look up
> > table
> >
> > The hash look up table(LUT) will not be initializing when starting testpmd
> with --disable-rss. So that some invalid queue indexes may still in the LUT.
> When enable RSS by creating RSS rule, some packets will not be into the valid
> queues.
> > This patch fixes this issue by initializing the LUT when creating an RSS rule.
> >
> > Fixes: feaae285b342 ("net/i40e: support hash configuration in RSS
> > flow")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> > ---
> >   drivers/net/i40e/i40e_ethdev.c | 134 ++++++++++++++++-----------------
> >   1 file changed, 63 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 393b5320f..e56543393 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -13070,6 +13070,55 @@ i40e_rss_conf_init(struct
> i40e_rte_flow_rss_conf *out,
> >   	return 0;
> >   }
> >
> > +/* If conf is NULL, function will init hash LUT with default
> > +configration*/
> 
> 
> Please fix the checkpatch issue here.
> 
> 
> > static int i40e_rss_set_lut(struct i40e_pf *pf,
> 
> 
> In order to eliminate any confuse with current i40e_set_rss_lut, please
> change the name, such as "i40e_rss_lut_init" or other better naming.
> 
Qi also gave me some comments. I will define i40e_rss_lut_init() to initialize the LUT.

> 
> > +		 struct i40e_rte_flow_rss_conf *conf) {
> > +	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;
> > +
> > +	if (conf == NULL)
> > +		num = RTE_MIN(num, I40E_MAX_Q_PER_TC);
> > +	else
> > +		num = RTE_MIN(num, conf->conf.queue_num);
> > +	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are
> configured",
> > +			num);
> 
> 
> Alignment should match open parenthesis.
> 
> 
> > +
> > +	if (num == 0) {
> > +		PMD_DRV_LOG(ERR,
> > +			"No PF queues are configured to enable RSS for
> port %u",
> > +			pf->dev_data->port_id);
> > +		return -ENOTSUP;
> > +	}
> > +
> > +	/* Fill in redirection table */
> > +	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> > +		if (j == num)
> > +			j = 0;
> > +		if (conf == NULL)
> > +			lut = (lut << 8) | (j & ((0x1 <<
> > +				hw->func_caps.rss_table_entry_width) - 1));
> > +		else
> > +			lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
> > +			hw->func_caps.rss_table_entry_width) - 1));
> > +		if ((i & 3) == 3)
> > +			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   /* Write HENA register to enable hash */  static int
> i40e_rss_hash_set(struct i40e_pf *pf, struct i40e_rte_flow_rss_conf
> *rss_conf) @@ -13318,12 +13367,24 @@ static int
> i40e_rss_enable_hash(struct i40e_pf *pf,
> >   		struct i40e_rte_flow_rss_conf *conf)
> >   {
> > +	enum rte_eth_rx_mq_mode mq_mode =
> > +pf->dev_data->dev_conf.rxmode.mq_mode;
> >   	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
> >   	struct i40e_rte_flow_rss_conf rss_conf;
> > +	int ret;
> 
> 
> Suggest set the 0 to ret and return ret at the end of this function, so
> ret could be use in all part.
> 
> 
> >
> >   	if (!(conf->conf.types & pf->adapter->flow_types_mask))
> >   		return -ENOTSUP;
> >
> > +	/* If the RSS is disabled before this, the LUT is uninitialized.
> > +	 * So it is necessary to initialize it here.
> > +	 */
> > +	if (!(mq_mode & ETH_MQ_RX_RSS_FLAG) && !pf-
> >rss_info.conf.queue_num &&
> > +	    !pf->adapter->rss_reta_updated) {
> > +		ret = i40e_rss_set_lut(pf, NULL);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >   	memset(&rss_conf, 0, sizeof(rss_conf));
> >   	rte_memcpy(&rss_conf, conf, sizeof(rss_conf));
> >
> > @@ -13362,39 +13423,7 @@ static int
> >   i40e_rss_config_queue_region(struct i40e_pf *pf,
> >   		struct i40e_rte_flow_rss_conf *conf)
> >   {
> > -	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, conf->conf.queue_num);
> > -	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are
> configured",
> > -			num);
> > -
> > -	if (num == 0) {
> > -		PMD_DRV_LOG(ERR,
> > -			"No PF queues are configured to enable RSS for
> port %u",
> > -			pf->dev_data->port_id);
> > -		return -ENOTSUP;
> > -	}
> > -
> > -	/* Fill in redirection table */
> > -	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> > -		if (j == num)
> > -			j = 0;
> > -		lut = (lut << 8) | (conf->conf.queue[j] & ((0x1 <<
> > -			hw->func_caps.rss_table_entry_width) - 1));
> > -		if ((i & 3) == 3)
> > -			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> > -	}
> > +	i40e_rss_set_lut(pf, conf);
> >
> >   	i40e_rss_mark_invalid_rule(pf, conf);
> >
> > @@ -13491,46 +13520,9 @@ i40e_rss_disable_hash(struct i40e_pf *pf,
> static int  i40e_rss_clear_queue_region(struct i40e_pf *pf)  {
> > -	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> >   	struct i40e_rte_flow_rss_conf *rss_info = &pf->rss_info;
> > -	uint16_t queue[I40E_MAX_Q_PER_TC];
> > -	uint32_t num_rxq, i;
> > -	uint32_t lut = 0;
> > -	uint16_t j, num;
> > -
> > -	num_rxq = RTE_MIN(pf->dev_data->nb_rx_queues,
> I40E_MAX_Q_PER_TC);
> >
> > -	for (j = 0; j < num_rxq; j++)
> > -		queue[j] = j;
> > -
> > -	/* 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, num_rxq);
> > -	PMD_DRV_LOG(INFO, "Max of contiguous %u PF queues are
> configured",
> > -			num);
> > -
> > -	if (num == 0) {
> > -		PMD_DRV_LOG(ERR,
> > -			"No PF queues are configured to enable RSS for
> port %u",
> > -			pf->dev_data->port_id);
> > -		return -ENOTSUP;
> > -	}
> > -
> > -	/* Fill in redirection table */
> > -	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
> > -		if (j == num)
> > -			j = 0;
> > -		lut = (lut << 8) | (queue[j] & ((0x1 <<
> > -			hw->func_caps.rss_table_entry_width) - 1));
> > -		if ((i & 3) == 3)
> > -			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
> > -	}
> > +	i40e_rss_set_lut(pf, NULL);
> 
> 
> Need to check return value.
> 
> 
> >
> >   	rss_info->conf.queue_num = 0;
> >   	memset(&rss_info->conf.queue, 0, sizeof(uint16_t));
> > --
> > 2.17.1
> >