[dpdk-dev] [PATCH v1] net/mlx4: fix Rx resource leak in case of error

[dpdk-dev] [PATCH v1] net/mlx4: fix Rx resource leak in case of error

[Adrien Mazarguil]

When creation of a flow rule fails during dev_start(), the usage count of
the common RSS context is not decremented, which triggers an assertion
failure in debug mode during dev_close().

This is addressed by tracking the initialization status of the common RSS
context in order to add missing cleanup code.

A similar issue exists in mlx4_rxq_attach(), where usage count is
incremented on a Rx queue but not released in case of error. This may lead
to the above issue since RSS contexts created by flow rules attach
themselves to Rx queues, incrementing their usage count.

Fixes: 5697a4142107 ("net/mlx4: relax Rx queue configuration order")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx4/mlx4.c     | 6 ++++--
 drivers/net/mlx4/mlx4.h     | 1 +
 drivers/net/mlx4/mlx4_rxq.c | 7 +++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 06f17703b..937074a4f 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -61,6 +61,8 @@ const char *pmd_mlx4_init_params[] = {
 	NULL,
 };
 
+static void mlx4_dev_stop(struct rte_eth_dev *dev);
+
 /**
  * DPDK callback for Ethernet device configuration.
  *
@@ -143,8 +145,7 @@ mlx4_dev_start(struct rte_eth_dev *dev)
 	dev->rx_pkt_burst = mlx4_rx_burst;
 	return 0;
 err:
-	/* Rollback. */
-	priv->started = 0;
+	mlx4_dev_stop(dev);
 	return ret;
 }
 
@@ -194,6 +195,7 @@ mlx4_dev_close(struct rte_eth_dev *dev)
 	dev->tx_pkt_burst = mlx4_tx_burst_removed;
 	rte_wmb();
 	mlx4_flow_clean(priv);
+	mlx4_rss_deinit(priv);
 	for (i = 0; i != dev->data->nb_rx_queues; ++i)
 		mlx4_rx_queue_release(dev->data->rx_queues[i]);
 	for (i = 0; i != dev->data->nb_tx_queues; ++i)
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index 45846554b..415b7d40f 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -103,6 +103,7 @@ struct priv {
 	uint32_t vf:1; /**< This is a VF device. */
 	uint32_t intr_alarm:1; /**< An interrupt alarm is scheduled. */
 	uint32_t isolated:1; /**< Toggle isolated mode. */
+	uint32_t rss_init:1; /**< Common RSS context is initialized. */
 	uint32_t hw_csum:1; /**< Checksum offload is supported. */
 	uint32_t hw_csum_l2tun:1; /**< Checksum support for L2 tunnels. */
 	uint32_t hw_fcs_strip:1; /**< FCS stripping toggling is supported. */
diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c
index 6be6a0b9a..a7acc047b 100644
--- a/drivers/net/mlx4/mlx4_rxq.c
+++ b/drivers/net/mlx4/mlx4_rxq.c
@@ -336,6 +336,8 @@ mlx4_rss_init(struct priv *priv)
 	unsigned int i;
 	int ret;
 
+	if (priv->rss_init)
+		return 0;
 	/* Prepare range for RSS contexts before creating the first WQ. */
 	ret = mlx4_glue->dv_set_context_attr
 		(priv->ctx,
@@ -418,6 +420,7 @@ mlx4_rss_init(struct priv *priv)
 		}
 		wq_num_prev = wq_num;
 	}
