[dpdk-dev] [PATCH 3/7] net/mlx4: fix Rx interrupts management

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

This commit addresses various issues that may lead to undefined behavior
when configuring Rx interrupts.

While failure to create a Rx queue completion channel in rxq_setup()
prevents that queue from being created, existing queues still have theirs.
Since the error handler disables dev_conf.intr_conf.rxq as well, subsequent
calls to rxq_setup() create Rx queues without interrupts. This leads to a
scenario where not all Rx queues support interrupts; missing checks on the
presence of completion channels may crash the application.

Considering that the PMD is not supposed to disable user-provided
configuration parameters (dev_conf.intr_conf.rxq), and that these can
change for subsequent rxq_setup() calls anyway, properly supporting a mixed
mode where not all Rx queues have interrupts enabled is a better approach.

To do so with a minimum set of changes, priv_intr_efd_enable() and
priv_create_intr_vec() are first refactored as a single
priv_rx_intr_vec_enable() function (same for their "disable" counterparts).
Since they had to be used together, there was no point in keeping them
separate.

Remaining changes:

- Always clean up before reconfiguring interrupts to avoid memory leaks.
- Always clean up when closing the device.
- Use malloc()/free() instead of their rte_*() counterparts since there is
  no need to store the vector in huge pages-backed memory.
- Allow more Rx queues than the size of the event file descriptor array as
  long as Rx interrupts are not requested on all of them.
- Properly clean up interrupt handle when disabling Rx interrupts (nb_efd
  and intr_vec reset to 0).
- Check completion channel presence while toggling Rx interrupts on a given
  queue.

Fixes: 9f05a4b81809 ("net/mlx4: support user space Rx interrupt event")

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Moti Haimovsky <motih@mellanox.com>
---
 drivers/net/mlx4/mlx4.c | 166 +++++++++++++++++++------------------------
 1 file changed, 72 insertions(+), 94 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 2b4722f..050d646 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -135,16 +135,10 @@ static int
 mlx4_rx_intr_disable(struct rte_eth_dev *dev, uint16_t idx);
 
 static int
-priv_intr_efd_enable(struct priv *priv);
+priv_rx_intr_vec_enable(struct priv *priv);
 
 static void
-priv_intr_efd_disable(struct priv *priv);
-
-static int
-priv_create_intr_vec(struct priv *priv);
-
-static void
-priv_destroy_intr_vec(struct priv *priv);
+priv_rx_intr_vec_disable(struct priv *priv);
 
 /**
  * Check if running as a secondary process.
@@ -3720,9 +3714,9 @@ rxq_setup(struct rte_eth_dev *dev, struct rxq *rxq, uint16_t desc,
 	if (dev->data->dev_conf.intr_conf.rxq) {
 		tmpl.channel = ibv_create_comp_channel(priv->ctx);
 		if (tmpl.channel == NULL) {
-			dev->data->dev_conf.intr_conf.rxq = 0;
 			ret = ENOMEM;
-			ERROR("%p: Comp Channel creation failure: %s",
+			ERROR("%p: Rx interrupt completion channel creation"
+			      " failure: %s",
 			      (void *)dev, strerror(ret));
 			goto error;
 		}
@@ -4037,10 +4031,11 @@ mlx4_dev_start(struct rte_eth_dev *dev)
 		     (void *)dev);
 		goto err;
 	}
-	if (dev->data->dev_conf.intr_conf.rxq) {
-		ret = priv_intr_efd_enable(priv);
-		if (!ret)
-			ret = priv_create_intr_vec(priv);
+	ret = priv_rx_intr_vec_enable(priv);
+	if (ret) {
+		ERROR("%p: Rx interrupt vector creation failed",
+		      (void *)dev);
+		goto err;
 	}
 	ret = mlx4_priv_flow_start(priv);
 	if (ret) {
@@ -4234,10 +4229,7 @@ mlx4_dev_close(struct rte_eth_dev *dev)
 		assert(priv->ctx == NULL);
 	priv_dev_removal_interrupt_handler_uninstall(priv, dev);
 	priv_dev_link_interrupt_handler_uninstall(priv, dev);
-	if (priv->dev->data->dev_conf.intr_conf.rxq) {
-		priv_destroy_intr_vec(priv);
-		priv_intr_efd_disable(priv);
-	}
+	priv_rx_intr_vec_disable(priv);
 	priv_unlock(priv);
 	memset(priv, 0, sizeof(*priv));
 }
@@ -5635,7 +5627,7 @@ priv_dev_removal_interrupt_handler_install(struct priv *priv,
 }
 
 /**
- * Fill epoll fd list for rxq interrupts.
+ * Allocate queue vector and fill epoll fd list for Rx interrupts.
  *
  * @param priv
  *   Pointer to private structure.
@@ -5644,108 +5636,89 @@ priv_dev_removal_interrupt_handler_install(struct priv *priv,
  *   0 on success, negative on failure.
  */
 static int
