[PATCH 0/2] next/mlx5: fix indirect RSS reference counting

[PATCH 0/2] next/mlx5: fix indirect RSS reference counting

[Dmitry Kozlyuk]

Dmitry Kozlyuk (2):
  net/mlx5: fix indirect RSS creation when port is stopped
  net/mlx5: fix RxQ reference counting for indirect RSS

 drivers/net/mlx5/mlx5_rxq.c | 56 +++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH 1/2] net/mlx5: fix indirect RSS creation when port is stopped

[Dmitry Kozlyuk]

mlx5_ind_table_obj_setup() was incrementing RxQ reference counters
even when the port was stopped, which prevented RxQ release
and triggered an internal assertion.
Only increment reference counter when the port is started.

Fixes: ec4e11d41d12 ("net/mlx5: preserve indirect actions on restart")

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
---
 drivers/net/mlx5/mlx5_rxq.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 47dc24793b..8f9a94572f 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2270,6 +2270,7 @@ mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
 			 struct mlx5_ind_table_obj *ind_tbl)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
+	bool dev_started = priv->dev_data->dev_started;
 	uint32_t queues_n = ind_tbl->queues_n;
 	uint16_t *queues = ind_tbl->queues;
 	unsigned int i, j;
@@ -2278,22 +2279,25 @@ mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
 			       log2above(queues_n) :
 			       log2above(priv->config.ind_table_max_size);
 
-	for (i = 0; i != queues_n; ++i) {
-		if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
-			ret = -rte_errno;
-			goto error;
+	if (dev_started)
+		for (i = 0; i != queues_n; ++i) {
+			if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
+				ret = -rte_errno;
+				goto error;
+			}
 		}
-	}
 	ret = priv->obj_ops.ind_table_new(dev, n, ind_tbl);
 	if (ret)
 		goto error;
 	__atomic_fetch_add(&ind_tbl->refcnt, 1, __ATOMIC_RELAXED);
 	return 0;
 error:
-	err = rte_errno;
-	for (j = 0; j < i; j++)
-		mlx5_rxq_deref(dev, ind_tbl->queues[j]);
-	rte_errno = err;
+	if (dev_started) {
+		err = rte_errno;
+		for (j = 0; j < i; j++)
+			mlx5_rxq_deref(dev, queues[j]);
+		rte_errno = err;
+	}
 	DRV_LOG(DEBUG, "Port %u cannot setup indirection table.",
 		dev->data->port_id);
 	return ret;
-- 
2.25.1

[PATCH 2/2] net/mlx5: fix RxQ reference counting for indirect RSS

[Dmitry Kozlyuk]

mlx5_ind_table_obj_modify() was not changing the reference counters
of neither the new set of RxQs, nor the old set of RxQs.
On the other hand, creation of the RSS incremented the RxQ refcnt.
If an RxQ was present in both the initial and the modified set,
its reference counter was incremented one extra time
compared to the queues that were only present in the new set.
This prevented releasing said RxQ resources on port stop:

    flow indirect_action 0 create action_id 1 \
        action rss queues 0 1 end / end
    flow indirect_action 0 update 1 \
        action rss queues 2 3 end / end
    quit
    ...
    mlx5_net: mlx5.c:1622: mlx5_dev_close():
        port 0 some Rx queue objects still remain
    mlx5_net: mlx5.c:1626: mlx5_dev_close():
        port 0 some Rx queues still remain

Increment reference counters for the new set of RxQs
and decrement them for the old set of RxQs.
Only do this when the port is started when the port is started.
Remove explicit referencing of RxQ from mlx5_ind_table_obj_attach()
because it reuses mlx5_ind_table_obj_modify() code doing this.

Fixes: ec4e11d41d12 ("net/mlx5: preserve indirect actions on restart")

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
---
 drivers/net/mlx5/mlx5_rxq.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 8f9a94572f..46d6536be5 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2392,7 +2392,8 @@ mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 			  bool standalone)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
-	unsigned int i;
+	bool dev_started = priv->dev_data->dev_started;
+	unsigned int i, j;
 	int ret = 0, err;
 	const unsigned int n = rte_is_power_of_2(queues_n) ?
 			       log2above(queues_n) :
@@ -2402,22 +2403,30 @@ mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 	RTE_SET_USED(standalone);
 	if (mlx5_ind_table_obj_check_standalone(dev, ind_tbl) < 0)
 		return -rte_errno;
