[dpdk-dev] [PATCH] net/mlx5: fix DevX event registration timing

[dpdk-dev] [PATCH] net/mlx5: fix DevX event registration timing

[Matan Azrad]

The DevX counter management triggers an asynchronous event to get back
the new counters values from the HW.

The counter management doesn't trigger 2 parallel events for the same
pool, hence, the pool cannot be updated again in the event waiting time.

When the port is stopped, the DevX event mechanism wrongly was
destroyed what remained all the waiting pools in waiting state forever.

As a result, the counters of the stuck pools were never updated again.

Separate the DevX interrupt installation from the dev installation and
remove the DevX interrupt unregistration\registration from the
stop\start operations.

Now, the DevX interrupt should be installed in probe and uninstalled in
close.

Cc: stable@dpdk.org
Fixes: f15db67df09c ("net/mlx5: accelerate DV flow counter query")

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5.c        |  15 +++-
 drivers/net/mlx5/mlx5.h        |   4 ++
 drivers/net/mlx5/mlx5_ethdev.c | 153 ++++++++++++++++++++++++++++++++---------
 3 files changed, 137 insertions(+), 35 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 7e0d7ec..fac5105 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -379,8 +379,10 @@ struct mlx5_dev_spawn_data {
 	 * there is no interrupt subhandler installed for
 	 * the given port index i.
 	 */
-	for (i = 0; i < sh->max_port; i++)
+	for (i = 0; i < sh->max_port; i++) {
 		sh->port[i].ih_port_id = RTE_MAX_ETHPORTS;
+		sh->port[i].devx_ih_port_id = RTE_MAX_ETHPORTS;
+	}
 	sh->pd = mlx5_glue->alloc_pd(sh->ctx);
 	if (sh->pd == NULL) {
 		DRV_LOG(ERR, "PD allocation failure");
@@ -481,6 +483,15 @@ struct mlx5_dev_spawn_data {
 	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);
 	if (sh->pd)
 		claim_zero(mlx5_glue->dealloc_pd(sh->pd));
@@ -845,6 +856,7 @@ struct mlx5_dev_spawn_data {
 		((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);
 	mlx5_traffic_disable(dev);
 	mlx5_flow_flush(dev, NULL);
 	/* Prevent crashes when queues are still in use. */
@@ -2739,6 +2751,7 @@ struct mlx5_dev_spawn_data {
 		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) {
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index bac1c0a..b6a51b2 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -501,6 +501,7 @@ struct mlx5_flow_counter_mng {
 /* Per port data of shared IB device. */
 struct mlx5_ibv_shared_port {
 	uint32_t ih_port_id;
+	uint32_t devx_ih_port_id;
 	/*
 	 * Interrupt handler port_id. Used by shared interrupt
 	 * handler to find the corresponding rte_eth device
@@ -588,6 +589,7 @@ struct mlx5_ibv_shared {
 	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_ibv_shared_port port[]; /* per device port data array. */
@@ -721,6 +723,8 @@ int mlx5_dev_to_pci_addr(const char *dev_path,
 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);
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 5f05b2b..2278b24 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1448,6 +1448,37 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 				     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);
+	assert(priv->ibv_port);
+	assert(priv->ibv_port <= sh->max_port);
+	assert(dev->data->port_id < RTE_MAX_ETHPORTS);
+	if (sh->port[priv->ibv_port - 1].devx_ih_port_id >= RTE_MAX_ETHPORTS)
+		goto exit;
+	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,
@@ -1490,8 +1521,9 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		assert(sh->intr_cnt);
 		goto exit;
 	}
-	sh->port[priv->ibv_port - 1].ih_port_id = (uint32_t)dev->data->port_id;
 	if (sh->intr_cnt) {
+		sh->port[priv->ibv_port - 1].ih_port_id =
+						(uint32_t)dev->data->port_id;
 		sh->intr_cnt++;
 		goto exit;
 	}
@@ -1500,52 +1532,81 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 	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");
-		goto error;
+		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);
+	assert(priv->ibv_port);
+	assert(priv->ibv_port <= sh->max_port);
+	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. */
+		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;
 	}
-	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);
 	if (priv->config.devx) {
 #ifndef HAVE_IBV_DEVX_ASYNC
-		goto error_unregister;
+		goto exit;
 #else
 		sh->devx_comp = mlx5_glue->devx_create_cmd_comp(sh->ctx);
 		if (sh->devx_comp) {
-			flags = fcntl(sh->devx_comp->fd, F_GETFL);
-			ret = fcntl(sh->devx_comp->fd, F_SETFL,
+			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");
-				goto error_unregister;
+					" 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;
 			}
-			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);
-		} else {
-			DRV_LOG(INFO, "failed to create devx async command "
-				"completion");
-			goto error_unregister;
 		}
 #endif /* HAVE_IBV_DEVX_ASYNC */
 	}
-	sh->intr_cnt++;
-	goto exit;
-error_unregister:
-	rte_intr_callback_unregister(&sh->intr_handle,
-				     mlx5_dev_interrupt_handler, sh);
-error:
-	/* Indicate there will be no interrupts. */
-	dev->data->dev_conf.intr_conf.lsc = 0;
-	dev->data->dev_conf.intr_conf.rmv = 0;
-	sh->intr_handle.fd = 0;
-	sh->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
-	sh->port[priv->ibv_port - 1].ih_port_id = RTE_MAX_ETHPORTS;
 exit:
 	pthread_mutex_unlock(&sh->intr_mutex);
 }
@@ -1575,6 +1636,30 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 }
 
 /**
+ * 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
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix DevX event registration timing

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Matan Azrad
> Sent: Tuesday, October 22, 2019 10:34 AM
> To: dev@dpdk.org
> Cc: Slava Ovsiienko <viacheslavo@mellanox.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/mlx5: fix DevX event registration timing
> 
> The DevX counter management triggers an asynchronous event to get back
> the new counters values from the HW.
> 
> The counter management doesn't trigger 2 parallel events for the same
> pool, hence, the pool cannot be updated again in the event waiting time.
> 
> When the port is stopped, the DevX event mechanism wrongly was
> destroyed what remained all the waiting pools in waiting state forever.
> 
> As a result, the counters of the stuck pools were never updated again.
> 
> Separate the DevX interrupt installation from the dev installation and
> remove the DevX interrupt unregistration\registration from the
> stop\start operations.
> 
> Now, the DevX interrupt should be installed in probe and uninstalled in
> close.
> 
> Cc: stable@dpdk.org
> Fixes: f15db67df09c ("net/mlx5: accelerate DV flow counter query")
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Slava Ovsiienko <viacheslavo@mellanox.com>

Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh