DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table
@ 2020-07-15  6:35 Shougang Wang
  2020-07-15  9:07 ` Chen, BoX C
                   ` (5 more replies)
  0 siblings, 6 replies; 23+ messages in thread
From: Shougang Wang @ 2020-07-15  6:35 UTC (permalink / raw)
  To: dev; +Cc: beilei.xing, jia.guo, Shougang Wang, stable

The hash look up table(LUT) will not be initializing when starting
testpmd with --disable-rss. So that some queues in LUT will be invalid
if rx queues is less than last time. 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 | 52 ++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 393b5320f..2a92bd8ef 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -13070,6 +13070,49 @@ i40e_rss_conf_init(struct i40e_rte_flow_rss_conf *out,
 	return 0;
 }
 
+static int
+i40e_rss_init_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;
+}
+
 /* 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 +13361,21 @@ 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;
 
 	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)
+		if (i40e_rss_init_lut(pf))
+			return -ENOTSUP;
+
 	memset(&rss_conf, 0, sizeof(rss_conf));
 	rte_memcpy(&rss_conf, conf, sizeof(rss_conf));
 
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table
  2020-07-15  6:35 [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table Shougang Wang
@ 2020-07-15  9:07 ` Chen, BoX C
  2020-07-18  4:07   ` Jeff Guo
  2020-07-21  5:49 ` [dpdk-dev] [PATCH v2] " Shougang Wang
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Chen, BoX C @ 2020-07-15  9:07 UTC (permalink / raw)
  To: Wang, ShougangX, dev; +Cc: Xing, Beilei, Guo, Jia, Wang, ShougangX, stable

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

Regards,
Chen Bo

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
> Sent: July 15, 2020 14:35
> 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] 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 queues in LUT will be invalid if rx queues is less
> than last time. 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 | 52
> ++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 393b5320f..2a92bd8ef 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -13070,6 +13070,49 @@ i40e_rss_conf_init(struct
> i40e_rte_flow_rss_conf *out,
>  	return 0;
>  }
> 
> +static int
> +i40e_rss_init_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;
> +}
> +
>  /* 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
> +13361,21 @@ 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;
> 
>  	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)
> +		if (i40e_rss_init_lut(pf))
> +			return -ENOTSUP;
> +
>  	memset(&rss_conf, 0, sizeof(rss_conf));
>  	rte_memcpy(&rss_conf, conf, sizeof(rss_conf));
> 
> --
> 2.17.1


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

* Re: [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table
  2020-07-15  9:07 ` Chen, BoX C
@ 2020-07-18  4:07   ` Jeff Guo
  0 siblings, 0 replies; 23+ messages in thread
From: Jeff Guo @ 2020-07-18  4:07 UTC (permalink / raw)
  To: Chen, BoX C, Wang, ShougangX, dev; +Cc: Xing, Beilei, stable

hi, shougang

On 7/15/2020 5:07 PM, Chen, BoX C wrote:
> Tested-by: zhang,xi <xix.zhang@intel.com>
>
> Regards,
> Chen Bo
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Shougang Wang
>> Sent: July 15, 2020 14:35
>> 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] 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 queues in LUT will be invalid if rx queues is less
>> than last time. When enable RSS by creating RSS rule, some packets will not
>> be into the valid queues.


What does "So that some queues in LUT will be invalid if rx queues is 
less than last time" means, Could you explain more clear?


>> 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 | 52
>> ++++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
>> index 393b5320f..2a92bd8ef 100644
>> --- a/drivers/net/i40e/i40e_ethdev.c
>> +++ b/drivers/net/i40e/i40e_ethdev.c
>> @@ -13070,6 +13070,49 @@ i40e_rss_conf_init(struct
>> i40e_rte_flow_rss_conf *out,
>>   return 0;
>>   }
>>
>> +static int
>> +i40e_rss_init_lut(struct i40e_pf *pf)


I saw that this function is almost the same functional as 
i40e_rss_config_queue_region, could you check if it could be reused and 
merged one.


>> +{
>> +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;
>> +}
>> +
>>   /* 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
>> +13361,21 @@ 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;
>>
>>   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.
>> + */


Please don't use an empty /* line, use /* Comment....