-	for (i = 0; i != queues_n; ++i) {
-		if (!mlx5_rxq_get(dev, queues[i])) {
-			ret = -rte_errno;
-			goto error;
+	if (dev_started)
+		for (i = 0; i != queues_n; ++i) {
+			if (!mlx5_rxq_ref(dev, queues[i])) {
+				ret = -rte_errno;
+				goto error;
+			}
 		}
-	}
 	MLX5_ASSERT(priv->obj_ops.ind_table_modify);
 	ret = priv->obj_ops.ind_table_modify(dev, n, queues, queues_n, ind_tbl);
 	if (ret)
 		goto error;
+	if (dev_started)
+		for (i = 0; i < ind_tbl->queues_n; i++)
+			claim_nonzero(mlx5_rxq_deref(dev, ind_tbl->queues[i]));
 	ind_tbl->queues_n = queues_n;
 	ind_tbl->queues = queues;
 	return 0;
 error:
-	err = rte_errno;
-	rte_errno = err;
+	if (dev_started) {
+		err = rte_errno;
+		for (j = 0; j < i; j++)
+			mlx5_rxq_deref(dev, queues[j]);
+		rte_errno = err;
+	}
 	DRV_LOG(DEBUG, "Port %u cannot setup indirection table.",
 		dev->data->port_id);
 	return ret;
@@ -2438,19 +2447,14 @@ int
 mlx5_ind_table_obj_attach(struct rte_eth_dev *dev,
 			  struct mlx5_ind_table_obj *ind_tbl)
 {
-	unsigned int i;
 	int ret;
 
 	ret = mlx5_ind_table_obj_modify(dev, ind_tbl, ind_tbl->queues,
 					ind_tbl->queues_n, true);
-	if (ret != 0) {
+	if (ret != 0)
 		DRV_LOG(ERR, "Port %u could not modify indirect table obj %p",
 			dev->data->port_id, (void *)ind_tbl);
-		return ret;
-	}
-	for (i = 0; i < ind_tbl->queues_n; i++)
-		mlx5_rxq_ref(dev, ind_tbl->queues[i]);
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.25.1

[PATCH 0/2] next/mlx5: fix indirect RSS reference counting

[Dmitry Kozlyuk]

Dmitry Kozlyuk (2):
  net/mlx5: fix indirect RSS creation when port is stopped
  net/mlx5: fix RxQ reference counting for indirect RSS

 drivers/net/mlx5/mlx5_rxq.c | 56 +++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH 1/2] net/mlx5: fix indirect RSS creation when port is stopped

[Dmitry Kozlyuk]

mlx5_ind_table_obj_setup() was incrementing RxQ reference counters
even when the port was stopped, which prevented RxQ release
and triggered an internal assertion.
Only increment reference counter when the port is started.

Fixes: ec4e11d41d12 ("net/mlx5: preserve indirect actions on restart")

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
---
 drivers/net/mlx5/mlx5_rxq.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 47dc24793b..8f9a94572f 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2270,6 +2270,7 @@ mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
 			 struct mlx5_ind_table_obj *ind_tbl)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
+	bool dev_started = priv->dev_data->dev_started;
 	uint32_t queues_n = ind_tbl->queues_n;
 	uint16_t *queues = ind_tbl->queues;
 	unsigned int i, j;
@@ -2278,22 +2279,25 @@ mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
 			       log2above(queues_n) :
 			       log2above(priv->config.ind_table_max_size);
 
-	for (i = 0; i != queues_n; ++i) {
-		if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
-			ret = -rte_errno;
-			goto error;
+	if (dev_started)
+		for (i = 0; i != queues_n; ++i) {
+			if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
+				ret = -rte_errno;
+				goto error;
+			}
 		}
-	}
 	ret = priv->obj_ops.ind_table_new(dev, n, ind_tbl);
 	if (ret)
 		goto error;
 	__atomic_fetch_add(&ind_tbl->refcnt, 1, __ATOMIC_RELAXED);
 	return 0;
 error:
-	err = rte_errno;
-	for (j = 0; j < i; j++)
-		mlx5_rxq_deref(dev, ind_tbl->queues[j]);
-	rte_errno = err;
+	if (dev_started) {
+		err = rte_errno;
+		for (j = 0; j < i; j++)
+			mlx5_rxq_deref(dev, queues[j]);
+		rte_errno = err;
+	}
 	DRV_LOG(DEBUG, "Port %u cannot setup indirection table.",
 		dev->data->port_id);
 	return ret;
-- 
2.25.1

[PATCH 2/2] net/mlx5: fix RxQ reference counting for indirect RSS

[Dmitry Kozlyuk]

mlx5_ind_table_obj_modify() was not changing the reference counters
of neither the new set of RxQs, nor the old set of RxQs.
On the other hand, creation of the RSS incremented the RxQ refcnt.
If an RxQ was present in both the initial and the modified set,
its reference counter was incremented one extra time
compared to the queues that were only present in the new set.
This prevented releasing said RxQ resources on port stop:

    flow indirect_action 0 create action_id 1 \
        action rss queues 0 1 end / end
    flow indirect_action 0 update 1 \
        action rss queues 2 3 end / end
    quit
    ...
    mlx5_net: mlx5.c:1622: mlx5_dev_close():
        port 0 some Rx queue objects still remain
    mlx5_net: mlx5.c:1626: mlx5_dev_close():
        port 0 some Rx queues still remain

Increment reference counters for the new set of RxQs
and decrement them for the old set of RxQs.
Only do this when the port is started when the port is started.
Remove explicit referencing of RxQ from mlx5_ind_table_obj_attach()
because it reuses mlx5_ind_table_obj_modify() code doing this.

Fixes: ec4e11d41d12 ("net/mlx5: preserve indirect actions on restart")

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
---
 drivers/net/mlx5/mlx5_rxq.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 8f9a94572f..46d6536be5 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2392,7 +2392,8 @@ mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 			  bool standalone)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
-	unsigned int i;
+	bool dev_started = priv->dev_data->dev_started;
+	unsigned int i, j;
 	int ret = 0, err;
 	const unsigned int n = rte_is_power_of_2(queues_n) ?
 			       log2above(queues_n) :
@@ -2402,22 +2403,30 @@ mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 	RTE_SET_USED(standalone);
 	if (mlx5_ind_table_obj_check_standalone(dev, ind_tbl) < 0)
 		return -rte_errno;
-	for (i = 0; i != queues_n; ++i) {
-		if (!mlx5_rxq_get(dev, queues[i])) {
-			ret = -rte_errno;
-			goto error;
+	if (dev_started)
+		for (i = 0; i != queues_n; ++i) {
+			if (!mlx5_rxq_ref(dev, queues[i])) {
+				ret = -rte_errno;
+				goto error;
+			}
 		}
-	}
 	MLX5_ASSERT(priv->obj_ops.ind_table_modify);
 	ret = priv->obj_ops.ind_table_modify(dev, n, queues, queues_n, ind_tbl);
 	if (ret)
 		goto error;
+	if (dev_started)
+		for (i = 0; i < ind_tbl->queues_n; i++)
+			claim_nonzero(mlx5_rxq_deref(dev, ind_tbl->queues[i]));
 	ind_tbl->queues_n = queues_n;
 	ind_tbl->queues = queues;
 	return 0;
 error:
-	err = rte_errno;
-	rte_errno = err;
+	if (dev_started) {
+		err = rte_errno;
+		for (j = 0; j < i; j++)
+			mlx5_rxq_deref(dev, queues[j]);
+		rte_errno = err;
+	}
 	DRV_LOG(DEBUG, "Port %u cannot setup indirection table.",
 		dev->data->port_id);
 	return ret;
@@ -2438,19 +2447,14 @@ int
 mlx5_ind_table_obj_attach(struct rte_eth_dev *dev,
 			  struct mlx5_ind_table_obj *ind_tbl)
 {
-	unsigned int i;
 	int ret;
 
 	ret = mlx5_ind_table_obj_modify(dev, ind_tbl, ind_tbl->queues,
 					ind_tbl->queues_n, true);
-	if (ret != 0) {
+	if (ret != 0)
 		DRV_LOG(ERR, "Port %u could not modify indirect table obj %p",
 			dev->data->port_id, (void *)ind_tbl);
-		return ret;
-	}
-	for (i = 0; i < ind_tbl->queues_n; i++)
-		mlx5_rxq_ref(dev, ind_tbl->queues[i]);
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.25.1

[PATCH v2 0/2] net/mlx5: fix indirect RSS reference counting

[Dmitry Kozlyuk]

v2:
  * Fix build with GCc on RHEL7 (CI).
  * Fix RxQ leak when an indirect RSS had been created before the start
    and then modified after the start (Matan).

Dmitry Kozlyuk (2):
  net/mlx5: fix indirect RSS creation when port is stopped
  net/mlx5: fix RxQ reference counting for indirect RSS

 drivers/net/mlx5/mlx5_flow_dv.c |  9 +++-
 drivers/net/mlx5/mlx5_rx.h      |  6 ++-
 drivers/net/mlx5/mlx5_rxq.c     | 85 ++++++++++++++++++++-------------
 3 files changed, 64 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] net/mlx5: fix indirect RSS creation when port is stopped

[Dmitry Kozlyuk]

mlx5_ind_table_obj_setup() was incrementing RxQ reference counters
even when the port was stopped, which prevented RxQ release
and triggered an internal assertion.
Only increment reference counter when the port is started.

Fixes: ec4e11d41d12 ("net/mlx5: preserve indirect actions on restart")

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Reviewed-by: Matan Azrad <matan@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c |  3 ++-
 drivers/net/mlx5/mlx5_rx.h      |  3 ++-
 drivers/net/mlx5/mlx5_rxq.c     | 41 +++++++++++++++++++++------------
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 9d4bd0560c..a8f63e22c4 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -14700,7 +14700,8 @@ __flow_dv_action_rss_setup(struct rte_eth_dev *dev,
 	size_t i;
 	int err;
 
-	if (mlx5_ind_table_obj_setup(dev, shared_rss->ind_tbl)) {
+	if (mlx5_ind_table_obj_setup(dev, shared_rss->ind_tbl,
+				     !!dev->data->dev_started)) {
 		return rte_flow_error_set(error, rte_errno,
 					  RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
 					  "cannot setup indirection table");
diff --git a/drivers/net/mlx5/mlx5_rx.h b/drivers/net/mlx5/mlx5_rx.h
index b19464bb37..283242f530 100644
--- a/drivers/net/mlx5/mlx5_rx.h
+++ b/drivers/net/mlx5/mlx5_rx.h
@@ -228,7 +228,8 @@ int mlx5_ind_table_obj_release(struct rte_eth_dev *dev,
 			       bool standalone,
 			       bool deref_rxqs);
 int mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
-			     struct mlx5_ind_table_obj *ind_tbl);
+			     struct mlx5_ind_table_obj *ind_tbl,
+			     bool ref_qs);
 int mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 			      struct mlx5_ind_table_obj *ind_tbl,
 			      uint16_t *queues, const uint32_t queues_n,
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 191f1e483f..2dd9490c36 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2263,39 +2263,45 @@ mlx5_ind_table_obj_verify(struct rte_eth_dev *dev)
  *   Pointer to Ethernet device.
  * @param ind_table
  *   Indirection table to modify.
+ * @param ref_qs
+ *   Whether to increment RxQ reference counters.
  *
  * @return
  *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
 int
 mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
-			 struct mlx5_ind_table_obj *ind_tbl)
+			 struct mlx5_ind_table_obj *ind_tbl,
+			 bool ref_qs)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 	uint32_t queues_n = ind_tbl->queues_n;
 	uint16_t *queues = ind_tbl->queues;
