From 0a2ae703192cd43de9b084dc073cf1aea8749af4 Mon Sep 17 00:00:00 2001 From: Adrien Mazarguil Date: Wed, 14 Jun 2017 13:49:13 +0200 Subject: [PATCH] net/mlx4: 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_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 Acked-by: Moti Haimovsky --- 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 b20ceccee9..ae4be9eec8 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 @@ skip_mr: 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)); } @@ -5637,7 +5629,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. @@ -5646,108 +5638,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. @@ -5757,22 +5730,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. @@ -5782,20 +5757,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.20.1