+	priv->rss_init = 1;
 	return 0;
 error:
 	ERROR("cannot initialize common RSS resources (queue %u): %s: %s",
@@ -446,6 +449,8 @@ mlx4_rss_deinit(struct priv *priv)
 {
 	unsigned int i;
 
+	if (!priv->rss_init)
+		return;
 	for (i = 0; i != priv->dev->data->nb_rx_queues; ++i) {
 		struct rxq *rxq = priv->dev->data->rx_queues[i];
 
@@ -454,6 +459,7 @@ mlx4_rss_deinit(struct priv *priv)
 			mlx4_rxq_detach(rxq);
 		}
 	}
+	priv->rss_init = 0;
 }
 
 /**
@@ -606,6 +612,7 @@ mlx4_rxq_attach(struct rxq *rxq)
 		claim_zero(mlx4_glue->destroy_wq(wq));
 	if (cq)
 		claim_zero(mlx4_glue->destroy_cq(cq));
+	--rxq->usecnt;
 	rte_errno = ret;
 	ERROR("error while attaching Rx queue %p: %s: %s",
 	      (void *)rxq, msg, strerror(ret));
-- 
2.11.0

[dpdk-dev] [PATCH v2 1/3] net/mlx4: fix Rx resource leak in case of error

[Adrien Mazarguil]

When creation of a flow rule fails during dev_start(), the usage count of
the common RSS context is not decremented, which triggers an assertion
failure in debug mode during dev_close().

This is addressed by tracking the initialization status of the common RSS
context in order to add missing cleanup code.

A similar issue exists in mlx4_rxq_attach(), where usage count is
incremented on a Rx queue but not released in case of error. This may lead
to the above issue since RSS contexts created by flow rules attach
themselves to Rx queues, incrementing their usage count.

Fixes: 5697a4142107 ("net/mlx4: relax Rx queue configuration order")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx4/mlx4.c     | 6 ++++--
 drivers/net/mlx4/mlx4.h     | 1 +
 drivers/net/mlx4/mlx4_rxq.c | 7 +++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 970d20dd1..3dd72dbf5 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -61,6 +61,8 @@ const char *pmd_mlx4_init_params[] = {
 	NULL,
 };
 
+static void mlx4_dev_stop(struct rte_eth_dev *dev);
+
 /**
  * DPDK callback for Ethernet device configuration.
  *
@@ -143,8 +145,7 @@ mlx4_dev_start(struct rte_eth_dev *dev)
 	dev->rx_pkt_burst = mlx4_rx_burst;
 	return 0;
 err:
-	/* Rollback. */
-	priv->started = 0;
+	mlx4_dev_stop(dev);
 	return ret;
 }
 
@@ -194,6 +195,7 @@ mlx4_dev_close(struct rte_eth_dev *dev)
 	dev->tx_pkt_burst = mlx4_tx_burst_removed;
 	rte_wmb();
 	mlx4_flow_clean(priv);
+	mlx4_rss_deinit(priv);
 	for (i = 0; i != dev->data->nb_rx_queues; ++i)
 		mlx4_rx_queue_release(dev->data->rx_queues[i]);
 	for (i = 0; i != dev->data->nb_tx_queues; ++i)
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index 45846554b..415b7d40f 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -103,6 +103,7 @@ struct priv {
 	uint32_t vf:1; /**< This is a VF device. */
 	uint32_t intr_alarm:1; /**< An interrupt alarm is scheduled. */
 	uint32_t isolated:1; /**< Toggle isolated mode. */
+	uint32_t rss_init:1; /**< Common RSS context is initialized. */
 	uint32_t hw_csum:1; /**< Checksum offload is supported. */
 	uint32_t hw_csum_l2tun:1; /**< Checksum support for L2 tunnels. */
 	uint32_t hw_fcs_strip:1; /**< FCS stripping toggling is supported. */
diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c
index b430678c7..65f099423 100644
--- a/drivers/net/mlx4/mlx4_rxq.c
+++ b/drivers/net/mlx4/mlx4_rxq.c
@@ -336,6 +336,8 @@ mlx4_rss_init(struct priv *priv)
 	unsigned int i;
 	int ret;
 
+	if (priv->rss_init)
+		return 0;
 	/* Prepare range for RSS contexts before creating the first WQ. */
 	ret = mlx4_glue->dv_set_context_attr
 		(priv->ctx,
@@ -418,6 +420,7 @@ mlx4_rss_init(struct priv *priv)
 		}
 		wq_num_prev = wq_num;
 	}