-	unsigned int i, j;
+	unsigned int i = 0, j;
 	int ret = 0, err;
 	const unsigned int n = rte_is_power_of_2(queues_n) ?
 			       log2above(queues_n) :
 			       log2above(priv->config.ind_table_max_size);
 
-	for (i = 0; i != queues_n; ++i) {
-		if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
-			ret = -rte_errno;
-			goto error;
+	if (ref_qs)
+		for (i = 0; i != queues_n; ++i) {
+			if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
+				ret = -rte_errno;
+				goto error;
+			}
 		}
-	}
 	ret = priv->obj_ops.ind_table_new(dev, n, ind_tbl);
 	if (ret)
 		goto error;
 	__atomic_fetch_add(&ind_tbl->refcnt, 1, __ATOMIC_RELAXED);
 	return 0;
 error:
-	err = rte_errno;
-	for (j = 0; j < i; j++)
-		mlx5_rxq_deref(dev, ind_tbl->queues[j]);
-	rte_errno = err;
+	if (ref_qs) {
+		err = rte_errno;
+		for (j = 0; j < i; j++)
+			mlx5_rxq_deref(dev, queues[j]);
+		rte_errno = err;
+	}
 	DRV_LOG(DEBUG, "Port %u cannot setup indirection table.",
 		dev->data->port_id);
 	return ret;
@@ -2312,13 +2318,15 @@ mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
  *   Number of queues in the array.
  * @param standalone
  *   Indirection table for Standalone queue.