>> +if (!(mq_mode & ETH_MQ_RX_RSS_FLAG) && !pf-
>>> rss_info.conf.queue_num)
>> +if (i40e_rss_init_lut(pf))
>> +return -ENOTSUP;


How can it know it is not support in function's caller,  better to use 
variable to restore the calling return and return the variable.


>> +
>>   memset(&rss_conf, 0, sizeof(rss_conf));
>>   rte_memcpy(&rss_conf, conf, sizeof(rss_conf));
>>
>> --
>> 2.17.1

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

* [dpdk-dev] [PATCH v2] net/i40e: fix incorrect hash look up table
  2020-07-15  6:35 [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table Shougang Wang
  2020-07-15  9:07 ` Chen, BoX C
@ 2020-07-21  5:49 ` Shougang Wang
  2020-07-21  6:41   ` Xie, WeiX
  2020-07-22  5:31   ` Zhang, Qi Z
  2020-07-22  8:15 ` [dpdk-dev] [PATCH v3] " Shougang Wang
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 23+ messages in thread
From: Shougang Wang @ 2020-07-21  5:49 UTC (permalink / raw)
  To: dev; +Cc: beilei.xing, jia.guo, Shougang Wang, stable

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


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

* Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect hash look up table
  2020-07-21  5:49 ` [dpdk-dev] [PATCH v2] " Shougang Wang
@ 2020-07-21  6:41   ` Xie, WeiX
  2020-07-22  5:50     ` Jeff Guo
  2020-07-22  5:31   ` Zhang, Qi Z
  1 sibling, 1 reply; 23+ messages in thread
From: Xie, WeiX @ 2020-07-21  6:41 UTC (permalink / raw)
  To: Wang, ShougangX, dev; +Cc: Xing, Beilei, Guo, Jia, Wang, ShougangX, stable

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


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

* Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect hash look up table
  2020-07-21  5:49 ` [dpdk-dev] [PATCH v2] " Shougang Wang
  2020-07-21  6:41   ` Xie, WeiX
@ 2020-07-22  5:31   ` Zhang, Qi Z
  2020-07-22  6:20     ` Wang, ShougangX
  1 sibling, 1 reply; 23+ messages in thread
From: Zhang, Qi Z @ 2020-07-22  5:31 UTC (permalink / raw)
  To: Wang, ShougangX, dev; +Cc: Xing, Beilei, Guo, Jia, Wang, ShougangX, stable



> -----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


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

* Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect hash look up table
  2020-07-21  6:41   ` Xie, WeiX
@ 2020-07-22  5:50     ` Jeff Guo
  2020-07-22  6:39       ` Wang, ShougangX
  0 siblings, 1 reply; 23+ messages in thread
From: Jeff Guo @ 2020-07-22  5:50 UTC (permalink / raw)
  To: Xie, WeiX, Wang, ShougangX, dev; +Cc: Xing, Beilei, stable

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
>

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

* Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect hash look up table
  2020-07-22  5:31   ` Zhang, Qi Z
@ 2020-07-22  6:20     ` Wang, ShougangX
  0 siblings, 0 replies; 23+ messages in thread
From: Wang, ShougangX @ 2020-07-22  6:20 UTC (permalink / raw)
  To: Zhang, Qi Z, dev; +Cc: Xing, Beilei, Guo, Jia, stable, Di, ChenxuX

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

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

* Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect hash look up table
  2020-07-22  5:50     ` Jeff Guo
@ 2020-07-22  6:39       ` Wang, ShougangX
  0 siblings, 0 replies; 23+ messages in thread
From: Wang, ShougangX @ 2020-07-22  6:39 UTC (permalink / raw)
  To: Guo, Jia, Xie, WeiX, dev; +Cc: Xing, Beilei, stable, Di, ChenxuX

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
> >

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

* [dpdk-dev] [PATCH v3] net/i40e: fix incorrect hash look up table
  2020-07-15  6:35 [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table Shougang Wang
  2020-07-15  9:07 ` Chen, BoX C
  2020-07-21  5:49 ` [dpdk-dev] [PATCH v2] " Shougang Wang
@ 2020-07-22  8:15 ` Shougang Wang
  2020-07-23  1:57   ` Yang, Qiming
  2020-07-24  2:47 ` [dpdk-dev] [PATCH v4] " Shougang Wang
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Shougang Wang @ 2020-07-22  8:15 UTC (permalink / raw)
  To: dev; +Cc: beilei.xing, jia.guo, Shougang Wang, stable

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


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

* Re: [dpdk-dev] [PATCH v3] net/i40e: fix incorrect hash look up table
  2020-07-22  8:15 ` [dpdk-dev] [PATCH v3] " Shougang Wang
@ 2020-07-23  1:57   ` Yang, Qiming
  2020-07-23 12:53     ` Zhang, Qi Z
  0 siblings, 1 reply; 23+ messages in thread
From: Yang, Qiming @ 2020-07-23  1:57 UTC (permalink / raw)
  To: Wang, ShougangX, dev; +Cc: Xing, Beilei, Guo, Jia, Wang, ShougangX, stable

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


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

* Re: [dpdk-dev] [PATCH v3] net/i40e: fix incorrect hash look up table
  2020-07-23  1:57   ` Yang, Qiming
@ 2020-07-23 12:53     ` Zhang, Qi Z
  2020-07-24  2:34       ` Wang, ShougangX
  0 siblings, 1 reply; 23+ messages in thread
From: Zhang, Qi Z @ 2020-07-23 12:53 UTC (permalink / raw)
  To: Yang, Qiming, Wang, ShougangX, dev
  Cc: Xing, Beilei, Guo, Jia, Wang, ShougangX, stable



> -----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


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

* Re: [dpdk-dev] [PATCH v3] net/i40e: fix incorrect hash look up table
  2020-07-23 12:53     ` Zhang, Qi Z
@ 2020-07-24  2:34       ` Wang, ShougangX
  0 siblings, 0 replies; 23+ messages in thread
From: Wang, ShougangX @ 2020-07-24  2:34 UTC (permalink / raw)
  To: Zhang, Qi Z, Yang, Qiming, dev; +Cc: Xing, Beilei, Guo, Jia, stable


> -----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
> 


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

* [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up table
  2020-07-15  6:35 [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table Shougang Wang
                   ` (2 preceding siblings ...)
  2020-07-22  8:15 ` [dpdk-dev] [PATCH v3] " Shougang Wang
@ 2020-07-24  2:47 ` Shougang Wang
  2020-07-24  3:57   ` Jeff Guo
  2020-07-24  5:07   ` Yang, Qiming
  2020-07-24  8:12 ` [dpdk-dev] [PATCH v5] " Shougang Wang
  2020-07-24  9:38 ` [dpdk-dev] [PATCH v6] " Shougang Wang
  5 siblings, 2 replies; 23+ messages in thread
From: Shougang Wang @ 2020-07-24  2:47 UTC (permalink / raw)
  To: dev; +Cc: beilei.xing, jia.guo, Shougang Wang, stable

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;
 	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 */
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up table
  2020-07-24  2:47 ` [dpdk-dev] [PATCH v4] " Shougang Wang
@ 2020-07-24  3:57   ` Jeff Guo
  2020-07-24  4:49     ` Wang, ShougangX
  2020-07-24  5:07   ` Yang, Qiming
  1 sibling, 1 reply; 23+ messages in thread
From: Jeff Guo @ 2020-07-24  3:57 UTC (permalink / raw)
  To: Shougang Wang, dev; +Cc: beilei.xing, stable

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 */

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

* Re: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up table
  2020-07-24  3:57   ` Jeff Guo
@ 2020-07-24  4:49     ` Wang, ShougangX
  0 siblings, 0 replies; 23+ messages in thread
From: Wang, ShougangX @ 2020-07-24  4:49 UTC (permalink / raw)
  To: Guo, Jia, dev; +Cc: Xing, Beilei, stable

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.

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

* Re: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up table
  2020-07-24  2:47 ` [dpdk-dev] [PATCH v4] " Shougang Wang
  2020-07-24  3:57   ` Jeff Guo
@ 2020-07-24  5:07   ` Yang, Qiming
  2020-07-24  5:42     ` Wang, ShougangX
  1 sibling, 1 reply; 23+ messages in thread
From: Yang, Qiming @ 2020-07-24  5:07 UTC (permalink / raw)
  To: Wang, ShougangX, dev; +Cc: Xing, Beilei, Guo, Jia, Wang, ShougangX, stable

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


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

* Re: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect hash look up table
  2020-07-24  5:07   ` Yang, Qiming
@ 2020-07-24  5:42     ` Wang, ShougangX
  0 siblings, 0 replies; 23+ messages in thread
From: Wang, ShougangX @ 2020-07-24  5:42 UTC (permalink / raw)
  To: Yang, Qiming, dev; +Cc: Xing, Beilei, Guo, Jia, stable, Di, ChenxuX

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

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

* [dpdk-dev] [PATCH v5] net/i40e: fix incorrect hash look up table
  2020-07-15  6:35 [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table Shougang Wang
                   ` (3 preceding siblings ...)
  2020-07-24  2:47 ` [dpdk-dev] [PATCH v4] " Shougang Wang
@ 2020-07-24  8:12 ` Shougang Wang
  2020-07-24  9:00   ` Xie, WeiX
  2020-07-24  9:38 ` [dpdk-dev] [PATCH v6] " Shougang Wang
  5 siblings, 1 reply; 23+ messages in thread
From: Shougang Wang @ 2020-07-24  8:12 UTC (permalink / raw)
  To: dev; +Cc: beilei.xing, jia.guo, Yang Qiming, Shougang Wang, stable

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 whatever 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>
---
v5:
-Removed useless function
---
 drivers/net/i40e/i40e_ethdev.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 05d5f2861..6fa0a5c71 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -302,7 +302,6 @@ static int i40e_dev_init_vlan(struct rte_eth_dev *dev);
 static int i40e_veb_release(struct i40e_veb *veb);
 static struct i40e_veb *i40e_veb_setup(struct i40e_pf *pf,
 						struct i40e_vsi *vsi);
-static int i40e_pf_config_mq_rx(struct i40e_pf *pf);
 static int i40e_vsi_config_double_vlan(struct i40e_vsi *vsi, int on);
 static inline int i40e_find_all_mac_for_vlan(struct i40e_vsi *vsi,
 					     struct i40e_macvlan_filter *mv_f,
@@ -398,6 +397,7 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_pf_config_rss(struct i40e_pf *pf);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -1954,7 +1954,7 @@ i40e_dev_configure(struct rte_eth_dev *dev)
 		goto err;
 
 	/* VMDQ setup.
-	 *  Needs to move VMDQ setting out of i40e_pf_config_mq_rx() as VMDQ and
+	 *  Needs to move VMDQ setting out of i40e_pf_config_rss() as VMDQ and
 	 *  RSS setting have different requirements.
 	 *  General PMD driver call sequence are NIC init, configure,
 	 *  rx/tx_queue_setup and dev_start. In rx/tx_queue_setup() function, it
@@ -6478,7 +6478,7 @@ i40e_dev_rx_init(struct i40e_pf *pf)
 	uint16_t i;
 	struct i40e_rx_queue *rxq;
 
-	i40e_pf_config_mq_rx(pf);
+	i40e_pf_config_rss(pf);
 	for (i = 0; i < data->nb_rx_queues; i++) {
 		rxq = data->rx_queues[i];
 		if (!rxq || !rxq->q_set)
@@ -8984,6 +8984,7 @@ i40e_pf_calc_configured_queues_num(struct i40e_pf *pf)
 static int
 i40e_pf_config_rss(struct i40e_pf *pf)
 {
+	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
 	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
 	struct rte_eth_rss_conf rss_conf;
 	uint32_t i, lut = 0;
@@ -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;
 	}
@@ -9195,21 +9197,6 @@ i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
 	return ret;
 }
 
-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;
-}
-
 /* Get the symmetric hash enable configurations per port */
 static void
 i40e_get_symmetric_hash_enable_per_port(struct i40e_hw *hw, uint8_t *enable)
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix incorrect hash look up table
  2020-07-24  8:12 ` [dpdk-dev] [PATCH v5] " Shougang Wang
@ 2020-07-24  9:00   ` Xie, WeiX
  0 siblings, 0 replies; 23+ messages in thread
From: Xie, WeiX @ 2020-07-24  9:00 UTC (permalink / raw)
  To: Wang, ShougangX, dev
  Cc: Xing, Beilei, Guo, Jia, Yang, Qiming, Wang, ShougangX, stable

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: Friday, July 24, 2020 4:13 PM
To: dev@dpdk.org
Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Wang, ShougangX <shougangx.wang@intel.com>; stable@dpdk.org
Subject: [dpdk-dev] [PATCH v5] 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 whatever 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>
---
v5:
-Removed useless function
---
 drivers/net/i40e/i40e_ethdev.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 05d5f2861..6fa0a5c71 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -302,7 +302,6 @@ static int i40e_dev_init_vlan(struct rte_eth_dev *dev);  static int i40e_veb_release(struct i40e_veb *veb);  static struct i40e_veb *i40e_veb_setup(struct i40e_pf *pf,
 						struct i40e_vsi *vsi);
-static int i40e_pf_config_mq_rx(struct i40e_pf *pf);  static int i40e_vsi_config_double_vlan(struct i40e_vsi *vsi, int on);  static inline int i40e_find_all_mac_for_vlan(struct i40e_vsi *vsi,
 					     struct i40e_macvlan_filter *mv_f, @@ -398,6 +397,7 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);  static void i40e_tunnel_filter_restore(struct i40e_pf *pf);  static void i40e_filter_restore(struct i40e_pf *pf);  static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_pf_config_rss(struct i40e_pf *pf);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -1954,7 +1954,7 @@ i40e_dev_configure(struct rte_eth_dev *dev)
 		goto err;
 
 	/* VMDQ setup.
-	 *  Needs to move VMDQ setting out of i40e_pf_config_mq_rx() as VMDQ and
+	 *  Needs to move VMDQ setting out of i40e_pf_config_rss() as VMDQ and
 	 *  RSS setting have different requirements.
 	 *  General PMD driver call sequence are NIC init, configure,
 	 *  rx/tx_queue_setup and dev_start. In rx/tx_queue_setup() function, it @@ -6478,7 +6478,7 @@ i40e_dev_rx_init(struct i40e_pf *pf)
 	uint16_t i;
 	struct i40e_rx_queue *rxq;
 
-	i40e_pf_config_mq_rx(pf);
+	i40e_pf_config_rss(pf);
 	for (i = 0; i < data->nb_rx_queues; i++) {
 		rxq = data->rx_queues[i];
 		if (!rxq || !rxq->q_set)
@@ -8984,6 +8984,7 @@ i40e_pf_calc_configured_queues_num(struct i40e_pf *pf)  static int  i40e_pf_config_rss(struct i40e_pf *pf)  {
+	enum rte_eth_rx_mq_mode mq_mode = 
+pf->dev_data->dev_conf.rxmode.mq_mode;
 	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
 	struct rte_eth_rss_conf rss_conf;
 	uint32_t i, lut = 0;
@@ -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;
 	}
@@ -9195,21 +9197,6 @@ i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
 	return ret;
 }
 
-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;
-}
-
 /* Get the symmetric hash enable configurations per port */  static void  i40e_get_symmetric_hash_enable_per_port(struct i40e_hw *hw, uint8_t *enable)
--
2.17.1


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

* [dpdk-dev] [PATCH v6] net/i40e: fix incorrect hash look up table
  2020-07-15  6:35 [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table Shougang Wang
                   ` (4 preceding siblings ...)
  2020-07-24  8:12 ` [dpdk-dev] [PATCH v5] " Shougang Wang
@ 2020-07-24  9:38 ` Shougang Wang
  2020-07-24  9:54   ` Jeff Guo
  5 siblings, 1 reply; 23+ messages in thread
From: Shougang Wang @ 2020-07-24  9:38 UTC (permalink / raw)
  To: dev; +Cc: beilei.xing, jia.guo, Yang Qiming, Shougang Wang, stable

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 whatever 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>
Tested-by: Zhang, XiX <xix.zhang@intel.com>
---
v6:
-Remove useless comment.
 drivers/net/i40e/i40e_ethdev.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 05d5f2861..890f05d90 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -302,7 +302,6 @@ static int i40e_dev_init_vlan(struct rte_eth_dev *dev);
 static int i40e_veb_release(struct i40e_veb *veb);
 static struct i40e_veb *i40e_veb_setup(struct i40e_pf *pf,
 						struct i40e_vsi *vsi);
-static int i40e_pf_config_mq_rx(struct i40e_pf *pf);
 static int i40e_vsi_config_double_vlan(struct i40e_vsi *vsi, int on);
 static inline int i40e_find_all_mac_for_vlan(struct i40e_vsi *vsi,
 					     struct i40e_macvlan_filter *mv_f,
@@ -398,6 +397,7 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_pf_config_rss(struct i40e_pf *pf);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -1954,8 +1954,6 @@ i40e_dev_configure(struct rte_eth_dev *dev)
 		goto err;
 
 	/* VMDQ setup.
-	 *  Needs to move VMDQ setting out of i40e_pf_config_mq_rx() as VMDQ and
-	 *  RSS setting have different requirements.
 	 *  General PMD driver call sequence are NIC init, configure,
 	 *  rx/tx_queue_setup and dev_start. In rx/tx_queue_setup() function, it
 	 *  will try to lookup the VSI that specific queue belongs to if VMDQ
@@ -6478,7 +6476,7 @@ i40e_dev_rx_init(struct i40e_pf *pf)
 	uint16_t i;
 	struct i40e_rx_queue *rxq;
 
-	i40e_pf_config_mq_rx(pf);
+	i40e_pf_config_rss(pf);
 	for (i = 0; i < data->nb_rx_queues; i++) {
 		rxq = data->rx_queues[i];
 		if (!rxq || !rxq->q_set)
@@ -8984,6 +8982,7 @@ i40e_pf_calc_configured_queues_num(struct i40e_pf *pf)
 static int
 i40e_pf_config_rss(struct i40e_pf *pf)
 {
+	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
 	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
 	struct rte_eth_rss_conf rss_conf;
 	uint32_t i, lut = 0;
@@ -9022,7 +9021,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;
 	}
@@ -9195,21 +9195,6 @@ i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
 	return ret;
 }
 
-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;
-}
-
 /* Get the symmetric hash enable configurations per port */
 static void
 i40e_get_symmetric_hash_enable_per_port(struct i40e_hw *hw, uint8_t *enable)
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH v6] net/i40e: fix incorrect hash look up table
  2020-07-24  9:38 ` [dpdk-dev] [PATCH v6] " Shougang Wang
@ 2020-07-24  9:54   ` Jeff Guo
  2020-07-24  9:58     ` Zhang, Qi Z
  0 siblings, 1 reply; 23+ messages in thread
From: Jeff Guo @ 2020-07-24  9:54 UTC (permalink / raw)
  To: Shougang Wang, dev; +Cc: beilei.xing, Yang Qiming, stable

Acked-by: Jeff Guo <jia.guo@intel.com>

On 7/24/2020 5:38 PM, 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 whatever 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>
> Tested-by: Zhang, XiX <xix.zhang@intel.com>
> ---
> v6:
> -Remove useless comment.
>   drivers/net/i40e/i40e_ethdev.c | 25 +++++--------------------
>   1 file changed, 5 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 05d5f2861..890f05d90 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -302,7 +302,6 @@ static int i40e_dev_init_vlan(struct rte_eth_dev *dev);
>   static int i40e_veb_release(struct i40e_veb *veb);
>   static struct i40e_veb *i40e_veb_setup(struct i40e_pf *pf,
>   						struct i40e_vsi *vsi);
> -static int i40e_pf_config_mq_rx(struct i40e_pf *pf);
>   static int i40e_vsi_config_double_vlan(struct i40e_vsi *vsi, int on);
>   static inline int i40e_find_all_mac_for_vlan(struct i40e_vsi *vsi,
>   					     struct i40e_macvlan_filter *mv_f,
> @@ -398,6 +397,7 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
>   static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
>   static void i40e_filter_restore(struct i40e_pf *pf);
>   static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
> +static int i40e_pf_config_rss(struct i40e_pf *pf);
>   
>   static const char *const valid_keys[] = {
>   	ETH_I40E_FLOATING_VEB_ARG,
> @@ -1954,8 +1954,6 @@ i40e_dev_configure(struct rte_eth_dev *dev)
>   		goto err;
>   
>   	/* VMDQ setup.
> -	 *  Needs to move VMDQ setting out of i40e_pf_config_mq_rx() as VMDQ and
> -	 *  RSS setting have different requirements.
>   	 *  General PMD driver call sequence are NIC init, configure,
>   	 *  rx/tx_queue_setup and dev_start. In rx/tx_queue_setup() function, it
>   	 *  will try to lookup the VSI that specific queue belongs to if VMDQ
> @@ -6478,7 +6476,7 @@ i40e_dev_rx_init(struct i40e_pf *pf)
>   	uint16_t i;
>   	struct i40e_rx_queue *rxq;
>   
> -	i40e_pf_config_mq_rx(pf);
> +	i40e_pf_config_rss(pf);
>   	for (i = 0; i < data->nb_rx_queues; i++) {
>   		rxq = data->rx_queues[i];
>   		if (!rxq || !rxq->q_set)
> @@ -8984,6 +8982,7 @@ i40e_pf_calc_configured_queues_num(struct i40e_pf *pf)
>   static int
>   i40e_pf_config_rss(struct i40e_pf *pf)
>   {
> +	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
>   	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
>   	struct rte_eth_rss_conf rss_conf;
>   	uint32_t i, lut = 0;
> @@ -9022,7 +9021,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;
>   	}
> @@ -9195,21 +9195,6 @@ i40e_tunnel_filter_handle(struct rte_eth_dev *dev,
>   	return ret;
>   }
>   
> -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;
> -}
> -
>   /* Get the symmetric hash enable configurations per port */
>   static void
>   i40e_get_symmetric_hash_enable_per_port(struct i40e_hw *hw, uint8_t *enable)

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

* Re: [dpdk-dev] [PATCH v6] net/i40e: fix incorrect hash look up table
  2020-07-24  9:54   ` Jeff Guo
@ 2020-07-24  9:58     ` Zhang, Qi Z
  0 siblings, 0 replies; 23+ messages in thread
From: Zhang, Qi Z @ 2020-07-24  9:58 UTC (permalink / raw)
  To: Guo, Jia, Wang, ShougangX, dev; +Cc: Xing, Beilei, Yang, Qiming, stable



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
> Sent: Friday, July 24, 2020 5:55 PM
> To: Wang, ShougangX <shougangx.wang@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6] net/i40e: fix incorrect hash look up table
> 
> Acked-by: Jeff Guo <jia.guo@intel.com>
> 
> On 7/24/2020 5:38 PM, 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 whatever 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>
> > Tested-by: Zhang, XiX <xix.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

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

end of thread, other threads:[~2020-07-24  9:58 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15  6:35 [dpdk-dev] [PATCH] net/i40e: fix incorrect hash look up table Shougang Wang
2020-07-15  9:07 ` Chen, BoX C
2020-07-18  4:07   ` Jeff Guo
2020-07-21  5:49 ` [dpdk-dev] [PATCH v2] " Shougang Wang
2020-07-21  6:41   ` Xie, WeiX
2020-07-22  5:50     ` Jeff Guo
2020-07-22  6:39       ` Wang, ShougangX
2020-07-22  5:31   ` Zhang, Qi Z
2020-07-22  6:20     ` Wang, ShougangX
2020-07-22  8:15 ` [dpdk-dev] [PATCH v3] " Shougang Wang
2020-07-23  1:57   ` Yang, Qiming
2020-07-23 12:53     ` Zhang, Qi Z
2020-07-24  2:34       ` Wang, ShougangX
2020-07-24  2:47 ` [dpdk-dev] [PATCH v4] " Shougang Wang
2020-07-24  3:57   ` Jeff Guo
2020-07-24  4:49     ` Wang, ShougangX
2020-07-24  5:07   ` Yang, Qiming
2020-07-24  5:42     ` Wang, ShougangX
2020-07-24  8:12 ` [dpdk-dev] [PATCH v5] " Shougang Wang
2020-07-24  9:00   ` Xie, WeiX
2020-07-24  9:38 ` [dpdk-dev] [PATCH v6] " Shougang Wang
2020-07-24  9:54   ` Jeff Guo
2020-07-24  9:58     ` Zhang, Qi Z

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).