[dpdk-dev] [PATCH 0/7] Fix Rx interrupt support in mlx4 and mlx5

[dpdk-dev] [PATCH 0/7] Fix Rx interrupt support in mlx4 and mlx5

[Adrien Mazarguil]

Several issues mainly related to error handling were found in both
implementations as they share most of their Rx interrupts handling code.

Another problem with the mlx4 implementation is that it does not work
properly with multiple ports adapters that share a common PCI device.

Adrien Mazarguil (7):
  net/mlx4: fix typos from prior commit
  net/mlx4: fix Rx interrupts with multiple ports
  net/mlx4: fix Rx interrupts management
  net/mlx5: fix misplaced Rx interrupts functions
  net/mlx5: fix Rx interrupts support checks
  net/mlx5: fix return value in Rx interrupts code
  net/mlx5: fix Rx interrupts management

 drivers/net/mlx4/mlx4.c         | 179 ++++++++++++++++-------------------
 drivers/net/mlx4/mlx4.h         |   1 +
 drivers/net/mlx5/mlx5.c         |   2 +
 drivers/net/mlx5/mlx5_rxq.c     | 142 ++++++++++++++++++---------
 drivers/net/mlx5/mlx5_rxtx.c    |  73 --------------
 drivers/net/mlx5/mlx5_rxtx.h    |  12 +--
 drivers/net/mlx5/mlx5_trigger.c |  16 ++--
 7 files changed, 194 insertions(+), 231 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH 1/7] net/mlx4: fix typos from prior commit

[Adrien Mazarguil]

Fixes: 9f05a4b81809 ("net/mlx4: support user space Rx interrupt event")

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Acked-by: Moti Haimovsky <motih@mellanox.com>
---
 drivers/net/mlx4/mlx4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 5bc2a50..178562e 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -5638,7 +5638,7 @@ priv_dev_removal_interrupt_handler_install(struct priv *priv,
  * Fill epoll fd list for rxq interrupts.
  *
  * @param priv
- *   Poinetr to private structure.
+ *   Pointer to private structure.
  *
  * @return
  *   0 on success, negative on failure.
@@ -5683,7 +5683,7 @@ priv_intr_efd_enable(struct priv *priv)
  * Clean epoll fd list for rxq interrupts.
  *
  * @param priv
- *   Ponter to private structure.
+ *   Pointer to private structure.
  */
 static void
 priv_intr_efd_disable(struct priv *priv)
-- 
2.1.4

[dpdk-dev] [PATCH 2/7] net/mlx4: fix Rx interrupts with multiple ports

[Adrien Mazarguil]

Several Ethernet device structures are allocated on top of a common PCI
device for mlx4 adapters with multiple ports. These inherit a common
interrupt handle from their parent PCI device, which prevents Rx interrupts
from working properly on all ports as their configuration is overwritten.

Use a local interrupt handle to address this issue.

Fixes: 9f05a4b81809 ("net/mlx4: support user space Rx interrupt event")

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Moti Haimovsky <motih@mellanox.com>
---
 drivers/net/mlx4/mlx4.c | 9 +++++++++
 drivers/net/mlx4/mlx4.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 178562e..2b4722f 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -6207,6 +6207,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 
 		eth_dev->device->driver = &mlx4_driver.driver;
 
+		/*
+		 * Copy and override interrupt handle to prevent it from
+		 * being shared between all ethdev instances of a given PCI
+		 * device. This is required to properly handle Rx interrupts
+		 * on all ports.
+		 */
+		priv->intr_handle_dev = *eth_dev->intr_handle;
+		eth_dev->intr_handle = &priv->intr_handle_dev;
+
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx4_dev_ops;
 
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index c46fc23..b74fbf8 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -345,6 +345,7 @@ struct priv {
 	unsigned int txqs_n; /* TX queues array size. */
 	struct rxq *(*rxqs)[]; /* RX queues. */
 	struct txq *(*txqs)[]; /* TX queues. */
+	struct rte_intr_handle intr_handle_dev; /* Device interrupt handler. */
 	struct rte_intr_handle intr_handle; /* Interrupt handler. */
 	struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */
 	LIST_HEAD(mlx4_flows, rte_flow) flows;
-- 
2.1.4

[dpdk-dev] [PATCH 3/7] net/mlx4: fix Rx interrupts management

[Adrien Mazarguil]

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 <adrien.mazarguil@6wind.com>
Acked-by: Moti Haimovsky <motih@mellanox.com>
---
 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 2b4722f..050d646 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 @@ rxq_setup(struct rte_eth_dev *dev, struct rxq *rxq, uint16_t desc,
 	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));
 }
