Hi,
> -----Original Message-----
> From: Suanming Mou <suanmingm@mellanox.com>
> Sent: Thursday, May 28, 2020 12:22 PM
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Matan Azrad
> <matan@mellanox.com>; dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@mellanox.com>; stable@dpdk.org
> Subject: [PATCH v2] net/mlx5: fix interrupt installation timing
>
> Currently, the DevX counter query works asynchronously with Devx
> interrupt handler return the query result. When port closes, the
> interrupt handler will be uninstalled and the Devx comp obj will
> also be destroyed. Meanwhile the query is still not cancelled.
>
> In this case, counter query may use the invalid Devx comp which
> has been destroyed, and query failure with invalid FD will be
> reported.
>
> Adjust the shared interrupt install and uninstall timing to make
> the counter asynchronous query stop before interrupt uninstall.
>
> Fixes: f15db67 ("net/mlx5: accelerate DV flow counter query")
> Cc: stable@dpdk.org
>
> Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
> ---
> v2: fix by adjusting the interrupt install timing.
> ---
> drivers/net/mlx5/mlx5.c | 101 +++++++++++++----
> drivers/net/mlx5/mlx5.h | 5 -
> drivers/net/mlx5/mlx5_ethdev.c | 243 ----------------------------------------
> drivers/net/mlx5/mlx5_trigger.c | 17 ++-
> 4 files changed, 97 insertions(+), 269 deletions(-)
Patch applied to next-net-mlx,
Kindest regards,
Raslan Darawsheh
@@ -10,6 +10,7 @@
#include <stdlib.h>
#include <errno.h>
#include <net/if.h>
+#include <fcntl.h>
#include <sys/mman.h>
#include <linux/rtnetlink.h>
@@ -656,6 +657,85 @@ struct mlx5_flow_id_pool *
}
/**
+ * Install shared asynchronous device events handler.
+ * This function is implemented to support event sharing
+ * between multiple ports of single IB device.
+ *
+ * @param sh
+ * Pointer to mlx5_ibv_shared object.
+ */
+static void
+mlx5_dev_shared_handler_install(struct mlx5_ibv_shared *sh)
+{
+ int ret;
+ int flags;
+
+ sh->intr_handle.fd = -1;
+ flags = fcntl(sh->ctx->async_fd, F_GETFL);
+ ret = fcntl(sh->ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
+ if (ret) {
+ DRV_LOG(INFO, "failed to change file descriptor async event"
+ " queue");
+ } else {
+ sh->intr_handle.fd = sh->ctx->async_fd;
+ sh->intr_handle.type = RTE_INTR_HANDLE_EXT;
+ if (rte_intr_callback_register(&sh->intr_handle,
+ mlx5_dev_interrupt_handler, sh)) {
+ DRV_LOG(INFO, "Fail to install the shared interrupt.");
+ sh->intr_handle.fd = -1;
+ }
+ }
+ if (sh->devx) {
+#ifdef HAVE_IBV_DEVX_ASYNC
+ sh->intr_handle_devx.fd = -1;
+ sh->devx_comp = mlx5_glue->devx_create_cmd_comp(sh->ctx);
+ if (!sh->devx_comp) {
+ DRV_LOG(INFO, "failed to allocate devx_comp.");
+ return;
+ }
+ flags = fcntl(sh->devx_comp->fd, F_GETFL);
+ ret = fcntl(sh->devx_comp->fd, F_SETFL, flags | O_NONBLOCK);
+ if (ret) {
+ DRV_LOG(INFO, "failed to change file descriptor"
+ " devx comp");
+ return;
+ }
+ sh->intr_handle_devx.fd = sh->devx_comp->fd;
+ sh->intr_handle_devx.type = RTE_INTR_HANDLE_EXT;
+ if (rte_intr_callback_register(&sh->intr_handle_devx,
+ mlx5_dev_interrupt_handler_devx, sh)) {
+ DRV_LOG(INFO, "Fail to install the devx shared"
+ " interrupt.");
+ sh->intr_handle_devx.fd = -1;
+ }
+#endif /* HAVE_IBV_DEVX_ASYNC */
+ }
+}
+
+/**
+ * Uninstall shared asynchronous device events handler.
+ * This function is implemented to support event sharing
+ * between multiple ports of single IB device.
+ *
+ * @param dev
+ * Pointer to mlx5_ibv_shared object.
+ */
+static void
+mlx5_dev_shared_handler_uninstall(struct mlx5_ibv_shared *sh)
+{
+ if (sh->intr_handle.fd >= 0)
+ mlx5_intr_callback_unregister(&sh->intr_handle,
+ mlx5_dev_interrupt_handler, sh);
+#ifdef HAVE_IBV_DEVX_ASYNC
+ if (sh->intr_handle_devx.fd >= 0)
+ rte_intr_callback_unregister(&sh->intr_handle_devx,
+ mlx5_dev_interrupt_handler_devx, sh);
+ if (sh->devx_comp)
+ mlx5_glue->devx_destroy_cmd_comp(sh->devx_comp);
+#endif
+}
+
+/**
* Allocate shared IB device context. If there is multiport device the
* master and representors will share this context, if there is single
* port dedicated IB device, the context will be used by only given
@@ -749,7 +829,6 @@ struct mlx5_flow_id_pool *
sizeof(sh->ibdev_name));
strncpy(sh->ibdev_path, sh->ctx->device->ibdev_path,
sizeof(sh->ibdev_path));
- pthread_mutex_init(&sh->intr_mutex, NULL);
/*
* Setting port_id to max unallowed value means
* there is no interrupt subhandler installed for
@@ -810,6 +889,7 @@ struct mlx5_flow_id_pool *
err = rte_errno;
goto error;
}
+ mlx5_dev_shared_handler_install(sh);
mlx5_flow_aging_init(sh);
mlx5_flow_counters_mng_init(sh);
mlx5_flow_ipool_create(sh, config);
@@ -886,20 +966,7 @@ struct mlx5_flow_id_pool *
**/
mlx5_flow_counters_mng_close(sh);
mlx5_flow_ipool_destroy(sh);
- MLX5_ASSERT(!sh->intr_cnt);
- if (sh->intr_cnt)
- mlx5_intr_callback_unregister
- (&sh->intr_handle, mlx5_dev_interrupt_handler, sh);
-#ifdef HAVE_MLX5_DEVX_ASYNC_SUPPORT
- if (sh->devx_intr_cnt) {
- if (sh->intr_handle_devx.fd)
- rte_intr_callback_unregister(&sh->intr_handle_devx,
- mlx5_dev_interrupt_handler_devx, sh);
- if (sh->devx_comp)
- mlx5dv_devx_destroy_cmd_comp(sh->devx_comp);
- }
-#endif
- pthread_mutex_destroy(&sh->intr_mutex);
+ mlx5_dev_shared_handler_uninstall(sh);
if (sh->pd)
claim_zero(mlx5_glue->dealloc_pd(sh->pd));
if (sh->tis)
@@ -1426,9 +1493,6 @@ struct mlx5_flow_id_pool *
DRV_LOG(DEBUG, "port %u closing device \"%s\"",
dev->data->port_id,
((priv->sh->ctx != NULL) ? priv->sh->ctx->device->name : ""));
- /* In case mlx5_dev_stop() has not been called. */
- mlx5_dev_interrupt_handler_uninstall(dev);
- mlx5_dev_interrupt_handler_devx_uninstall(dev);
/*
* If default mreg copy action is removed at the stop stage,
* the search will return none and nothing will be done anymore.
@@ -3613,7 +3677,6 @@ struct mlx5_flow_id_pool *
rte_eth_copy_pci_info(list[i].eth_dev, pci_dev);
/* Restore non-PCI flags cleared by the above call. */
list[i].eth_dev->data->dev_flags |= restore;
- mlx5_dev_interrupt_handler_devx_install(list[i].eth_dev);
rte_eth_dev_probing_finish(list[i].eth_dev);
}
if (i != ns) {
@@ -512,10 +512,7 @@ struct mlx5_ibv_shared {
struct mlx5_indexed_pool *ipool[MLX5_IPOOL_MAX];
/* Memory Pool for mlx5 flow resources. */
/* Shared interrupt handler section. */
- pthread_mutex_t intr_mutex; /* Interrupt config mutex. */
- uint32_t intr_cnt; /* Interrupt handler reference counter. */
struct rte_intr_handle intr_handle; /* Interrupt handler for device. */
- uint32_t devx_intr_cnt; /* Devx interrupt handler reference counter. */
struct rte_intr_handle intr_handle_devx; /* DEVX interrupt handler. */
struct mlx5dv_devx_cmd_comp *devx_comp; /* DEVX async comp obj. */
struct mlx5_devx_obj *tis; /* TIS object. */
@@ -671,8 +668,6 @@ int mlx5_dev_set_flow_ctrl(struct rte_eth_dev *dev,
void mlx5_dev_interrupt_handler_devx(void *arg);
void mlx5_dev_interrupt_handler_uninstall(struct rte_eth_dev *dev);
void mlx5_dev_interrupt_handler_install(struct rte_eth_dev *dev);
-void mlx5_dev_interrupt_handler_devx_uninstall(struct rte_eth_dev *dev);
-void mlx5_dev_interrupt_handler_devx_install(struct rte_eth_dev *dev);
int mlx5_set_link_down(struct rte_eth_dev *dev);
int mlx5_set_link_up(struct rte_eth_dev *dev);
int mlx5_is_removed(struct rte_eth_dev *dev);
@@ -1445,249 +1445,6 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
}
/**
- * Uninstall shared asynchronous device events handler.
- * This function is implemented to support event sharing
- * between multiple ports of single IB device.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-static void
-mlx5_dev_shared_handler_uninstall(struct rte_eth_dev *dev)
-{
- struct mlx5_priv *priv = dev->data->dev_private;
- struct mlx5_ibv_shared *sh = priv->sh;
-
- if (rte_eal_process_type() != RTE_PROC_PRIMARY)
- return;
- pthread_mutex_lock(&sh->intr_mutex);
- MLX5_ASSERT(priv->ibv_port);
- MLX5_ASSERT(priv->ibv_port <= sh->max_port);
- MLX5_ASSERT(dev->data->port_id < RTE_MAX_ETHPORTS);
- if (sh->port[priv->ibv_port - 1].ih_port_id >= RTE_MAX_ETHPORTS)
- goto exit;
- MLX5_ASSERT(sh->port[priv->ibv_port - 1].ih_port_id ==
- (uint32_t)dev->data->port_id);
- MLX5_ASSERT(sh->intr_cnt);
- sh->port[priv->ibv_port - 1].ih_port_id = RTE_MAX_ETHPORTS;
- if (!sh->intr_cnt || --sh->intr_cnt)
- goto exit;
- mlx5_intr_callback_unregister(&sh->intr_handle,
- mlx5_dev_interrupt_handler, sh);
- sh->intr_handle.fd = 0;
- sh->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
-exit:
- pthread_mutex_unlock(&sh->intr_mutex);
-}
-
-/**
- * Uninstall devx shared asynchronous device events handler.
- * This function is implemeted to support event sharing
- * between multiple ports of single IB device.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-static void
-mlx5_dev_shared_handler_devx_uninstall(struct rte_eth_dev *dev)
-{
- struct mlx5_priv *priv = dev->data->dev_private;
- struct mlx5_ibv_shared *sh = priv->sh;
-
- if (rte_eal_process_type() != RTE_PROC_PRIMARY)
- return;
- pthread_mutex_lock(&sh->intr_mutex);
- MLX5_ASSERT(priv->ibv_port);
- MLX5_ASSERT(priv->ibv_port <= sh->max_port);
- MLX5_ASSERT(dev->data->port_id < RTE_MAX_ETHPORTS);
- if (sh->port[priv->ibv_port - 1].devx_ih_port_id >= RTE_MAX_ETHPORTS)
- goto exit;
- MLX5_ASSERT(sh->port[priv->ibv_port - 1].devx_ih_port_id ==
- (uint32_t)dev->data->port_id);
- sh->port[priv->ibv_port - 1].devx_ih_port_id = RTE_MAX_ETHPORTS;
- if (!sh->devx_intr_cnt || --sh->devx_intr_cnt)
- goto exit;
- if (sh->intr_handle_devx.fd) {
- rte_intr_callback_unregister(&sh->intr_handle_devx,
- mlx5_dev_interrupt_handler_devx,
- sh);
- sh->intr_handle_devx.fd = 0;
- sh->intr_handle_devx.type = RTE_INTR_HANDLE_UNKNOWN;
- }
- if (sh->devx_comp) {
- mlx5_glue->devx_destroy_cmd_comp(sh->devx_comp);
- sh->devx_comp = NULL;
- }
-exit:
- pthread_mutex_unlock(&sh->intr_mutex);
-}
-
-/**
- * Install shared asynchronous device events handler.
- * This function is implemented to support event sharing
- * between multiple ports of single IB device.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-static void
-mlx5_dev_shared_handler_install(struct rte_eth_dev *dev)
-{
- struct mlx5_priv *priv = dev->data->dev_private;
- struct mlx5_ibv_shared *sh = priv->sh;
- int ret;
- int flags;
-
- if (rte_eal_process_type() != RTE_PROC_PRIMARY)
- return;
- pthread_mutex_lock(&sh->intr_mutex);
- MLX5_ASSERT(priv->ibv_port);
- MLX5_ASSERT(priv->ibv_port <= sh->max_port);
- MLX5_ASSERT(dev->data->port_id < RTE_MAX_ETHPORTS);
- if (sh->port[priv->ibv_port - 1].ih_port_id < RTE_MAX_ETHPORTS) {
- /* The handler is already installed for this port. */
- MLX5_ASSERT(sh->intr_cnt);
- goto exit;
- }
- if (sh->intr_cnt) {
- sh->port[priv->ibv_port - 1].ih_port_id =
- (uint32_t)dev->data->port_id;
- sh->intr_cnt++;
- goto exit;
- }
- /* No shared handler installed. */
- MLX5_ASSERT(sh->ctx->async_fd > 0);
- flags = fcntl(sh->ctx->async_fd, F_GETFL);
- ret = fcntl(sh->ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
- if (ret) {
- DRV_LOG(INFO, "failed to change file descriptor async event"
- " queue");
- /* Indicate there will be no interrupts. */
- dev->data->dev_conf.intr_conf.lsc = 0;
- dev->data->dev_conf.intr_conf.rmv = 0;
- } else {
- sh->intr_handle.fd = sh->ctx->async_fd;
- sh->intr_handle.type = RTE_INTR_HANDLE_EXT;
- rte_intr_callback_register(&sh->intr_handle,
- mlx5_dev_interrupt_handler, sh);
- sh->intr_cnt++;
- sh->port[priv->ibv_port - 1].ih_port_id =
- (uint32_t)dev->data->port_id;
- }
-exit:
- pthread_mutex_unlock(&sh->intr_mutex);
-}
-
-/**
- * Install devx shared asyncronous device events handler.
- * This function is implemeted to support event sharing
- * between multiple ports of single IB device.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-static void
-mlx5_dev_shared_handler_devx_install(struct rte_eth_dev *dev)
-{
- struct mlx5_priv *priv = dev->data->dev_private;
- struct mlx5_ibv_shared *sh = priv->sh;
-
- if (rte_eal_process_type() != RTE_PROC_PRIMARY)
- return;
- pthread_mutex_lock(&sh->intr_mutex);
- MLX5_ASSERT(priv->ibv_port);
- MLX5_ASSERT(priv->ibv_port <= sh->max_port);
- MLX5_ASSERT(dev->data->port_id < RTE_MAX_ETHPORTS);
- if (sh->port[priv->ibv_port - 1].devx_ih_port_id < RTE_MAX_ETHPORTS) {
- /* The handler is already installed for this port. */
- MLX5_ASSERT(sh->devx_intr_cnt);
- goto exit;
- }
- if (sh->devx_intr_cnt) {
- sh->devx_intr_cnt++;
- sh->port[priv->ibv_port - 1].devx_ih_port_id =
- (uint32_t)dev->data->port_id;
- goto exit;
- }
- if (priv->config.devx) {
-#ifndef HAVE_IBV_DEVX_ASYNC
- goto exit;
-#else
- sh->devx_comp = mlx5_glue->devx_create_cmd_comp(sh->ctx);
- if (sh->devx_comp) {
- int flags = fcntl(sh->devx_comp->fd, F_GETFL);
- int ret = fcntl(sh->devx_comp->fd, F_SETFL,
- flags | O_NONBLOCK);
-
- if (ret) {
- DRV_LOG(INFO, "failed to change file descriptor"
- " devx async event queue");
- } else {
- sh->intr_handle_devx.fd = sh->devx_comp->fd;
- sh->intr_handle_devx.type = RTE_INTR_HANDLE_EXT;
- rte_intr_callback_register
- (&sh->intr_handle_devx,
- mlx5_dev_interrupt_handler_devx, sh);
- sh->devx_intr_cnt++;
- sh->port[priv->ibv_port - 1].devx_ih_port_id =
- (uint32_t)dev->data->port_id;
- }
- }
-#endif /* HAVE_IBV_DEVX_ASYNC */
- }
-exit:
- pthread_mutex_unlock(&sh->intr_mutex);
-}
-
-/**
- * Uninstall interrupt handler.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-void
-mlx5_dev_interrupt_handler_uninstall(struct rte_eth_dev *dev)
-{
- mlx5_dev_shared_handler_uninstall(dev);
-}
-
-/**
- * Install interrupt handler.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-void
-mlx5_dev_interrupt_handler_install(struct rte_eth_dev *dev)
-{
- mlx5_dev_shared_handler_install(dev);
-}
-
-/**
- * Devx uninstall interrupt handler.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-void
-mlx5_dev_interrupt_handler_devx_uninstall(struct rte_eth_dev *dev)
-{
- mlx5_dev_shared_handler_devx_uninstall(dev);
-}
-
-/**
- * Devx install interrupt handler.
- *
- * @param dev
- * Pointer to Ethernet device.
- */
-void
-mlx5_dev_interrupt_handler_devx_install(struct rte_eth_dev *dev)
-{
- mlx5_dev_shared_handler_devx_install(dev);
-}
-
-/**
* DPDK callback to bring the link DOWN.
*
* @param dev
@@ -264,6 +264,7 @@
int
mlx5_dev_start(struct rte_eth_dev *dev)
{
+ struct mlx5_priv *priv = dev->data->dev_private;
int ret;
int fine_inline;
@@ -335,7 +336,18 @@
dev->rx_pkt_burst = mlx5_select_rx_function(dev);
/* Enable datapath on secondary process. */
mlx5_mp_req_start_rxtx(dev);
- mlx5_dev_interrupt_handler_install(dev);
+ if (priv->sh->intr_handle.fd >= 0) {
+ priv->sh->port[priv->ibv_port - 1].ih_port_id =
+ (uint32_t)dev->data->port_id;
+ } else {
+ DRV_LOG(INFO, "port %u starts without LSC and RMV interrupts.",
+ dev->data->port_id);
+ dev->data->dev_conf.intr_conf.lsc = 0;
+ dev->data->dev_conf.intr_conf.rmv = 0;
+ }
+ if (priv->sh->intr_handle_devx.fd >= 0)
+ priv->sh->port[priv->ibv_port - 1].devx_ih_port_id =
+ (uint32_t)dev->data->port_id;
return 0;
error:
ret = rte_errno; /* Save rte_errno before cleanup. */
@@ -377,7 +389,8 @@
/* All RX queue flags will be cleared in the flush interface. */
mlx5_flow_list_flush(dev, &priv->flows, true);
mlx5_rx_intr_vec_disable(dev);
- mlx5_dev_interrupt_handler_uninstall(dev);
+ priv->sh->port[priv->ibv_port - 1].ih_port_id = RTE_MAX_ETHPORTS;
+ priv->sh->port[priv->ibv_port - 1].devx_ih_port_id = RTE_MAX_ETHPORTS;
mlx5_txq_stop(dev);
mlx5_rxq_stop(dev);
}