[dpdk-dev] [PATCH 1/2] net/mlx5: fix flow queues array allocation

[dpdk-dev] [PATCH 1/2] net/mlx5: fix flow queues array allocation

[Nelio Laranjeiro]

Flow queues array offset is not properly computed causing all sorts of
issues.  Address it by making the rte_flow structure less complex.

Fixes: 360663e1df46 ("net/mlx5: prepare support for RSS action rule")

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx5/mlx5_flow.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 3691e95..c735c43 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -98,11 +98,11 @@ struct rte_flow {
 	struct ibv_exp_flow *ibv_flow; /**< Verbs flow. */
 	struct ibv_exp_wq *wq; /**< Verbs work queue. */
 	struct ibv_cq *cq; /**< Verbs completion queue. */
-	struct rxq *(*rxqs)[]; /**< Pointer to the queues array. */
 	uint16_t rxqs_n; /**< Number of queues in this flow, 0 if drop queue. */
 	uint32_t mark:1; /**< Set if the flow is marked. */
 	uint32_t drop:1; /**< Drop queue. */
 	uint64_t hash_fields; /**< Fields that participate in the hash. */
+	struct rxq *rxqs[]; /**< Pointer to the queues array. */
 };
 
 /** Static initializer for items. */
@@ -1038,24 +1038,20 @@ priv_flow_create_action_queue(struct priv *priv,
 	assert(priv->pd);
 	assert(priv->ctx);
 	assert(!action->drop);
-	rte_flow = rte_calloc(__func__, 1,
-			      sizeof(*rte_flow) + sizeof(struct rxq *) *
-			      action->queues_n, 0);
+	rte_flow = rte_calloc(__func__, 1, sizeof(*rte_flow) +
+			      sizeof(*rte_flow->rxqs) * action->queues_n, 0);
 	if (!rte_flow) {
 		rte_flow_error_set(error, ENOMEM, RTE_FLOW_ERROR_TYPE_HANDLE,
 				   NULL, "cannot allocate flow memory");
 		return NULL;
 	}
-	rte_flow->rxqs = (struct rxq *(*)[])((uintptr_t)rte_flow +
-					     sizeof(struct rxq *) *
-					     action->queues_n);
 	for (i = 0; i < action->queues_n; ++i) {
 		struct rxq_ctrl *rxq;
 
 		rxq = container_of((*priv->rxqs)[action->queues[i]],
 				   struct rxq_ctrl, rxq);
 		wqs[i] = rxq->wq;
-		(*rte_flow->rxqs)[i] = &rxq->rxq;
+		rte_flow->rxqs[i] = &rxq->rxq;
 		++rte_flow->rxqs_n;
 		rxq->rxq.mark |= action->mark;
 	}
@@ -1265,7 +1261,7 @@ priv_flow_destroy(struct priv *priv,
 		 * present in any other marked flow (RSS or not).
 		 */
 		for (queue_n = 0; queue_n < flow->rxqs_n; ++queue_n) {
-			rxq = (*flow->rxqs)[queue_n];
+			rxq = flow->rxqs[queue_n];
 			for (tmp = LIST_FIRST(&priv->flows);
 			     tmp;
 			     tmp = LIST_NEXT(tmp, next)) {
@@ -1278,7 +1274,7 @@ priv_flow_destroy(struct priv *priv,
 				     ++tqueue_n) {
 					struct rxq *trxq;
 
-					trxq = (*tmp->rxqs)[tqueue_n];
+					trxq = tmp->rxqs[tqueue_n];
 					if (rxq == trxq)
 						++mark_n;
 				}
@@ -1489,7 +1485,7 @@ priv_flow_stop(struct priv *priv)
 			unsigned int n;
 
 			for (n = 0; n < flow->rxqs_n; ++n)
-				(*flow->rxqs)[n]->mark = 0;
+				flow->rxqs[n]->mark = 0;
 		}
 		DEBUG("Flow %p removed", (void *)flow);
 	}
@@ -1534,7 +1530,7 @@ priv_flow_start(struct priv *priv)
 			unsigned int n;
 
 			for (n = 0; n < flow->rxqs_n; ++n)
-				(*flow->rxqs)[n]->mark = 1;
+				flow->rxqs[n]->mark = 1;
 		}
 	}
 	return 0;