@@ -5635,7 +5627,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.
@@ -5644,108 +5636,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.
@@ -5755,22 +5728,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.
@@ -5780,20 +5755,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.1.4

[dpdk-dev] [PATCH 4/7] net/mlx5: fix misplaced Rx interrupts functions

[Adrien Mazarguil]

These functions do not belong to the data path. Their prototypes are also
misplaced.

Fixes: 3c7d44af252a ("net/mlx5: support user space Rx interrupt event")

Cc: stable@dpdk.org
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx5/mlx5_rxq.c  | 73 +++++++++++++++++++++++++++++++++++++++
 drivers/net/mlx5/mlx5_rxtx.c | 73 ---------------------------------------
 drivers/net/mlx5/mlx5_rxtx.h | 12 +++----
 3 files changed, 79 insertions(+), 79 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 2a26839..5421d1f 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1418,3 +1418,76 @@ priv_destroy_intr_vec(struct priv *priv)
 
 	rte_free(intr_handle->intr_vec);
 }
+
+/**
+ * DPDK callback for rx queue interrupt enable.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param rx_queue_id
+ *   RX queue number.
+ *
+ * @return
+ *   0 on success, negative on failure.
+ */
+int
+mlx5_rx_intr_enable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
+{
+#ifdef HAVE_UPDATE_CQ_CI
+	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;
+
+	ibv_mlx5_exp_update_cq_ci(cq, ci);
+	ret = ibv_req_notify_cq(cq, 0);
+#else
+	int ret = -1;
+	(void)dev;
+	(void)rx_queue_id;
+#endif
+	if (ret)
+		WARN("unable to arm interrupt on rx queue %d", rx_queue_id);
+	return ret;
+}
+
+/**
+ * DPDK callback for rx queue interrupt disable.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param rx_queue_id
+ *   RX queue number.
+ *
+ * @return
+ *   0 on success, negative on failure.
+ */
+int
+mlx5_rx_intr_disable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
+{
+#ifdef HAVE_UPDATE_CQ_CI
+	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;
+
+	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);
+#else
+	int ret = -1;
+	(void)dev;
+	(void)rx_queue_id;
+#endif
+	if (ret)
+		WARN("unable to disable interrupt on rx queue %d",
+		     rx_queue_id);
+	return ret;
+}
diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 3f6fb70..cade625 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -2142,76 +2142,3 @@ removed_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
 	(void)pkts_n;
 	return 0;
 }