+ * @param ref_qs
+ *   Whether to increment RxQ reference counters.
  *
  * @return
  *   The Verbs/DevX object initialized, NULL otherwise and rte_errno is set.
  */
 static struct mlx5_ind_table_obj *
 mlx5_ind_table_obj_new(struct rte_eth_dev *dev, const uint16_t *queues,
-		       uint32_t queues_n, bool standalone)
+		       uint32_t queues_n, bool standalone, bool ref_qs)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 	struct mlx5_ind_table_obj *ind_tbl;
@@ -2333,7 +2341,7 @@ mlx5_ind_table_obj_new(struct rte_eth_dev *dev, const uint16_t *queues,
 	ind_tbl->queues_n = queues_n;
 	ind_tbl->queues = (uint16_t *)(ind_tbl + 1);
 	memcpy(ind_tbl->queues, queues, queues_n * sizeof(*queues));
-	ret = mlx5_ind_table_obj_setup(dev, ind_tbl);
+	ret = mlx5_ind_table_obj_setup(dev, ind_tbl, ref_qs);
 	if (ret < 0) {
 		mlx5_free(ind_tbl);
 		return NULL;
@@ -2537,6 +2545,7 @@ mlx5_hrxq_modify(struct rte_eth_dev *dev, uint32_t hrxq_idx,
 	struct mlx5_priv *priv = dev->data->dev_private;
 	struct mlx5_hrxq *hrxq =
 		mlx5_ipool_get(priv->sh->ipool[MLX5_IPOOL_HRXQ], hrxq_idx);
+	bool dev_started = !!dev->data->dev_started;
 	int ret;
 
 	if (!hrxq) {
@@ -2565,7 +2574,8 @@ mlx5_hrxq_modify(struct rte_eth_dev *dev, uint32_t hrxq_idx,
 		ind_tbl = mlx5_ind_table_obj_get(dev, queues, queues_n);
 		if (!ind_tbl)
 			ind_tbl = mlx5_ind_table_obj_new(dev, queues, queues_n,
-							 hrxq->standalone);
+							 hrxq->standalone,
+							 dev_started);
 	}
 	if (!ind_tbl) {
 		rte_errno = ENOMEM;
@@ -2657,7 +2667,8 @@ __mlx5_hrxq_create(struct rte_eth_dev *dev,
 		ind_tbl = mlx5_ind_table_obj_get(dev, queues, queues_n);
 	if (!ind_tbl)
 		ind_tbl = mlx5_ind_table_obj_new(dev, queues, queues_n,
-						 standalone);
+						 standalone,
+						 !!dev->data->dev_started);
 	if (!ind_tbl)
 		return NULL;
 	hrxq = mlx5_ipool_zmalloc(priv->sh->ipool[MLX5_IPOOL_HRXQ], &hrxq_idx);
-- 
2.25.1

[PATCH v3 0/2] net/mlx5: fix indirect RSS reference counting

[Dmitry Kozlyuk]

v3:
  * No changes, slipped when sending v2.
v2:
  * Fix build with GCc on RHEL7 (CI).
  * Fix RxQ leak when an indirect RSS had been created before the start
    and then modified after the start (Matan).

Dmitry Kozlyuk (2):
  net/mlx5: fix indirect RSS creation when port is stopped
  net/mlx5: fix RxQ reference counting for indirect RSS

 drivers/net/mlx5/mlx5_flow_dv.c |  9 +++-
 drivers/net/mlx5/mlx5_rx.h      |  6 ++-
 drivers/net/mlx5/mlx5_rxq.c     | 85 ++++++++++++++++++++-------------
 3 files changed, 64 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] net/mlx5: fix indirect RSS creation when port is stopped

[Dmitry Kozlyuk]

mlx5_ind_table_obj_setup() was incrementing RxQ reference counters
even when the port was stopped, which prevented RxQ release
and triggered an internal assertion.
Only increment reference counter when the port is started.

Fixes: ec4e11d41d12 ("net/mlx5: preserve indirect actions on restart")

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Reviewed-by: Matan Azrad <matan@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c |  3 ++-
 drivers/net/mlx5/mlx5_rx.h      |  3 ++-
 drivers/net/mlx5/mlx5_rxq.c     | 41 +++++++++++++++++++++------------
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 9d4bd0560c..a8f63e22c4 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -14700,7 +14700,8 @@ __flow_dv_action_rss_setup(struct rte_eth_dev *dev,
 	size_t i;
 	int err;
 
-	if (mlx5_ind_table_obj_setup(dev, shared_rss->ind_tbl)) {
+	if (mlx5_ind_table_obj_setup(dev, shared_rss->ind_tbl,
+				     !!dev->data->dev_started)) {
 		return rte_flow_error_set(error, rte_errno,
 					  RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
 					  "cannot setup indirection table");
diff --git a/drivers/net/mlx5/mlx5_rx.h b/drivers/net/mlx5/mlx5_rx.h
index b19464bb37..283242f530 100644
--- a/drivers/net/mlx5/mlx5_rx.h
+++ b/drivers/net/mlx5/mlx5_rx.h
@@ -228,7 +228,8 @@ int mlx5_ind_table_obj_release(struct rte_eth_dev *dev,
 			       bool standalone,
 			       bool deref_rxqs);
 int mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
-			     struct mlx5_ind_table_obj *ind_tbl);
+			     struct mlx5_ind_table_obj *ind_tbl,
+			     bool ref_qs);
 int mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 			      struct mlx5_ind_table_obj *ind_tbl,
 			      uint16_t *queues, const uint32_t queues_n,
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 191f1e483f..2dd9490c36 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2263,39 +2263,45 @@ mlx5_ind_table_obj_verify(struct rte_eth_dev *dev)
  *   Pointer to Ethernet device.
  * @param ind_table
  *   Indirection table to modify.
+ * @param ref_qs
+ *   Whether to increment RxQ reference counters.
  *
  * @return
  *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
 int
 mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
-			 struct mlx5_ind_table_obj *ind_tbl)
+			 struct mlx5_ind_table_obj *ind_tbl,
+			 bool ref_qs)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 	uint32_t queues_n = ind_tbl->queues_n;
 	uint16_t *queues = ind_tbl->queues;
-	unsigned int i, j;
+	unsigned int i = 0, j;
 	int ret = 0, err;
 	const unsigned int n = rte_is_power_of_2(queues_n) ?
 			       log2above(queues_n) :
 			       log2above(priv->config.ind_table_max_size);
 
-	for (i = 0; i != queues_n; ++i) {
-		if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
-			ret = -rte_errno;
-			goto error;
+	if (ref_qs)
+		for (i = 0; i != queues_n; ++i) {
+			if (mlx5_rxq_ref(dev, queues[i]) == NULL) {
+				ret = -rte_errno;
+				goto error;
+			}
 		}
-	}
 	ret = priv->obj_ops.ind_table_new(dev, n, ind_tbl);
 	if (ret)
 		goto error;
 	__atomic_fetch_add(&ind_tbl->refcnt, 1, __ATOMIC_RELAXED);
 	return 0;
 error:
-	err = rte_errno;
-	for (j = 0; j < i; j++)
-		mlx5_rxq_deref(dev, ind_tbl->queues[j]);
-	rte_errno = err;
+	if (ref_qs) {
+		err = rte_errno;
+		for (j = 0; j < i; j++)
+			mlx5_rxq_deref(dev, queues[j]);
+		rte_errno = err;
+	}
 	DRV_LOG(DEBUG, "Port %u cannot setup indirection table.",
 		dev->data->port_id);
 	return ret;
@@ -2312,13 +2318,15 @@ mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
  *   Number of queues in the array.
  * @param standalone
  *   Indirection table for Standalone queue.
+ * @param ref_qs
+ *   Whether to increment RxQ reference counters.
  *
  * @return
  *   The Verbs/DevX object initialized, NULL otherwise and rte_errno is set.
  */
 static struct mlx5_ind_table_obj *
 mlx5_ind_table_obj_new(struct rte_eth_dev *dev, const uint16_t *queues,
-		       uint32_t queues_n, bool standalone)
+		       uint32_t queues_n, bool standalone, bool ref_qs)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 	struct mlx5_ind_table_obj *ind_tbl;
@@ -2333,7 +2341,7 @@ mlx5_ind_table_obj_new(struct rte_eth_dev *dev, const uint16_t *queues,
 	ind_tbl->queues_n = queues_n;
 	ind_tbl->queues = (uint16_t *)(ind_tbl + 1);
 	memcpy(ind_tbl->queues, queues, queues_n * sizeof(*queues));
-	ret = mlx5_ind_table_obj_setup(dev, ind_tbl);
+	ret = mlx5_ind_table_obj_setup(dev, ind_tbl, ref_qs);
 	if (ret < 0) {
 		mlx5_free(ind_tbl);
 		return NULL;
@@ -2537,6 +2545,7 @@ mlx5_hrxq_modify(struct rte_eth_dev *dev, uint32_t hrxq_idx,
 	struct mlx5_priv *priv = dev->data->dev_private;
 	struct mlx5_hrxq *hrxq =
 		mlx5_ipool_get(priv->sh->ipool[MLX5_IPOOL_HRXQ], hrxq_idx);
+	bool dev_started = !!dev->data->dev_started;
 	int ret;
 
 	if (!hrxq) {
@@ -2565,7 +2574,8 @@ mlx5_hrxq_modify(struct rte_eth_dev *dev, uint32_t hrxq_idx,
 		ind_tbl = mlx5_ind_table_obj_get(dev, queues, queues_n);
 		if (!ind_tbl)
 			ind_tbl = mlx5_ind_table_obj_new(dev, queues, queues_n,
-							 hrxq->standalone);
+							 hrxq->standalone,
+							 dev_started);
 	}
 	if (!ind_tbl) {
 		rte_errno = ENOMEM;
@@ -2657,7 +2667,8 @@ __mlx5_hrxq_create(struct rte_eth_dev *dev,
 		ind_tbl = mlx5_ind_table_obj_get(dev, queues, queues_n);
 	if (!ind_tbl)
 		ind_tbl = mlx5_ind_table_obj_new(dev, queues, queues_n,
-						 standalone);
+						 standalone,
+						 !!dev->data->dev_started);
 	if (!ind_tbl)
 		return NULL;
 	hrxq = mlx5_ipool_zmalloc(priv->sh->ipool[MLX5_IPOOL_HRXQ], &hrxq_idx);
-- 
2.25.1

[PATCH v3 2/2] net/mlx5: fix RxQ reference counting for indirect RSS

[Dmitry Kozlyuk]

mlx5_ind_table_obj_modify() was not changing the reference counters
of neither the new set of RxQs, nor the old set of RxQs.
On the other hand, creation of the RSS incremented the RxQ refcnt.
If an RxQ was present in both the initial and the modified set,
its reference counter was incremented one extra time
compared to the queues that were only present in the new set.
This prevented releasing said RxQ resources on port stop:

    flow indirect_action 0 create action_id 1 \
        action rss queues 0 1 end / end
    flow indirect_action 0 update 1 \
        action rss queues 2 3 end / end
    quit
    ...
    mlx5_net: mlx5.c:1622: mlx5_dev_close():
        port 0 some Rx queue objects still remain
    mlx5_net: mlx5.c:1626: mlx5_dev_close():
        port 0 some Rx queues still remain

Increment reference counters for the new set of RxQs
and decrement them for the old set of RxQs when needed.
Remove explicit referencing of RxQ from mlx5_ind_table_obj_attach()
because it reuses mlx5_ind_table_obj_modify() code doing this.

Fixes: ec4e11d41d12 ("net/mlx5: preserve indirect actions on restart")

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Reviewed-by: Matan Azrad <matan@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c |  6 ++++-
 drivers/net/mlx5/mlx5_rx.h      |  3 ++-
 drivers/net/mlx5/mlx5_rxq.c     | 44 ++++++++++++++++++++-------------
 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index a8f63e22c4..9979d16f74 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -15073,6 +15073,7 @@ __flow_dv_action_rss_update(struct rte_eth_dev *dev, uint32_t idx,
 	void *queue = NULL;
 	uint16_t *queue_old = NULL;
 	uint32_t queue_size = action_conf->queue_num * sizeof(uint16_t);
+	bool dev_started = !!dev->data->dev_started;
 
 	if (!shared_rss)
 		return rte_flow_error_set(error, EINVAL,
@@ -15095,7 +15096,10 @@ __flow_dv_action_rss_update(struct rte_eth_dev *dev, uint32_t idx,
 	rte_spinlock_lock(&shared_rss->action_rss_sl);
 	queue_old = shared_rss->ind_tbl->queues;
 	ret = mlx5_ind_table_obj_modify(dev, shared_rss->ind_tbl,
-					queue, action_conf->queue_num, true);
+					queue, action_conf->queue_num,
+					true /* standalone */,
+					dev_started /* ref_new_qs */,
+					dev_started /* deref_old_qs */);
 	if (ret) {
 		mlx5_free(queue);
 		ret = rte_flow_error_set(error, rte_errno,
diff --git a/drivers/net/mlx5/mlx5_rx.h b/drivers/net/mlx5/mlx5_rx.h
index 283242f530..f10eee406b 100644
--- a/drivers/net/mlx5/mlx5_rx.h
+++ b/drivers/net/mlx5/mlx5_rx.h
@@ -233,7 +233,8 @@ int mlx5_ind_table_obj_setup(struct rte_eth_dev *dev,
 int mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 			      struct mlx5_ind_table_obj *ind_tbl,
 			      uint16_t *queues, const uint32_t queues_n,
-			      bool standalone);
+			      bool standalone,
+			      bool ref_new_qs, bool deref_old_qs);
 int mlx5_ind_table_obj_attach(struct rte_eth_dev *dev,
 			      struct mlx5_ind_table_obj *ind_tbl);
 int mlx5_ind_table_obj_detach(struct rte_eth_dev *dev,
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 2dd9490c36..dadcd0825d 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2387,6 +2387,10 @@ mlx5_ind_table_obj_check_standalone(struct rte_eth_dev *dev __rte_unused,
  *   Number of queues in the array.
  * @param standalone
  *   Indirection table for Standalone queue.
+ * @param ref_new_qs
+ *   Whether to increment new RxQ set reference counters.
+ * @param deref_old_qs
+ *   Whether to decrement old RxQ set reference counters.
  *
  * @return
  *   0 on success, a negative errno value otherwise and rte_errno is set.
@@ -2395,10 +2399,10 @@ int
 mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 			  struct mlx5_ind_table_obj *ind_tbl,
 			  uint16_t *queues, const uint32_t queues_n,
-			  bool standalone)
+			  bool standalone, bool ref_new_qs, bool deref_old_qs)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
-	unsigned int i;
+	unsigned int i = 0, j;
 	int ret = 0, err;
 	const unsigned int n = rte_is_power_of_2(queues_n) ?
 			       log2above(queues_n) :
@@ -2408,22 +2412,30 @@ mlx5_ind_table_obj_modify(struct rte_eth_dev *dev,
 	RTE_SET_USED(standalone);
 	if (mlx5_ind_table_obj_check_standalone(dev, ind_tbl) < 0)
 		return -rte_errno;
-	for (i = 0; i != queues_n; ++i) {
-		if (!mlx5_rxq_get(dev, queues[i])) {
-			ret = -rte_errno;
-			goto error;
+	if (ref_new_qs)
+		for (i = 0; i != queues_n; ++i) {
+			if (!mlx5_rxq_ref(dev, queues[i])) {
+				ret = -rte_errno;
+				goto error;
+			}
 		}
-	}
 	MLX5_ASSERT(priv->obj_ops.ind_table_modify);
 	ret = priv->obj_ops.ind_table_modify(dev, n, queues, queues_n, ind_tbl);
 	if (ret)
 		goto error;
+	if (deref_old_qs)
+		for (i = 0; i < ind_tbl->queues_n; i++)
+			claim_nonzero(mlx5_rxq_deref(dev, ind_tbl->queues[i]));
 	ind_tbl->queues_n = queues_n;
 	ind_tbl->queues = queues;
 	return 0;
 error:
-	err = rte_errno;
-	rte_errno = err;
+	if (ref_new_qs) {
+		err = rte_errno;
+		for (j = 0; j < i; j++)
+			mlx5_rxq_deref(dev, queues[j]);
+		rte_errno = err;
+	}
 	DRV_LOG(DEBUG, "Port %u cannot setup indirection table.",
 		dev->data->port_id);
 	return ret;
@@ -2444,19 +2456,17 @@ int
 mlx5_ind_table_obj_attach(struct rte_eth_dev *dev,
 			  struct mlx5_ind_table_obj *ind_tbl)
 {
-	unsigned int i;
 	int ret;
 
 	ret = mlx5_ind_table_obj_modify(dev, ind_tbl, ind_tbl->queues,
-					ind_tbl->queues_n, true);
-	if (ret != 0) {
+					ind_tbl->queues_n,
+					true /* standalone */,
+					true /* ref_new_qs */,
+					false /* deref_old_qs */);
+	if (ret != 0)
 		DRV_LOG(ERR, "Port %u could not modify indirect table obj %p",
 			dev->data->port_id, (void *)ind_tbl);
-		return ret;
-	}
-	for (i = 0; i < ind_tbl->queues_n; i++)
-		mlx5_rxq_ref(dev, ind_tbl->queues[i]);
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.25.1

RE: [PATCH v3 0/2] net/mlx5: fix indirect RSS reference counting

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Sent: Wednesday, November 24, 2021 11:40 AM
> To: dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@nvidia.com>
> Subject: [PATCH v3 0/2] net/mlx5: fix indirect RSS reference counting
> 
> v3:
>   * No changes, slipped when sending v2.
> v2:
>   * Fix build with GCc on RHEL7 (CI).
>   * Fix RxQ leak when an indirect RSS had been created before the start
>     and then modified after the start (Matan).
> 
> Dmitry Kozlyuk (2):
>   net/mlx5: fix indirect RSS creation when port is stopped
>   net/mlx5: fix RxQ reference counting for indirect RSS
> 
>  drivers/net/mlx5/mlx5_flow_dv.c |  9 +++-
>  drivers/net/mlx5/mlx5_rx.h      |  6 ++-
>  drivers/net/mlx5/mlx5_rxq.c     | 85 ++++++++++++++++++++-------------
>  3 files changed, 64 insertions(+), 36 deletions(-)
> 
> --
> 2.25.1
Series applied to next-net-mlx,


Kindest regards,
Raslan Darawsheh