+	priv->rss_init = 1;
 	return 0;
 error:
 	ERROR("cannot initialize common RSS resources (queue %u): %s: %s",
@@ -446,6 +449,8 @@ mlx4_rss_deinit(struct priv *priv)
 {
 	unsigned int i;
 
+	if (!priv->rss_init)
+		return;
 	for (i = 0; i != priv->dev->data->nb_rx_queues; ++i) {
 		struct rxq *rxq = priv->dev->data->rx_queues[i];
 
@@ -454,6 +459,7 @@ mlx4_rss_deinit(struct priv *priv)
 			mlx4_rxq_detach(rxq);
 		}
 	}
+	priv->rss_init = 0;
 }
 
 /**
@@ -606,6 +612,7 @@ mlx4_rxq_attach(struct rxq *rxq)
 		claim_zero(mlx4_glue->destroy_wq(wq));
 	if (cq)
 		claim_zero(mlx4_glue->destroy_cq(cq));
+	--rxq->usecnt;
 	rte_errno = ret;
 	ERROR("error while attaching Rx queue %p: %s: %s",
 	      (void *)rxq, msg, strerror(ret));
-- 
2.11.0

[dpdk-dev] [PATCH v2 2/3] net/mlx4: fix default RSS hash fields

[Adrien Mazarguil]

Using special types value -1 with mlx4_conv_rss_types() is supposed to
return a supported set of Verbs RSS hash fields, that is, priv->hw_rss_sup
unmodified.

Due to the way this function is written and because it is also used to
initially populate priv->hw_rss_sup however, this special value works
properly only once and fails with ENOTSUP errors afterward.

This problem can be seen when re-creating default flows (e.g. by entering
and leaving isolated mode).

Fixes: 024e87bef40b ("net/mlx4: restore UDP RSS by probing capabilities")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx4/mlx4.c      | 15 +++++++--------
 drivers/net/mlx4/mlx4_flow.c | 13 ++++---------
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 3dd72dbf5..4e472fa1d 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -569,14 +569,13 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		if (!priv->hw_rss_sup) {
 			WARN("no RSS capabilities reported; disabling support"
 			     " for UDP RSS and inner VXLAN RSS");
-			/* Fake support for all possible RSS hash fields. */
-			priv->hw_rss_sup = ~UINT64_C(0);
-			priv->hw_rss_sup = mlx4_conv_rss_types(priv, -1);
-			/* Filter out known unsupported fields. */
-			priv->hw_rss_sup &=
-				~(uint64_t)(IBV_RX_HASH_SRC_PORT_UDP |
-					    IBV_RX_HASH_DST_PORT_UDP |
-					    IBV_RX_HASH_INNER);
+			priv->hw_rss_sup =
+				IBV_RX_HASH_SRC_IPV4 |
+				IBV_RX_HASH_DST_IPV4 |
+				IBV_RX_HASH_SRC_IPV6 |
+				IBV_RX_HASH_DST_IPV6 |
+				IBV_RX_HASH_SRC_PORT_TCP |
+				IBV_RX_HASH_DST_PORT_TCP;
 		}
 		DEBUG("supported RSS hash fields mask: %016" PRIx64,
 		      priv->hw_rss_sup);
diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index e3d7aa8ef..bebad074e 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -125,20 +125,15 @@ mlx4_conv_rss_types(struct priv *priv, uint64_t types)
 	uint64_t conv = 0;
 	unsigned int i;
 
+	if (types == (uint64_t)-1)
+		return priv->hw_rss_sup;
 	for (i = 0; i != RTE_DIM(in); ++i)
 		if (types & in[i]) {
 			seen |= types & in[i];
 			conv |= out[i];
 		}
-	if ((conv & priv->hw_rss_sup) == conv) {
-		if (types == (uint64_t)-1) {
-			/* Include inner RSS by default if supported. */
-			conv |= priv->hw_rss_sup & IBV_RX_HASH_INNER;
-			return conv;
-		}
-		if (!(types & ~seen))
-			return conv;
-	}
+	if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen))
+		return conv;
 	rte_errno = ENOTSUP;
 	return (uint64_t)-1;
 }
-- 
2.11.0

[dpdk-dev] [PATCH v2 3/3] net/mlx4: fix inner RSS support for broken kernels

[Adrien Mazarguil]

Linux 4.15 and 4.16 may report inner RSS as a supported capability of the
device, however it can't be used due to missing code in the kernel.

This triggers an error when creating the default hash QP and prevents this
PMD from starting up without a prior call to rte_flow_isolate().

Fixes: 55e8991e3199 ("net/mlx4: restore inner VXLAN RSS support")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx4/mlx4.c | 107 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 95 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 4e472fa1d..c1cc95786 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -387,6 +387,99 @@ mlx4_args(struct rte_devargs *devargs, struct mlx4_conf *conf)
 	return ret;
 }
 