-priv_intr_efd_enable(struct priv *priv)
+priv_rx_intr_vec_enable(struct priv *priv)
 {
 	unsigned int i;
 	unsigned int rxqs_n = priv->rxqs_n;
 	unsigned int n = RTE_MIN(rxqs_n, (uint32_t)RTE_MAX_RXTX_INTR_VEC_ID);
+	unsigned int count = 0;
 	struct rte_intr_handle *intr_handle = priv->dev->intr_handle;
 
-	if (n == 0)
+	if (!priv->dev->data->dev_conf.intr_conf.rxq)
 		return 0;
-	if (n < rxqs_n) {
-		WARN("rxqs num is larger than EAL max interrupt vector "
-		     "%u > %u unable to supprt rxq interrupts",
-		     rxqs_n, (uint32_t)RTE_MAX_RXTX_INTR_VEC_ID);
-		return -EINVAL;
+	priv_rx_intr_vec_disable(priv);
+	intr_handle->intr_vec = malloc(sizeof(intr_handle->intr_vec[rxqs_n]));
+	if (intr_handle->intr_vec == NULL) {
+		ERROR("failed to allocate memory for interrupt vector,"
+		      " Rx interrupts will not be supported");
+		return -ENOMEM;
 	}
 	intr_handle->type = RTE_INTR_HANDLE_EXT;
 	for (i = 0; i != n; ++i) {
 		struct rxq *rxq = (*priv->rxqs)[i];
-		int fd = rxq->channel->fd;
+		int fd;
 		int flags;
 		int rc;
 
+		/* Skip queues that cannot request interrupts. */
+		if (!rxq || !rxq->channel) {
+			/* Use invalid intr_vec[] index to disable entry. */
+			intr_handle->intr_vec[i] =
+				RTE_INTR_VEC_RXTX_OFFSET +
+				RTE_MAX_RXTX_INTR_VEC_ID;
+			continue;
+		}
+		if (count >= RTE_MAX_RXTX_INTR_VEC_ID) {
+			ERROR("too many Rx queues for interrupt vector size"
+			      " (%d), Rx interrupts cannot be enabled",
+			      RTE_MAX_RXTX_INTR_VEC_ID);
+			priv_rx_intr_vec_disable(priv);
+			return -1;
+		}
+		fd = rxq->channel->fd;
 		flags = fcntl(fd, F_GETFL);
 		rc = fcntl(fd, F_SETFL, flags | O_NONBLOCK);
 		if (rc < 0) {
-			WARN("failed to change rxq interrupt file "
-			     "descriptor %d for queue index %d", fd, i);
+			ERROR("failed to make Rx interrupt file descriptor"
+			      " %d non-blocking for queue index %d", fd, i);
+			priv_rx_intr_vec_disable(priv);
 			return rc;
 		}
-		intr_handle->efds[i] = fd;
+		intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + count;
+		intr_handle->efds[count] = fd;
+		count++;
 	}
-	intr_handle->nb_efd = n;
+	if (!count)
+		priv_rx_intr_vec_disable(priv);
+	else
+		intr_handle->nb_efd = count;
 	return 0;
 }
 
 /**
- * Clean epoll fd list for rxq interrupts.
+ * Clean up Rx interrupts handler.
  *
  * @param priv
  *   Pointer to private structure.
  */
 static void
-priv_intr_efd_disable(struct priv *priv)
+priv_rx_intr_vec_disable(struct priv *priv)
 {
 	struct rte_intr_handle *intr_handle = priv->dev->intr_handle;
 
 	rte_intr_free_epoll_fd(intr_handle);
+	free(intr_handle->intr_vec);
+	intr_handle->nb_efd = 0;
+	intr_handle->intr_vec = NULL;
 }
 
 /**
- * Create and init interrupt vector array.
- *
- * @param priv
- *   Pointer to private structure.
- *
- * @return
- *   0 on success, negative on failure.
- */
-static int
-priv_create_intr_vec(struct priv *priv)
-{
-	unsigned int rxqs_n = priv->rxqs_n;
-	unsigned int i;
-	struct rte_intr_handle *intr_handle = priv->dev->intr_handle;
-
-	if (rxqs_n == 0)
-		return 0;
-	intr_handle->intr_vec = (int *)
-		rte_malloc("intr_vec", rxqs_n * sizeof(int), 0);
-	if (intr_handle->intr_vec == NULL) {
-		WARN("Failed to allocate memory for intr_vec "
-		     "rxq interrupt will not be supported");
-		return -ENOMEM;
-	}
-	for (i = 0; i != rxqs_n; ++i) {
-		/* 1:1 mapping between rxq and interrupt. */
-		intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i;
-	}
-	return 0;
-}
-
-/**
- * Destroy init interrupt vector array.
- *
- * @param priv
- *   Pointer to private structure.
- */
-static void
-priv_destroy_intr_vec(struct priv *priv)
-{
-	struct rte_intr_handle *intr_handle = priv->dev->intr_handle;
-
-	rte_free(intr_handle->intr_vec);
-}
-
-/**
- * DPDK callback for rx queue interrupt enable.
+ * DPDK callback for Rx queue interrupt enable.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
  * @param idx
- *   RX queue index.
+ *   Rx queue index.
  *
  * @return
  *   0 on success, negative on failure.
@@ -5755,22 +5728,24 @@ mlx4_rx_intr_enable(struct rte_eth_dev *dev, uint16_t idx)
 {
 	struct priv *priv = dev->data->dev_private;
 	struct rxq *rxq = (*priv->rxqs)[idx];
-	struct ibv_cq *cq = rxq->cq;
-	int ret = 0;
+	int ret;
 
-	ret = ibv_req_notify_cq(cq, 0);
+	if (!rxq || !rxq->channel)
+		ret = EINVAL;
+	else
+		ret = ibv_req_notify_cq(rxq->cq, 0);
 	if (ret)
 		WARN("unable to arm interrupt on rx queue %d", idx);
 	return -ret;
 }
 
 /**
- * DPDK callback for rx queue interrupt disable.
+ * DPDK callback for Rx queue interrupt disable.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
  * @param idx
- *   RX queue index.
+ *   Rx queue index.
  *
  * @return
  *   0 on success, negative on failure.
@@ -5780,20 +5755,23 @@ mlx4_rx_intr_disable(struct rte_eth_dev *dev, uint16_t idx)
 {
 	struct priv *priv = dev->data->dev_private;
 	struct rxq *rxq = (*priv->rxqs)[idx];
-	struct ibv_cq *cq = rxq->cq;
 	struct ibv_cq *ev_cq;
 	void *ev_ctx;
-	int ret = 0;
+	int ret;
 
-	ret = ibv_get_cq_event(cq->channel, &ev_cq, &ev_ctx);
-	if (ret || ev_cq != cq)
-		ret = -1;
-	else
-		ibv_ack_cq_events(cq, 1);
+	if (!rxq || !rxq->channel) {
+		ret = EINVAL;
+	} else {
+		ret = ibv_get_cq_event(rxq->cq->channel, &ev_cq, &ev_ctx);
+		if (ret || ev_cq != rxq->cq)
+			ret = EINVAL;
+	}
 	if (ret)
 		WARN("unable to disable interrupt on rx queue %d",
 		     idx);
-	return ret;
+	else
+		ibv_ack_cq_events(rxq->cq, 1);
+	return -ret;
 }
 
 /**
-- 
2.1.4