-- 
2.1.4

[dpdk-dev] [PATCH 2/2] net/mlx5: panic when destroying a queue in use

[Nelio Laranjeiro]

Since the queue release API does not allow failures (return value is void)
and the flow API does not allow a queue to be released as long as a flow
rule depends on it, the only rational decision to avoid undefined behavior
is to panic in this situation.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx5/mlx5.h      |  1 +
 drivers/net/mlx5/mlx5_flow.c | 31 +++++++++++++++++++++++++++++++
 drivers/net/mlx5/mlx5_rxq.c  |  4 ++++
 3 files changed, 36 insertions(+)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index e8bf361..71e19ec 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -313,5 +313,6 @@ int mlx5_flow_destroy(struct rte_eth_dev *, struct rte_flow *,
 int mlx5_flow_flush(struct rte_eth_dev *, struct rte_flow_error *);
 int priv_flow_start(struct priv *);
 void priv_flow_stop(struct priv *);
+int priv_flow_rxq_in_use(struct priv *, struct rxq *);
 
 #endif /* RTE_PMD_MLX5_H_ */
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index c735c43..8d62f85 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -1535,3 +1535,34 @@ priv_flow_start(struct priv *priv)
 	}
 	return 0;
 }
+
+/**
+ * Verify if the Rx queue is used in a flow.
+ *
+ * @param priv
+ *   Pointer to private structure.
+ * @param rxq
+ *   Pointer to the queue to search.
+ *
+ * @return
+ *   Nonzero if the queue is used by a flow.
+ */
+int
+priv_flow_rxq_in_use(struct priv *priv, struct rxq *rxq)
+{
+	struct rte_flow *flow;
+
+	for (flow = LIST_FIRST(&priv->flows);
+	     flow;
+	     flow = LIST_NEXT(flow, next)) {
+		unsigned int n;
+
+		if (flow->drop)
+			continue;
+		for (n = 0; n < flow->rxqs_n; ++n) {
+			if (flow->rxqs[n] == rxq)
+				return 1;
+		}
+	}
+	return 0;
+}
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 5566482..8b78233 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -59,6 +59,7 @@
 #include <rte_ethdev.h>
 #include <rte_common.h>
 #include <rte_interrupts.h>
+#include <rte_debug.h>
 #ifdef PEDANTIC
 #pragma GCC diagnostic error "-Wpedantic"
 #endif
@@ -1248,6 +1249,9 @@ mlx5_rx_queue_release(void *dpdk_rxq)
 	rxq_ctrl = container_of(rxq, struct rxq_ctrl, rxq);
 	priv = rxq_ctrl->priv;
 	priv_lock(priv);
