[dpdk-dev] [PATCH] net/qede: fix RSS table entries for 100G adapter

[dpdk-dev] [PATCH] net/qede: fix RSS table entries for 100G adapter

[Rasesh Mody]

With the change in base APIs the logic for 100G handling needs to be
adjusted to pass cid values instead for queue ids. The current API
works assuming its queue id.

Fixes: 69d7ba88f1a1 ("net/qede/base: use L2-handles for RSS configuration")

Signed-off-by: Rasesh Mody <rasesh.mody@cavium.com>
---
 drivers/net/qede/qede_eth_if.c |   27 -------------
 drivers/net/qede/qede_eth_if.h |    2 -
 drivers/net/qede/qede_ethdev.c |   85 ++++++++++++++++++++++++++++++++++------
 drivers/net/qede/qede_ethdev.h |    1 +
 4 files changed, 73 insertions(+), 42 deletions(-)

diff --git a/drivers/net/qede/qede_eth_if.c b/drivers/net/qede/qede_eth_if.c
index 86bb129..a3c0b13 100644
--- a/drivers/net/qede/qede_eth_if.c
+++ b/drivers/net/qede/qede_eth_if.c
@@ -67,33 +67,6 @@ static int qed_stop_vport(struct ecore_dev *edev, uint8_t vport_id)
 	return 0;
 }
 
