diff mbox series

net/i40e: fix incorrect hash look up table

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

Checks

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

Commit Message

Shougang Wang July 15, 2020, 6:35 a.m. UTC 
  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(+)

Comments

Chen, BoX C July 15, 2020, 9:07 a.m. UTC | #1 
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
Guo, Jia July 18, 2020, 4:07 a.m. UTC | #2 
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
diff mbox series

Patch

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