+	if (priv_flow_rxq_in_use(priv, rxq))
+		rte_panic("Rx queue %p is still used by a flow and cannot be"
+			  " removed\n", (void *)rxq_ctrl);
 	for (i = 0; (i != priv->rxqs_n); ++i)
 		if ((*priv->rxqs)[i] == rxq) {
 			DEBUG("%p: removing RX queue %p from list",
-- 
2.1.4

Re: [dpdk-dev] [PATCH 2/2] net/mlx5: panic when destroying a queue in use

[Ferruh Yigit]

On 4/11/2017 4:21 PM, Nelio Laranjeiro wrote:
> Since the queue release API does not allow failures (return value is void)
> and the flow API does not allow a queue to be released as long as a flow
> rule depends on it, the only rational decision to avoid undefined behavior
> is to panic in this situation.
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

<...>

> @@ -1248,6 +1249,9 @@ mlx5_rx_queue_release(void *dpdk_rxq)
>  	rxq_ctrl = container_of(rxq, struct rxq_ctrl, rxq);
>  	priv = rxq_ctrl->priv;
>  	priv_lock(priv);
> +	if (priv_flow_rxq_in_use(priv, rxq))
> +		rte_panic("Rx queue %p is still used by a flow and cannot be"
> +			  " removed\n", (void *)rxq_ctrl);

Actually, this adds exit code to the PMD, not sure this is good idea.

There was a patch to remove them from libraries. The exit decision
should be belong to the application, not to the PMD, what do you think?

>  	for (i = 0; (i != priv->rxqs_n); ++i)
>  		if ((*priv->rxqs)[i] == rxq) {
>  			DEBUG("%p: removing RX queue %p from list",
> 

Re: [dpdk-dev] [PATCH 2/2] net/mlx5: panic when destroying a queue in use

[Adrien Mazarguil]

On Wed, Apr 12, 2017 at 05:48:40PM +0100, Ferruh Yigit wrote:
> On 4/11/2017 4:21 PM, Nelio Laranjeiro wrote:
> > Since the queue release API does not allow failures (return value is void)
> > and the flow API does not allow a queue to be released as long as a flow
> > rule depends on it, the only rational decision to avoid undefined behavior
> > is to panic in this situation.
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> <...>
> 
> > @@ -1248,6 +1249,9 @@ mlx5_rx_queue_release(void *dpdk_rxq)
> >  	rxq_ctrl = container_of(rxq, struct rxq_ctrl, rxq);
> >  	priv = rxq_ctrl->priv;
> >  	priv_lock(priv);
> > +	if (priv_flow_rxq_in_use(priv, rxq))
> > +		rte_panic("Rx queue %p is still used by a flow and cannot be"
> > +			  " removed\n", (void *)rxq_ctrl);
> 
> Actually, this adds exit code to the PMD, not sure this is good idea.
> 
> There was a patch to remove them from libraries. The exit decision
> should be belong to the application, not to the PMD, what do you think?

I think rte_panic() makes sense in such cases. It's a voluntary crash
similar to assert() (BUG_ON() for kernel folks) to avoids memory corruption
and other nasty things which unfortunately can occur when applications
happen to do things in the wrong order.

In this specific case, it is caused by shortcomings in the current API,
namely the lack of a return value for this function, which cannot be changed
at this stage. This is not a fix as there is not really a commit to blame.

> 
> >  	for (i = 0; (i != priv->rxqs_n); ++i)
> >  		if ((*priv->rxqs)[i] == rxq) {
> >  			DEBUG("%p: removing RX queue %p from list",
> > 
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH 1/2] net/mlx5: fix flow queues array allocation

[Ferruh Yigit]

On 4/11/2017 4:21 PM, Nelio Laranjeiro wrote:
> Flow queues array offset is not properly computed causing all sorts of
> issues.  Address it by making the rte_flow structure less complex.
> 
> Fixes: 360663e1df46 ("net/mlx5: prepare support for RSS action rule")
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Series applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH 1/2] net/mlx5: fix flow queues array allocation

[Ferruh Yigit]

On 4/12/2017 4:02 PM, Ferruh Yigit wrote:
> On 4/11/2017 4:21 PM, Nelio Laranjeiro wrote:
>> Flow queues array offset is not properly computed causing all sorts of
>> issues.  Address it by making the rte_flow structure less complex.
>>
>> Fixes: 360663e1df46 ("net/mlx5: prepare support for RSS action rule")
>>
>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Series applied to dpdk-next-net/master, thanks.

Removed from tree, and patchwork status updated to "New", please check
comment about rte_panic() usage in patch 2/2.

Thanks,
ferruh

Re: [dpdk-dev] [PATCH 1/2] net/mlx5: fix flow queues array allocation

[Ferruh Yigit]

On 4/12/2017 5:51 PM, Ferruh Yigit wrote:
> On 4/12/2017 4:02 PM, Ferruh Yigit wrote:
>> On 4/11/2017 4:21 PM, Nelio Laranjeiro wrote:
>>> Flow queues array offset is not properly computed causing all sorts of
>>> issues.  Address it by making the rte_flow structure less complex.
>>>
>>> Fixes: 360663e1df46 ("net/mlx5: prepare support for RSS action rule")
>>>
>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>
>> Series applied to dpdk-next-net/master, thanks.
> 
> Removed from tree, and patchwork status updated to "New", please check
> comment about rte_panic() usage in patch 2/2.

Series applied _back_ to dpdk-next-net/master, thanks.