From e1016cb733839cbccb65ad2e46246bb4b8e4708a Mon Sep 17 00:00:00 2001 From: Adrien Mazarguil Date: Wed, 14 Jun 2017 13:49:17 +0200 Subject: [PATCH] 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 --- drivers/net/mlx5/mlx5_rxq.c | 155 ++++++++++++++------------------ drivers/net/mlx5/mlx5_rxtx.h | 6 +- drivers/net/mlx5/mlx5_trigger.c | 16 ++-- 3 files changed, 78 insertions(+), 99 deletions(-) diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c index 43a775698f..22be999e8b 100644 --- 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 1a3ede44b3..450a569667 100644 --- 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 8c5aa69109..40f23da93a 100644 --- 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); } -- 2.20.1