-
-/**
- * DPDK callback for rx queue interrupt enable.
- *
- * @param dev
- *   Pointer to Ethernet device structure.
- * @param rx_queue_id
- *   RX queue number
- *
- * @return
- *   0 on success, negative on failure.
- */
-int
-mlx5_rx_intr_enable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
-{
-#ifdef HAVE_UPDATE_CQ_CI
-	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;
-
-	ibv_mlx5_exp_update_cq_ci(cq, ci);
-	ret = ibv_req_notify_cq(cq, 0);
-#else
-	int ret = -1;
-	(void)dev;
-	(void)rx_queue_id;
-#endif
-	if (ret)
-		WARN("unable to arm interrupt on rx queue %d", rx_queue_id);
-	return ret;
-}
-
-/**
- * DPDK callback for rx queue interrupt disable.
- *
- * @param dev
- *   Pointer to Ethernet device structure.
- * @param rx_queue_id
- *   RX queue number
- *
- * @return
- *   0 on success, negative on failure.
- */
-int
-mlx5_rx_intr_disable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
-{
-#ifdef HAVE_UPDATE_CQ_CI
-	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;
-
-	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);
-#else
-	int ret = -1;
-	(void)dev;
-	(void)rx_queue_id;
-#endif
-	if (ret)
-		WARN("unable to disable interrupt on rx queue %d",
-		     rx_queue_id);
-	return ret;
-}
diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index 8db8eb1..cd79b5d 100644
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -298,10 +298,6 @@ int priv_create_hash_rxqs(struct priv *);
 void priv_destroy_hash_rxqs(struct priv *);
 int priv_allow_flow_type(struct priv *, enum hash_rxq_flow_type);
 int priv_rehash_flows(struct priv *);
-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);
 void rxq_cleanup(struct rxq_ctrl *);
 int rxq_rehash(struct rte_eth_dev *, struct rxq_ctrl *);
 int rxq_ctrl_setup(struct rte_eth_dev *, struct rxq_ctrl *, uint16_t,
@@ -311,6 +307,12 @@ 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 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);
 
 /* mlx5_txq.c */
 
@@ -333,8 +335,6 @@ uint16_t removed_tx_burst(void *, struct rte_mbuf **, uint16_t);
 uint16_t removed_rx_burst(void *, struct rte_mbuf **, uint16_t);
 int mlx5_rx_descriptor_status(void *, uint16_t);
 int mlx5_tx_descriptor_status(void *, uint16_t);
-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);
 
 /* mlx5_mr.c */
 
-- 
2.1.4

[dpdk-dev] [PATCH 5/7] net/mlx5: fix Rx interrupts support checks

[Adrien Mazarguil]

Not exposing Rx interrupts callbacks when this feature is unsupported is
less intrusive than having two different versions for these functions.

Fixes: 3c7d44af252a ("net/mlx5: support user space Rx interrupt event")

Cc: stable@dpdk.org
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx5/mlx5.c      |  2 ++
 drivers/net/mlx5/mlx5_rxq.c  | 16 ++++------------
 drivers/net/mlx5/mlx5_rxtx.h |  2 ++
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index bcb2c1b..49d4dba 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -246,8 +246,10 @@ static const struct eth_dev_ops mlx5_dev_ops = {
 	.filter_ctrl = mlx5_dev_filter_ctrl,
 	.rx_descriptor_status = mlx5_rx_descriptor_status,
 	.tx_descriptor_status = mlx5_tx_descriptor_status,
+#ifdef HAVE_UPDATE_CQ_CI
 	.rx_queue_intr_enable = mlx5_rx_intr_enable,
 	.rx_queue_intr_disable = mlx5_rx_intr_disable,
+#endif
 };
 
 static struct {
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 5421d1f..fa350bf 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1419,6 +1419,8 @@ priv_destroy_intr_vec(struct priv *priv)
 	rte_free(intr_handle->intr_vec);
 }
 
+#ifdef HAVE_UPDATE_CQ_CI
+
 /**
  * DPDK callback for rx queue interrupt enable.
  *
@@ -1433,7 +1435,6 @@ priv_destroy_intr_vec(struct priv *priv)
 int
 mlx5_rx_intr_enable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
-#ifdef HAVE_UPDATE_CQ_CI
 	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);
@@ -1443,11 +1444,6 @@ mlx5_rx_intr_enable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 
 	ibv_mlx5_exp_update_cq_ci(cq, ci);
 	ret = ibv_req_notify_cq(cq, 0);
-#else
-	int ret = -1;
-	(void)dev;
-	(void)rx_queue_id;
-#endif
 	if (ret)
 		WARN("unable to arm interrupt on rx queue %d", rx_queue_id);
 	return ret;
@@ -1467,7 +1463,6 @@ 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)
 {
-#ifdef HAVE_UPDATE_CQ_CI
 	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);
@@ -1481,13 +1476,10 @@ mlx5_rx_intr_disable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 		ret = -1;
 	else
 		ibv_ack_cq_events(cq, 1);
-#else
-	int ret = -1;
-	(void)dev;
-	(void)rx_queue_id;
-#endif
 	if (ret)
 		WARN("unable to disable interrupt on rx queue %d",
 		     rx_queue_id);
 	return ret;
 }
+
+#endif /* HAVE_UPDATE_CQ_CI */
diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index cd79b5d..1a3ede4 100644
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -311,8 +311,10 @@ 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);
+#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);
+#endif /* HAVE_UPDATE_CQ_CI */
 
 /* mlx5_txq.c */
 
-- 
2.1.4

[dpdk-dev] [PATCH 6/7] net/mlx5: fix return value in Rx interrupts code

[Adrien Mazarguil]

A negative return value is documented for that function in case of error.

Fixes: 3c7d44af252a ("net/mlx5: support user space Rx interrupt event")

Cc: stable@dpdk.org
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx5/mlx5_rxq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index fa350bf..46d566b 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1446,7 +1446,7 @@ mlx5_rx_intr_enable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	ret = ibv_req_notify_cq(cq, 0);
 	if (ret)
 		WARN("unable to arm interrupt on rx queue %d", rx_queue_id);
-	return ret;
+	return -ret;
 }
 
 /**
-- 
2.1.4

[dpdk-dev] [PATCH 7/7] net/mlx5: fix Rx interrupts management

[Adrien Mazarguil]

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>
---
 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 46d566b..5aa8121 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -978,10 +978,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;
 		}
 	}
@@ -1310,124 +1310,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.
@@ -1438,24 +1416,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.
@@ -1466,20 +1446,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
--- 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
--- 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.1.4

Re: [dpdk-dev] [PATCH 0/7] Fix Rx interrupt support in mlx4 and mlx5

[Nélio Laranjeiro]

On Wed, Jun 14, 2017 at 01:49:10PM +0200, Adrien Mazarguil wrote:
> Several issues mainly related to error handling were found in both
> implementations as they share most of their Rx interrupts handling code.
> 
> Another problem with the mlx4 implementation is that it does not work
> properly with multiple ports adapters that share a common PCI device.
> 
> Adrien Mazarguil (7):
>   net/mlx4: fix typos from prior commit
>   net/mlx4: fix Rx interrupts with multiple ports
>   net/mlx4: fix Rx interrupts management
>   net/mlx5: fix misplaced Rx interrupts functions
>   net/mlx5: fix Rx interrupts support checks
>   net/mlx5: fix return value in Rx interrupts code
>   net/mlx5: fix Rx interrupts management
> 
>  drivers/net/mlx4/mlx4.c         | 179 ++++++++++++++++-------------------
>  drivers/net/mlx4/mlx4.h         |   1 +
>  drivers/net/mlx5/mlx5.c         |   2 +
>  drivers/net/mlx5/mlx5_rxq.c     | 142 ++++++++++++++++++---------
>  drivers/net/mlx5/mlx5_rxtx.c    |  73 --------------
>  drivers/net/mlx5/mlx5_rxtx.h    |  12 +--
>  drivers/net/mlx5/mlx5_trigger.c |  16 ++--
>  7 files changed, 194 insertions(+), 231 deletions(-)
> 
> -- 
> 2.1.4

For the series:

Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH 2/7] net/mlx4: fix Rx interrupts with multiple ports

[Ferruh Yigit]

On 6/14/2017 12:49 PM, Adrien Mazarguil wrote:
> Several Ethernet device structures are allocated on top of a common PCI
> device for mlx4 adapters with multiple ports. These inherit a common
> interrupt handle from their parent PCI device, which prevents Rx interrupts
> from working properly on all ports as their configuration is overwritten.
> 
> Use a local interrupt handle to address this issue.

Hi Adrien,

I am not clear why local copy required, and main concern from my point
of view is if this is a common problem for all PMDs and should be
addressed in higher level?

The variable is already per eth_dev, but this patch moves it the private
data. What overwrites it within eth_dev?

Thanks,
ferruh

> 
> Fixes: 9f05a4b81809 ("net/mlx4: support user space Rx interrupt event")
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Acked-by: Moti Haimovsky <motih@mellanox.com>
> ---
>  drivers/net/mlx4/mlx4.c | 9 +++++++++
>  drivers/net/mlx4/mlx4.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index 178562e..2b4722f 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -6207,6 +6207,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
>  
>  		eth_dev->device->driver = &mlx4_driver.driver;
>  
> +		/*
> +		 * Copy and override interrupt handle to prevent it from
> +		 * being shared between all ethdev instances of a given PCI
> +		 * device. This is required to properly handle Rx interrupts
> +		 * on all ports.
> +		 */
> +		priv->intr_handle_dev = *eth_dev->intr_handle;
> +		eth_dev->intr_handle = &priv->intr_handle_dev;
> +
>  		priv->dev = eth_dev;
>  		eth_dev->dev_ops = &mlx4_dev_ops;
>  
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index c46fc23..b74fbf8 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -345,6 +345,7 @@ struct priv {
>  	unsigned int txqs_n; /* TX queues array size. */
>  	struct rxq *(*rxqs)[]; /* RX queues. */
>  	struct txq *(*txqs)[]; /* TX queues. */
> +	struct rte_intr_handle intr_handle_dev; /* Device interrupt handler. */
>  	struct rte_intr_handle intr_handle; /* Interrupt handler. */
>  	struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */
>  	LIST_HEAD(mlx4_flows, rte_flow) flows;
> 

Re: [dpdk-dev] [PATCH 2/7] net/mlx4: fix Rx interrupts with multiple ports

[Adrien Mazarguil]

On Fri, Jun 16, 2017 at 02:07:54PM +0100, Ferruh Yigit wrote:
> On 6/14/2017 12:49 PM, Adrien Mazarguil wrote:
> > Several Ethernet device structures are allocated on top of a common PCI
> > device for mlx4 adapters with multiple ports. These inherit a common
> > interrupt handle from their parent PCI device, which prevents Rx interrupts
> > from working properly on all ports as their configuration is overwritten.
> > 
> > Use a local interrupt handle to address this issue.
> 
> Hi Adrien,
> 
> I am not clear why local copy required, and main concern from my point
> of view is if this is a common problem for all PMDs and should be
> addressed in higher level?

This issue only affects PMDs that handle multiple Ethernet ports through a
single PCI device. Such PMDs (like mlx4) identify themselves as PCI drivers
that manually have to register multiple rte_eth_dev instances through
rte_eth_dev_allocate(), which they then have to initialize.

> The variable is already per eth_dev, but this patch moves it the private
> data. What overwrites it within eth_dev?

Calling rte_eth_copy_pci_info() makes the rte_eth_dev structure inherit the
default interrupt handle of the underlying PCI device. By "inherit", I mean
eth_dev->intr_handle points to it, in that sense it's not per eth_dev but
per PCI device.

mlx4 Rx interrupts are associated with a given Verbs context, and each port
has its own Verbs context, so they cannot be shared, while other PMDs using
other methods for catching interrupts may be perfectly fine with a single
vector associated with the PCI device. It depends on the PMD, for instance
there is no such problem with mlx5 as exactly one PCI device is associated
with a given port.

This patch merely allocates a specific interrupt handle associated with the
eth_dev itself and makes the eth_dev handle point to that instead of the
default PCI handle. This "local" handle is initialized using the PCI handle
as a template before modifying the pointer. It's completely safe.

> > Fixes: 9f05a4b81809 ("net/mlx4: support user space Rx interrupt event")
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Acked-by: Moti Haimovsky <motih@mellanox.com>
> > ---
> >  drivers/net/mlx4/mlx4.c | 9 +++++++++
> >  drivers/net/mlx4/mlx4.h | 1 +
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> > index 178562e..2b4722f 100644
> > --- a/drivers/net/mlx4/mlx4.c
> > +++ b/drivers/net/mlx4/mlx4.c
> > @@ -6207,6 +6207,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
> >  
> >  		eth_dev->device->driver = &mlx4_driver.driver;
> >  
> > +		/*
> > +		 * Copy and override interrupt handle to prevent it from
> > +		 * being shared between all ethdev instances of a given PCI
> > +		 * device. This is required to properly handle Rx interrupts
> > +		 * on all ports.
> > +		 */
> > +		priv->intr_handle_dev = *eth_dev->intr_handle;
> > +		eth_dev->intr_handle = &priv->intr_handle_dev;
> > +
> >  		priv->dev = eth_dev;
> >  		eth_dev->dev_ops = &mlx4_dev_ops;
> >  
> > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> > index c46fc23..b74fbf8 100644
> > --- a/drivers/net/mlx4/mlx4.h
> > +++ b/drivers/net/mlx4/mlx4.h
> > @@ -345,6 +345,7 @@ struct priv {
> >  	unsigned int txqs_n; /* TX queues array size. */
> >  	struct rxq *(*rxqs)[]; /* RX queues. */
> >  	struct txq *(*txqs)[]; /* TX queues. */
> > +	struct rte_intr_handle intr_handle_dev; /* Device interrupt handler. */
> >  	struct rte_intr_handle intr_handle; /* Interrupt handler. */
> >  	struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */
> >  	LIST_HEAD(mlx4_flows, rte_flow) flows;
> > 
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH 2/7] net/mlx4: fix Rx interrupts with multiple ports

[Ferruh Yigit]

On 6/16/2017 2:39 PM, Adrien Mazarguil wrote:
> On Fri, Jun 16, 2017 at 02:07:54PM +0100, Ferruh Yigit wrote:
>> On 6/14/2017 12:49 PM, Adrien Mazarguil wrote:
>>> Several Ethernet device structures are allocated on top of a common PCI
>>> device for mlx4 adapters with multiple ports. These inherit a common
>>> interrupt handle from their parent PCI device, which prevents Rx interrupts
>>> from working properly on all ports as their configuration is overwritten.
>>>
>>> Use a local interrupt handle to address this issue.
>>
>> Hi Adrien,
>>
>> I am not clear why local copy required, and main concern from my point
>> of view is if this is a common problem for all PMDs and should be
>> addressed in higher level?
> 
> This issue only affects PMDs that handle multiple Ethernet ports through a
> single PCI device. Such PMDs (like mlx4) identify themselves as PCI drivers
> that manually have to register multiple rte_eth_dev instances through
> rte_eth_dev_allocate(), which they then have to initialize.
> 
>> The variable is already per eth_dev, but this patch moves it the private
>> data. What overwrites it within eth_dev?
> 
> Calling rte_eth_copy_pci_info() makes the rte_eth_dev structure inherit the
> default interrupt handle of the underlying PCI device. By "inherit", I mean
> eth_dev->intr_handle points to it, in that sense it's not per eth_dev but
> per PCI device.
> 
> mlx4 Rx interrupts are associated with a given Verbs context, and each port
> has its own Verbs context, so they cannot be shared, while other PMDs using
> other methods for catching interrupts may be perfectly fine with a single
> vector associated with the PCI device. It depends on the PMD, for instance
> there is no such problem with mlx5 as exactly one PCI device is associated
> with a given port.
> 
> This patch merely allocates a specific interrupt handle associated with the
> eth_dev itself and makes the eth_dev handle point to that instead of the
> default PCI handle. This "local" handle is initialized using the PCI handle
> as a template before modifying the pointer. It's completely safe.

Thanks for clarification.

> 
>>> Fixes: 9f05a4b81809 ("net/mlx4: support user space Rx interrupt event")
>>>
>>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>> Acked-by: Moti Haimovsky <motih@mellanox.com>
>>> ---
>>>  drivers/net/mlx4/mlx4.c | 9 +++++++++
>>>  drivers/net/mlx4/mlx4.h | 1 +
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
>>> index 178562e..2b4722f 100644
>>> --- a/drivers/net/mlx4/mlx4.c
>>> +++ b/drivers/net/mlx4/mlx4.c
>>> @@ -6207,6 +6207,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
>>>  
>>>  		eth_dev->device->driver = &mlx4_driver.driver;
>>>  
>>> +		/*
>>> +		 * Copy and override interrupt handle to prevent it from
>>> +		 * being shared between all ethdev instances of a given PCI
>>> +		 * device. This is required to properly handle Rx interrupts
>>> +		 * on all ports.
>>> +		 */
>>> +		priv->intr_handle_dev = *eth_dev->intr_handle;
>>> +		eth_dev->intr_handle = &priv->intr_handle_dev;
>>> +
>>>  		priv->dev = eth_dev;
>>>  		eth_dev->dev_ops = &mlx4_dev_ops;
>>>  
>>> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
>>> index c46fc23..b74fbf8 100644
>>> --- a/drivers/net/mlx4/mlx4.h
>>> +++ b/drivers/net/mlx4/mlx4.h
>>> @@ -345,6 +345,7 @@ struct priv {
>>>  	unsigned int txqs_n; /* TX queues array size. */
>>>  	struct rxq *(*rxqs)[]; /* RX queues. */
>>>  	struct txq *(*txqs)[]; /* TX queues. */
>>> +	struct rte_intr_handle intr_handle_dev; /* Device interrupt handler. */
>>>  	struct rte_intr_handle intr_handle; /* Interrupt handler. */
>>>  	struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */
>>>  	LIST_HEAD(mlx4_flows, rte_flow) flows;
>>>
>>
> 

Re: [dpdk-dev] [PATCH 0/7] Fix Rx interrupt support in mlx4 and mlx5

[Ferruh Yigit]

On 6/14/2017 12:49 PM, Adrien Mazarguil wrote:
> Several issues mainly related to error handling were found in both
> implementations as they share most of their Rx interrupts handling code.
> 
> Another problem with the mlx4 implementation is that it does not work
> properly with multiple ports adapters that share a common PCI device.
> 
> Adrien Mazarguil (7):
>   net/mlx4: fix typos from prior commit
>   net/mlx4: fix Rx interrupts with multiple ports
>   net/mlx4: fix Rx interrupts management
>   net/mlx5: fix misplaced Rx interrupts functions
>   net/mlx5: fix Rx interrupts support checks
>   net/mlx5: fix return value in Rx interrupts code
>   net/mlx5: fix Rx interrupts management
> 
>  drivers/net/mlx4/mlx4.c         | 179 ++++++++++++++++-------------------
>  drivers/net/mlx4/mlx4.h         |   1 +
>  drivers/net/mlx5/mlx5.c         |   2 +
>  drivers/net/mlx5/mlx5_rxq.c     | 142 ++++++++++++++++++---------
>  drivers/net/mlx5/mlx5_rxtx.c    |  73 --------------
>  drivers/net/mlx5/mlx5_rxtx.h    |  12 +--
>  drivers/net/mlx5/mlx5_trigger.c |  16 ++--
>  7 files changed, 194 insertions(+), 231 deletions(-)

Series applied to dpdk-next-net/master, thanks.