+/**
+ * Interpret RSS capabilities reported by device.
+ *
+ * This function returns the set of usable Verbs RSS hash fields, kernel
+ * quirks taken into account.
+ *
+ * @param ctx
+ *   Verbs context.
+ * @param pd
+ *   Verbs protection domain.
+ * @param device_attr_ex
+ *   Extended device attributes to interpret.
+ *
+ * @return
+ *   Usable RSS hash fields mask in Verbs format.
+ */
+static uint64_t
+mlx4_hw_rss_sup(struct ibv_context *ctx, struct ibv_pd *pd,
+		struct ibv_device_attr_ex *device_attr_ex)
+{
+	uint64_t hw_rss_sup = device_attr_ex->rss_caps.rx_hash_fields_mask;
+	struct ibv_cq *cq = NULL;
+	struct ibv_wq *wq = NULL;
+	struct ibv_rwq_ind_table *ind = NULL;
+	struct ibv_qp *qp = NULL;
+
+	if (!hw_rss_sup) {
+		WARN("no RSS capabilities reported; disabling support for UDP"
+		     " RSS and inner VXLAN RSS");
+		return IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4 |
+			IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6 |
+			IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP;
+	}
+	if (!(hw_rss_sup & IBV_RX_HASH_INNER))
+		return hw_rss_sup;
+	/*
+	 * Although reported as supported, missing code in some Linux
+	 * versions (v4.15, v4.16) prevents the creation of hash QPs with
+	 * inner capability.
+	 *
+	 * There is no choice but to attempt to instantiate a temporary RSS
+	 * context in order to confirm its support.
+	 */
+	cq = mlx4_glue->create_cq(ctx, 1, NULL, NULL, 0);
+	wq = cq ? mlx4_glue->create_wq
+		(ctx,
+		 &(struct ibv_wq_init_attr){
+			.wq_type = IBV_WQT_RQ,
+			.max_wr = 1,
+			.max_sge = 1,
+			.pd = pd,
+			.cq = cq,
+		 }) : NULL;
+	ind = wq ? mlx4_glue->create_rwq_ind_table
+		(ctx,
+		 &(struct ibv_rwq_ind_table_init_attr){
+			.log_ind_tbl_size = 0,
+			.ind_tbl = &wq,
+			.comp_mask = 0,
+		 }) : NULL;
+	qp = ind ? mlx4_glue->create_qp_ex
+		(ctx,
+		 &(struct ibv_qp_init_attr_ex){
+			.comp_mask =
+				(IBV_QP_INIT_ATTR_PD |
+				 IBV_QP_INIT_ATTR_RX_HASH |
+				 IBV_QP_INIT_ATTR_IND_TABLE),
+			.qp_type = IBV_QPT_RAW_PACKET,
+			.pd = pd,
+			.rwq_ind_tbl = ind,
+			.rx_hash_conf = {
+				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
+				.rx_hash_key_len = MLX4_RSS_HASH_KEY_SIZE,
+				.rx_hash_key = mlx4_rss_hash_key_default,
+				.rx_hash_fields_mask = hw_rss_sup,
+			},
+		 }) : NULL;
+	if (!qp) {
+		WARN("disabling unusable inner RSS capability due to kernel"
+		     " quirk");
+		hw_rss_sup &= ~IBV_RX_HASH_INNER;
+	} else {
+		claim_zero(ibv_destroy_qp(qp));
+	}
+	if (ind)
+		claim_zero(ibv_destroy_rwq_ind_table(ind));
+	if (wq)
+		claim_zero(ibv_destroy_wq(wq));
+	if (cq)
+		claim_zero(ibv_destroy_cq(cq));
+	return hw_rss_sup;
+}
+
 static struct rte_pci_driver mlx4_driver;
 
 /**
@@ -565,18 +658,8 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			 PCI_DEVICE_ID_MELLANOX_CONNECTX3PRO);
 		DEBUG("L2 tunnel checksum offloads are %ssupported",
 		      priv->hw_csum_l2tun ? "" : "not ");
-		priv->hw_rss_sup = device_attr_ex.rss_caps.rx_hash_fields_mask;
-		if (!priv->hw_rss_sup) {
-			WARN("no RSS capabilities reported; disabling support"
-			     " for UDP RSS and inner VXLAN RSS");
-			priv->hw_rss_sup =
-				IBV_RX_HASH_SRC_IPV4 |
-				IBV_RX_HASH_DST_IPV4 |
-				IBV_RX_HASH_SRC_IPV6 |
-				IBV_RX_HASH_DST_IPV6 |
-				IBV_RX_HASH_SRC_PORT_TCP |
-				IBV_RX_HASH_DST_PORT_TCP;
-		}
+		priv->hw_rss_sup = mlx4_hw_rss_sup(priv->ctx, priv->pd,
+						   &device_attr_ex);
 		DEBUG("supported RSS hash fields mask: %016" PRIx64,
 		      priv->hw_rss_sup);
 		priv->hw_fcs_strip = !!(device_attr_ex.raw_packet_caps &
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2 1/3] net/mlx4: fix Rx resource leak in case of error

[Shahaf Shuler]

Thursday, April 26, 2018 7:26 PM, Adrien Mazarguil:
> Subject: [PATCH v2 1/3] net/mlx4: fix Rx resource leak in case of error
> 
> When creation of a flow rule fails during dev_start(), the usage count of the
> common RSS context is not decremented, which triggers an assertion failure
> in debug mode during dev_close().
> 
> This is addressed by tracking the initialization status of the common RSS
> context in order to add missing cleanup code.
> 
> A similar issue exists in mlx4_rxq_attach(), where usage count is
> incremented on a Rx queue but not released in case of error. This may lead
> to the above issue since RSS contexts created by flow rules attach
> themselves to Rx queues, incrementing their usage count.
> 
> Fixes: 5697a4142107 ("net/mlx4: relax Rx queue configuration order")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Series applied to next-net-mlx, thanks.