net/mlx5: fix Rx interrupts management

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_ctrl_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_ctrl_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_ctrl_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: 3c7d44af252a ("net/mlx5: support user space Rx interrupt event")
Cc: stable@dpdk.org
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 43a7756..22be999 100644 (file)
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -980,10 +980,10 @@ rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
        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",
-                       (void *)dev, strerror(ret));
+                       ERROR("%p: Rx interrupt completion channel creation"
+                             " failure: %s",
+                             (void *)dev, strerror(ret));
                        goto error;
                }
        }
@@ -1312,124 +1312,102 @@ mlx5_rx_burst_secondary_setup(void *dpdk_rxq, struct rte_mbuf **pkts,
 }
 
 /**
- * Fill epoll fd list for rxq interrupts.
+ * Allocate queue vector and fill epoll fd list for Rx interrupts.
  *
  * @param priv
- *   Private structure.
+ *   Pointer to private structure.
  *
  * @return
  *   0 on success, negative on failure.
  */
 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];
                struct rxq_ctrl *rxq_ctrl =
                        container_of(rxq, struct rxq_ctrl, rxq);
-               int fd = rxq_ctrl->channel->fd;
+               int fd;
                int flags;
                int rc;
 
+               /* Skip queues that cannot request interrupts. */
+               if (!rxq || !rxq_ctrl->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_ctrl->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 -1;
                }
-               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
- *   Private structure.
+ *   Pointer to private structure.
  */
 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);
-}
-
-/**
- * Create and init interrupt vector array.
- *
- * @param priv
- *   Private structure.
- *
- * @return
- *   0 on success, negative on failure.
- */
-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
- *   Private structure.
- *
- * @return
- *   0 on success, negative on failure.
- */
-void
-priv_destroy_intr_vec(struct priv *priv)
-{
-       struct rte_intr_handle *intr_handle = priv->dev->intr_handle;
-
-       rte_free(intr_handle->intr_vec);
+       free(intr_handle->intr_vec);
+       intr_handle->nb_efd = 0;
+       intr_handle->intr_vec = NULL;
 }
 
 #ifdef HAVE_UPDATE_CQ_CI
 
 /**
- * DPDK callback for rx queue interrupt enable.
+ * DPDK callback for Rx queue interrupt enable.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
  * @param rx_queue_id
- *   RX queue number.
+ *   Rx queue number.
  *
  * @return
  *   0 on success, negative on failure.
@@ -1440,24 +1418,26 @@ mlx5_rx_intr_enable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
        struct priv *priv = mlx5_get_priv(dev);
        struct rxq *rxq = (*priv->rxqs)[rx_queue_id];
        struct rxq_ctrl *rxq_ctrl = container_of(rxq, struct rxq_ctrl, rxq);
-       struct ibv_cq *cq = rxq_ctrl->cq;
-       uint16_t ci = rxq->cq_ci;
-       int ret = 0;
+       int ret;
 
-       ibv_mlx5_exp_update_cq_ci(cq, ci);
-       ret = ibv_req_notify_cq(cq, 0);
+       if (!rxq || !rxq_ctrl->channel) {
+               ret = EINVAL;
+       } else {
+               ibv_mlx5_exp_update_cq_ci(rxq_ctrl->cq, rxq->cq_ci);
+               ret = ibv_req_notify_cq(rxq_ctrl->cq, 0);
+       }
        if (ret)
                WARN("unable to arm interrupt on rx queue %d", rx_queue_id);
        return -ret;
 }
 
 /**
- * DPDK callback for rx queue interrupt disable.
+ * DPDK callback for Rx queue interrupt disable.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
  * @param rx_queue_id
- *   RX queue number.
+ *   Rx queue number.
  *
  * @return
  *   0 on success, negative on failure.
@@ -1468,20 +1448,23 @@ mlx5_rx_intr_disable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
        struct priv *priv = mlx5_get_priv(dev);
        struct rxq *rxq = (*priv->rxqs)[rx_queue_id];
        struct rxq_ctrl *rxq_ctrl = container_of(rxq, struct rxq_ctrl, rxq);
-       struct ibv_cq *cq = rxq_ctrl->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_ctrl->channel) {
+               ret = EINVAL;
+       } else {
+               ret = ibv_get_cq_event(rxq_ctrl->cq->channel, &ev_cq, &ev_ctx);
+               if (ret || ev_cq != rxq_ctrl->cq)
+                       ret = EINVAL;
+       }
        if (ret)
                WARN("unable to disable interrupt on rx queue %d",
                     rx_queue_id);
-       return ret;
+       else
+               ibv_ack_cq_events(rxq_ctrl->cq, 1);
+       return -ret;
 }
 
 #endif /* HAVE_UPDATE_CQ_CI */

diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index 1a3ede4..450a569 100644 (file)
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -307,10 +307,8 @@ int mlx5_rx_queue_setup(struct rte_eth_dev *, uint16_t, uint16_t, unsigned int,
                        const struct rte_eth_rxconf *, struct rte_mempool *);
 void mlx5_rx_queue_release(void *);
 uint16_t mlx5_rx_burst_secondary_setup(void *, struct rte_mbuf **, uint16_t);
-int priv_intr_efd_enable(struct priv *priv);
-void priv_intr_efd_disable(struct priv *priv);
-int priv_create_intr_vec(struct priv *priv);
-void priv_destroy_intr_vec(struct priv *priv);
+int priv_rx_intr_vec_enable(struct priv *priv);
+void priv_rx_intr_vec_disable(struct priv *priv);
 #ifdef HAVE_UPDATE_CQ_CI
 int mlx5_rx_intr_enable(struct rte_eth_dev *dev, uint16_t rx_queue_id);
 int mlx5_rx_intr_disable(struct rte_eth_dev *dev, uint16_t rx_queue_id);

diff --git a/drivers/net/mlx5/mlx5_trigger.c b/drivers/net/mlx5/mlx5_trigger.c
index 8c5aa69..40f23da 100644 (file)
--- a/drivers/net/mlx5/mlx5_trigger.c
+++ b/drivers/net/mlx5/mlx5_trigger.c
@@ -94,12 +94,13 @@ mlx5_dev_start(struct rte_eth_dev *dev)
                      (void *)priv, strerror(err));
                goto error;
        }
-       priv_dev_interrupt_handler_install(priv, dev);
-       if (dev->data->dev_conf.intr_conf.rxq) {
-               err = priv_intr_efd_enable(priv);
-               if (!err)
-                       err = priv_create_intr_vec(priv);
+       err = priv_rx_intr_vec_enable(priv);
+       if (err) {
+               ERROR("%p: RX interrupt vector creation failed",
+                     (void *)priv);
+               goto error;
        }
+       priv_dev_interrupt_handler_install(priv, dev);
        priv_xstats_init(priv);
        priv_unlock(priv);
        return 0;
@@ -140,11 +141,8 @@ mlx5_dev_stop(struct rte_eth_dev *dev)
        priv_destroy_hash_rxqs(priv);
        priv_fdir_disable(priv);
        priv_flow_stop(priv);
+       priv_rx_intr_vec_disable(priv);
        priv_dev_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->started = 0;
        priv_unlock(priv);
 }