-bool qed_update_rss_parm_cmt(struct ecore_dev *edev, uint16_t *p_tbl)
-{
-	uint16_t max = 0, k;
-	bool rss_mode = 0; /* disable */
-	int divisor;
-
-	/* Find largest entry, since it's possible RSS needs to
-	 * be disabled [in case only 1 queue per-hwfn]
-	 */
-	for (k = 0; k < ECORE_RSS_IND_TABLE_SIZE; k++)
-		max = (max > p_tbl[k]) ?  max : p_tbl[k];
-
-	/* Either fix RSS values or disable RSS */
-	if (edev->num_hwfns < max + 1) {
-		divisor = (max + edev->num_hwfns - 1) / edev->num_hwfns;
-		DP_VERBOSE(edev, ECORE_MSG_SPQ,
-			   "CMT - fixing RSS values (modulo %02x)\n",
-			   divisor);
-		for (k = 0; k < ECORE_RSS_IND_TABLE_SIZE; k++)
-			p_tbl[k] = p_tbl[k] % divisor;
-
-		rss_mode = 1;
-	}
-
-	return rss_mode;
-}
-
 static int
 qed_update_vport(struct ecore_dev *edev, struct qed_update_vport_params *params)
 {
diff --git a/drivers/net/qede/qede_eth_if.h b/drivers/net/qede/qede_eth_if.h
index d845bac..097e025 100644
--- a/drivers/net/qede/qede_eth_if.h
+++ b/drivers/net/qede/qede_eth_if.h
@@ -129,6 +129,4 @@ struct qed_eth_ops {
 int qed_configure_filter_rx_mode(struct rte_eth_dev *eth_dev,
 				 enum qed_filter_rx_mode_type type);
 
-bool qed_update_rss_parm_cmt(struct ecore_dev *edev, uint16_t *p_tbl);
-
 #endif /* _QEDE_ETH_IF_H */
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index 4ef5765..7501eb2 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -1576,6 +1576,61 @@ static int qede_rss_hash_conf_get(struct rte_eth_dev *eth_dev,
 	return 0;
 }
 
+static bool qede_update_rss_parm_cmt(struct ecore_dev *edev,
+				    struct ecore_rss_params *rss)
+{
+	int i, fn;
+	bool rss_mode = 1; /* enable */
+	struct ecore_queue_cid *cid;
+	struct ecore_rss_params *t_rss;
+
+	/* In regular scenario, we'd simply need to take input handlers.
+	 * But in CMT, we'd have to split the handlers according to the
+	 * engine they were configured on. We'd then have to understand
+	 * whether RSS is really required, since 2-queues on CMT doesn't
+	 * require RSS.
+	 */
+
+	/* CMT should be round-robin */
+	for (i = 0; i < ECORE_RSS_IND_TABLE_SIZE; i++) {
+		cid = rss->rss_ind_table[i];
+
+		if (cid->p_owner == ECORE_LEADING_HWFN(edev))
+			t_rss = &rss[0];
+		else
+			t_rss = &rss[1];
+
+		t_rss->rss_ind_table[i / edev->num_hwfns] = cid;
+	}
+
+	t_rss = &rss[1];
+	t_rss->update_rss_ind_table = 1;
+	t_rss->rss_table_size_log = 7;
+	t_rss->update_rss_config = 1;
+
+	/* Make sure RSS is actually required */
+	for_each_hwfn(edev, fn) {
+		for (i = 1; i < ECORE_RSS_IND_TABLE_SIZE / edev->num_hwfns;
+		     i++) {
+			if (rss[fn].rss_ind_table[i] !=
+			    rss[fn].rss_ind_table[0])
+				break;
+		}
+
+		if (i == ECORE_RSS_IND_TABLE_SIZE / edev->num_hwfns) {
+			DP_INFO(edev,
+				"CMT - 1 queue per-hwfn; Disabling RSS\n");
+			rss_mode = 0;
+			goto out;
+		}
+	}
+
+out:
+	t_rss->rss_enable = rss_mode;
+
+	return rss_mode;
+}
+
 int qede_rss_reta_update(struct rte_eth_dev *eth_dev,
 			 struct rte_eth_rss_reta_entry64 *reta_conf,
 			 uint16_t reta_size)
@@ -1583,11 +1638,11 @@ int qede_rss_reta_update(struct rte_eth_dev *eth_dev,
 	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
 	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
 	struct ecore_sp_vport_update_params vport_update_params;
-	struct ecore_rss_params params;
+	struct ecore_rss_params *params;
 	struct ecore_hwfn *p_hwfn;
 	uint16_t i, idx, shift;
 	uint8_t entry;
-	int rc;
+	int rc = 0;
 
 	if (reta_size > ETH_RSS_RETA_SIZE_128) {
 		DP_ERR(edev, "reta_size %d is not supported by hardware\n",
@@ -1596,7 +1651,8 @@ int qede_rss_reta_update(struct rte_eth_dev *eth_dev,
 	}
 
 	memset(&vport_update_params, 0, sizeof(vport_update_params));
-	memset(&params, 0, sizeof(params));
+	params = rte_zmalloc("qede_rss", sizeof(*params) * edev->num_hwfns,
+			     RTE_CACHE_LINE_SIZE);
 
 	for (i = 0; i < reta_size; i++) {
 		idx = i / RTE_RETA_GROUP_SIZE;
@@ -1604,24 +1660,25 @@ int qede_rss_reta_update(struct rte_eth_dev *eth_dev,
 		if (reta_conf[idx].mask & (1ULL << shift)) {
 			entry = reta_conf[idx].reta[shift];
 			/* Pass rxq handles to ecore */
-			params.rss_ind_table[i] =
+			params->rss_ind_table[i] =
 					qdev->fp_array[entry].rxq->handle;
 			/* Update the local copy for RETA query command */
 			qdev->rss_ind_table[i] = entry;
 		}
 	}
 
+	params->update_rss_ind_table = 1;
+	params->rss_table_size_log = 7;
+	params->update_rss_config = 1;
+
 	/* Fix up RETA for CMT mode device */
 	if (edev->num_hwfns > 1)
-		qdev->rss_enable = qed_update_rss_parm_cmt(edev,
-					params.rss_ind_table[0]);
-	params.update_rss_ind_table = 1;
-	params.rss_table_size_log = 7;
-	params.update_rss_config = 1;
+		qdev->rss_enable = qede_update_rss_parm_cmt(edev,
+							    params);
 	vport_update_params.vport_id = 0;
 	/* Use the current value of rss_enable */
-	params.rss_enable = qdev->rss_enable;
-	vport_update_params.rss_params = &params;
+	params->rss_enable = qdev->rss_enable;
+	vport_update_params.rss_params = params;
 
 	for_each_hwfn(edev, i) {
 		p_hwfn = &edev->hwfns[i];
@@ -1630,11 +1687,13 @@ int qede_rss_reta_update(struct rte_eth_dev *eth_dev,
 					   ECORE_SPQ_MODE_EBLOCK, NULL);
 		if (rc) {
 			DP_ERR(edev, "vport-update for RSS failed\n");
-			return rc;
+			goto out;
 		}
 	}
 
-	return 0;
+out:
+	rte_free(params);
+	return rc;
 }
 
 static int qede_rss_reta_query(struct rte_eth_dev *eth_dev,
diff --git a/drivers/net/qede/qede_ethdev.h b/drivers/net/qede/qede_ethdev.h
index 26b6332..e4323a0 100644
--- a/drivers/net/qede/qede_ethdev.h
+++ b/drivers/net/qede/qede_ethdev.h
@@ -37,6 +37,7 @@
 #include "base/ecore_sp_commands.h"
 #include "base/ecore_l2.h"
 #include "base/ecore_dev_api.h"
+#include "base/ecore_l2.h"
 
 #include "qede_logs.h"
 #include "qede_if.h"
-- 
1.7.10.3

Re: [dpdk-dev] [PATCH] net/qede: fix RSS table entries for 100G adapter

[Mody, Rasesh]

Hi Thomas,

> From: Rasesh Mody [mailto:rasesh.mody@cavium.com]
> Sent: Sunday, May 07, 2017 3:53 PM
> 
> With the change in base APIs the logic for 100G handling needs to be
> adjusted to pass cid values instead for queue ids. The current API works
> assuming its queue id.
> 
> Fixes: 69d7ba88f1a1 ("net/qede/base: use L2-handles for RSS configuration")

This is a *CRITICAL MUST FIX* patch for 100Gb adapters supported by qed/qede on DPDK 17.05. There will be functional and performance issues/regression without this fix on 100Gb adapters. 

Without this change, RSS in 100Gb adapters do not function properly and leads to performance issues.  Rx Queue stop command on 100Gb adapter will results in FW exception, where the Rx queue stop request gets stuck in FW and leads to PMD/application error saying failed to stop RXQ 0.

The issue was introduced by one of the 17.05 base driver patches (net/qede/base: use L2-handles for RSS configuration), which has changed the logic to use queue handles instead of queue ids. However, we missed to make the corresponding changes in 17.05 QEDE for the 100Gb adapter. This patch addresses that by making changes in QEDE to work with queue handles and associate the queues to respective HW functions based on queue handles.

Please include this fix in 17.05 release.

Thanks!
-Rasesh

> 
> Signed-off-by: Rasesh Mody <rasesh.mody@cavium.com>
> ---
>  drivers/net/qede/qede_eth_if.c |   27 -------------
>  drivers/net/qede/qede_eth_if.h |    2 -
>  drivers/net/qede/qede_ethdev.c |   85
> ++++++++++++++++++++++++++++++++++------
>  drivers/net/qede/qede_ethdev.h |    1 +
>  4 files changed, 73 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/qede/qede_eth_if.c
> b/drivers/net/qede/qede_eth_if.c index 86bb129..a3c0b13 100644
> --- a/drivers/net/qede/qede_eth_if.c
> +++ b/drivers/net/qede/qede_eth_if.c
> @@ -67,33 +67,6 @@ static int qed_stop_vport(struct ecore_dev *edev,
> uint8_t vport_id)
>  	return 0;
>  }
> 
> -bool qed_update_rss_parm_cmt(struct ecore_dev *edev, uint16_t *p_tbl)
> -{
> -	uint16_t max = 0, k;
> -	bool rss_mode = 0; /* disable */
> -	int divisor;
> -
> -	/* Find largest entry, since it's possible RSS needs to
> -	 * be disabled [in case only 1 queue per-hwfn]
> -	 */
> -	for (k = 0; k < ECORE_RSS_IND_TABLE_SIZE; k++)
> -		max = (max > p_tbl[k]) ?  max : p_tbl[k];
> -
> -	/* Either fix RSS values or disable RSS */
> -	if (edev->num_hwfns < max + 1) {
> -		divisor = (max + edev->num_hwfns - 1) / edev->num_hwfns;
> -		DP_VERBOSE(edev, ECORE_MSG_SPQ,
> -			   "CMT - fixing RSS values (modulo %02x)\n",
> -			   divisor);
> -		for (k = 0; k < ECORE_RSS_IND_TABLE_SIZE; k++)
> -			p_tbl[k] = p_tbl[k] % divisor;
> -
> -		rss_mode = 1;
> -	}
> -
> -	return rss_mode;
> -}
> -
>  static int
>  qed_update_vport(struct ecore_dev *edev, struct
> qed_update_vport_params *params)  { diff --git
> a/drivers/net/qede/qede_eth_if.h b/drivers/net/qede/qede_eth_if.h
> index d845bac..097e025 100644
> --- a/drivers/net/qede/qede_eth_if.h
> +++ b/drivers/net/qede/qede_eth_if.h
> @@ -129,6 +129,4 @@ struct qed_eth_ops {  int
> qed_configure_filter_rx_mode(struct rte_eth_dev *eth_dev,
>  				 enum qed_filter_rx_mode_type type);
> 
> -bool qed_update_rss_parm_cmt(struct ecore_dev *edev, uint16_t *p_tbl);
> -
>  #endif /* _QEDE_ETH_IF_H */
> diff --git a/drivers/net/qede/qede_ethdev.c
> b/drivers/net/qede/qede_ethdev.c index 4ef5765..7501eb2 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -1576,6 +1576,61 @@ static int qede_rss_hash_conf_get(struct
> rte_eth_dev *eth_dev,
>  	return 0;
>  }
> 
> +static bool qede_update_rss_parm_cmt(struct ecore_dev *edev,
> +				    struct ecore_rss_params *rss)
> +{
> +	int i, fn;
> +	bool rss_mode = 1; /* enable */
> +	struct ecore_queue_cid *cid;
> +	struct ecore_rss_params *t_rss;
> +
> +	/* In regular scenario, we'd simply need to take input handlers.
> +	 * But in CMT, we'd have to split the handlers according to the
> +	 * engine they were configured on. We'd then have to understand
> +	 * whether RSS is really required, since 2-queues on CMT doesn't
> +	 * require RSS.
> +	 */
> +
> +	/* CMT should be round-robin */
> +	for (i = 0; i < ECORE_RSS_IND_TABLE_SIZE; i++) {
> +		cid = rss->rss_ind_table[i];
> +
> +		if (cid->p_owner == ECORE_LEADING_HWFN(edev))
> +			t_rss = &rss[0];
> +		else
> +			t_rss = &rss[1];
> +
> +		t_rss->rss_ind_table[i / edev->num_hwfns] = cid;
> +	}
> +
> +	t_rss = &rss[1];
> +	t_rss->update_rss_ind_table = 1;
> +	t_rss->rss_table_size_log = 7;
> +	t_rss->update_rss_config = 1;
> +
> +	/* Make sure RSS is actually required */
> +	for_each_hwfn(edev, fn) {
> +		for (i = 1; i < ECORE_RSS_IND_TABLE_SIZE / edev-
> >num_hwfns;
> +		     i++) {
> +			if (rss[fn].rss_ind_table[i] !=
> +			    rss[fn].rss_ind_table[0])
> +				break;
> +		}
> +
> +		if (i == ECORE_RSS_IND_TABLE_SIZE / edev->num_hwfns) {
> +			DP_INFO(edev,
> +				"CMT - 1 queue per-hwfn; Disabling RSS\n");
> +			rss_mode = 0;
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	t_rss->rss_enable = rss_mode;
> +
> +	return rss_mode;
> +}
> +
>  int qede_rss_reta_update(struct rte_eth_dev *eth_dev,
>  			 struct rte_eth_rss_reta_entry64 *reta_conf,
>  			 uint16_t reta_size)
> @@ -1583,11 +1638,11 @@ int qede_rss_reta_update(struct rte_eth_dev
> *eth_dev,
>  	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
>  	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
>  	struct ecore_sp_vport_update_params vport_update_params;
> -	struct ecore_rss_params params;
> +	struct ecore_rss_params *params;
>  	struct ecore_hwfn *p_hwfn;
>  	uint16_t i, idx, shift;
>  	uint8_t entry;
> -	int rc;
> +	int rc = 0;
> 
>  	if (reta_size > ETH_RSS_RETA_SIZE_128) {
>  		DP_ERR(edev, "reta_size %d is not supported by
> hardware\n", @@ -1596,7 +1651,8 @@ int qede_rss_reta_update(struct
> rte_eth_dev *eth_dev,
>  	}
> 
>  	memset(&vport_update_params, 0, sizeof(vport_update_params));
> -	memset(&params, 0, sizeof(params));
> +	params = rte_zmalloc("qede_rss", sizeof(*params) * edev-
> >num_hwfns,
> +			     RTE_CACHE_LINE_SIZE);
> 
>  	for (i = 0; i < reta_size; i++) {
>  		idx = i / RTE_RETA_GROUP_SIZE;
> @@ -1604,24 +1660,25 @@ int qede_rss_reta_update(struct rte_eth_dev
> *eth_dev,
>  		if (reta_conf[idx].mask & (1ULL << shift)) {
>  			entry = reta_conf[idx].reta[shift];
>  			/* Pass rxq handles to ecore */
> -			params.rss_ind_table[i] =
> +			params->rss_ind_table[i] =
>  					qdev->fp_array[entry].rxq->handle;
>  			/* Update the local copy for RETA query command */
>  			qdev->rss_ind_table[i] = entry;
>  		}
>  	}
> 
> +	params->update_rss_ind_table = 1;
> +	params->rss_table_size_log = 7;
> +	params->update_rss_config = 1;
> +
>  	/* Fix up RETA for CMT mode device */
>  	if (edev->num_hwfns > 1)
> -		qdev->rss_enable = qed_update_rss_parm_cmt(edev,
> -					params.rss_ind_table[0]);
> -	params.update_rss_ind_table = 1;
> -	params.rss_table_size_log = 7;
> -	params.update_rss_config = 1;
> +		qdev->rss_enable = qede_update_rss_parm_cmt(edev,
> +							    params);
>  	vport_update_params.vport_id = 0;
>  	/* Use the current value of rss_enable */
> -	params.rss_enable = qdev->rss_enable;
> -	vport_update_params.rss_params = &params;
> +	params->rss_enable = qdev->rss_enable;
> +	vport_update_params.rss_params = params;
> 
>  	for_each_hwfn(edev, i) {
>  		p_hwfn = &edev->hwfns[i];
> @@ -1630,11 +1687,13 @@ int qede_rss_reta_update(struct rte_eth_dev
> *eth_dev,
>  					   ECORE_SPQ_MODE_EBLOCK,
> NULL);
>  		if (rc) {
>  			DP_ERR(edev, "vport-update for RSS failed\n");
> -			return rc;
> +			goto out;
>  		}
>  	}
> 
> -	return 0;
> +out:
> +	rte_free(params);
> +	return rc;
>  }
> 
>  static int qede_rss_reta_query(struct rte_eth_dev *eth_dev, diff --git
> a/drivers/net/qede/qede_ethdev.h b/drivers/net/qede/qede_ethdev.h
> index 26b6332..e4323a0 100644
> --- a/drivers/net/qede/qede_ethdev.h
> +++ b/drivers/net/qede/qede_ethdev.h
> @@ -37,6 +37,7 @@
>  #include "base/ecore_sp_commands.h"
>  #include "base/ecore_l2.h"
>  #include "base/ecore_dev_api.h"
> +#include "base/ecore_l2.h"
> 
>  #include "qede_logs.h"
>  #include "qede_if.h"
> --
> 1.7.10.3

Re: [dpdk-dev] [PATCH] net/qede: fix RSS table entries for 100G adapter

[Thomas Monjalon]

08/05/2017 21:03, Mody, Rasesh:
> Hi Thomas,
> 
> > From: Rasesh Mody [mailto:rasesh.mody@cavium.com]
> > Sent: Sunday, May 07, 2017 3:53 PM
> > 
> > With the change in base APIs the logic for 100G handling needs to be
> > adjusted to pass cid values instead for queue ids. The current API works
> > assuming its queue id.
> > 
> > Fixes: 69d7ba88f1a1 ("net/qede/base: use L2-handles for RSS configuration")
> 
> This is a *CRITICAL MUST FIX* patch for 100Gb adapters supported by qed/qede on DPDK 17.05. There will be functional and performance issues/regression without this fix on 100Gb adapters. 
> 
> Without this change, RSS in 100Gb adapters do not function properly and leads to performance issues.  Rx Queue stop command on 100Gb adapter will results in FW exception, where the Rx queue stop request gets stuck in FW and leads to PMD/application error saying failed to stop RXQ 0.
> 
> The issue was introduced by one of the 17.05 base driver patches (net/qede/base: use L2-handles for RSS configuration), which has changed the logic to use queue handles instead of queue ids. However, we missed to make the corresponding changes in 17.05 QEDE for the 100Gb adapter. This patch addresses that by making changes in QEDE to work with queue handles and associate the queues to respective HW functions based on queue handles.
> 
> Please include this fix in 17.05 release.

Applied, thanks

This patch is big.
So I will wait for compilation tests of the night before releasing.