DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [RFC PATCH] vfio: avoid re-installing irq handler
@ 2019-07-15 15:58 Hyong Youb Kim
  0 siblings, 0 replies; 6+ messages in thread
From: Hyong Youb Kim @ 2019-07-15 15:58 UTC (permalink / raw)
  To: David Marchand, Thomas Monjalon, Jerin Jacob Kollanukkaran, Ferruh Yigit
  Cc: dev, John Daley, Hyong Youb Kim

A rough patch for the approach mentioned earlier. It is only for
discussion.
http://mails.dpdk.org/archives/dev/2019-July/138113.html

To try this out, first revert the following then apply.
commit 89aac60e0be9 ("vfio: fix interrupts race condition")

Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
---
 .../common/include/rte_eal_interrupts.h       |  1 +
 lib/librte_eal/linux/eal/eal_interrupts.c     | 68 ++++++++++++++++++-
 2 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
index b370c0d26..d509967d3 100644
--- a/lib/librte_eal/common/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
@@ -76,6 +76,7 @@ struct rte_intr_handle {
 	enum rte_intr_handle_type type;  /**< handle type */
 	uint32_t max_intr;             /**< max interrupt requested */
 	uint32_t nb_efd;               /**< number of available efd(event fd) */
+	uint8_t vfio_efd_assigned;
 	uint8_t efd_counter_size;      /**< size of efd counter, used for vdev */
 	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
 	struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
diff --git a/lib/librte_eal/linux/eal/eal_interrupts.c b/lib/librte_eal/linux/eal/eal_interrupts.c
index 79ad5e8d7..5b03388dd 100644
--- a/lib/librte_eal/linux/eal/eal_interrupts.c
+++ b/lib/librte_eal/linux/eal/eal_interrupts.c
@@ -107,9 +107,21 @@ static pthread_t intr_thread;
 #define MSIX_IRQ_SET_BUF_LEN (sizeof(struct vfio_irq_set) + \
 			      sizeof(int) * (RTE_MAX_RXTX_INTR_VEC_ID + 1))
 
-/* enable legacy (INTx) interrupts */
+#ifndef __INTEL_COMPILER
+#pragma GCC diagnostic ignored "-Wcast-qual"
+#endif
+
+static void
+set_vfio_efd_assigned(const struct rte_intr_handle *intr_handle, uint8_t v) {
+	((struct rte_intr_handle *)intr_handle)->vfio_efd_assigned = v;
+}
+
+#ifndef __INTEL_COMPILER
+#pragma GCC diagnostic error "-Wcast-qual"
+#endif
+
 static int
-vfio_enable_intx(const struct rte_intr_handle *intr_handle) {
+vfio_assign_efd_intx(const struct rte_intr_handle *intr_handle) {
 	struct vfio_irq_set *irq_set;
 	char irq_set_buf[IRQ_SET_BUF_LEN];
 	int len, ret;
@@ -134,8 +146,36 @@ vfio_enable_intx(const struct rte_intr_handle *intr_handle) {
 						intr_handle->fd);
 		return -1;
 	}
+	/*
+	 * In the kernel, irq has been set up, its handler installed (request_irq),
+	 * and efd is assigned to it.  Remember this so we can avoid
+	 * re-doing irq setup later.
+	 */
+	set_vfio_efd_assigned(intr_handle, 1);
+	return 0;
+}
+
+/* enable legacy (INTx) interrupts */
+static int
+vfio_enable_intx(const struct rte_intr_handle *intr_handle) {
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	/*
+	 * Assign efd to irq only if it's not been done, to avoid
+	 * re-installing irq handler in the kernel.
+	 */
+	if (!intr_handle->vfio_efd_assigned) {
+		ret = vfio_assign_efd_intx(intr_handle);
+		if (ret)
+			return ret;
+	}
+
+	len = sizeof(irq_set_buf);
 
 	/* unmask INTx after enabling */
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
 	memset(irq_set, 0, len);
 	len = sizeof(struct vfio_irq_set);
 	irq_set->argsz = len;
@@ -194,6 +234,12 @@ vfio_disable_intx(const struct rte_intr_handle *intr_handle) {
 			"Error disabling INTx interrupts for fd %d\n", intr_handle->fd);
 		return -1;
 	}
+	/*
+	 * Irq handler's been removed in the kernel. The next time we
+	 * enable intr_handle, we need to assign efd to irq. Remember
+	 * that.
+	 */
+	set_vfio_efd_assigned(intr_handle, 0);
 	return 0;
 }
 
@@ -205,6 +251,13 @@ vfio_enable_msi(const struct rte_intr_handle *intr_handle) {
 	struct vfio_irq_set *irq_set;
 	int *fd_ptr;
 
+	/*
+	 * vfio does not support MSI masking/unmaking. So if irq has
+	 * been set up previously, nothing to do.
+	 */
+	if (intr_handle->vfio_efd_assigned)
+		return 0;
+
 	len = sizeof(irq_set_buf);
 
 	irq_set = (struct vfio_irq_set *) irq_set_buf;
@@ -223,6 +276,7 @@ vfio_enable_msi(const struct rte_intr_handle *intr_handle) {
 						intr_handle->fd);
 		return -1;
 	}
+	set_vfio_efd_assigned(intr_handle, 1);
 	return 0;
 }
 
@@ -248,6 +302,7 @@ vfio_disable_msi(const struct rte_intr_handle *intr_handle) {
 		RTE_LOG(ERR, EAL,
 			"Error disabling MSI interrupts for fd %d\n", intr_handle->fd);
 
+	set_vfio_efd_assigned(intr_handle, 0);
 	return ret;
 }
 
@@ -259,6 +314,13 @@ vfio_enable_msix(const struct rte_intr_handle *intr_handle) {
 	struct vfio_irq_set *irq_set;
 	int *fd_ptr;
 
+	/*
+	 * vfio does not support MSI-X masking/unmaking. So if irq has
+	 * been set up previously, nothing to do.
+	 */
+	if (intr_handle->vfio_efd_assigned)
+		return 0;
+
 	len = sizeof(irq_set_buf);
 
 	irq_set = (struct vfio_irq_set *) irq_set_buf;
@@ -284,6 +346,7 @@ vfio_enable_msix(const struct rte_intr_handle *intr_handle) {
 		return -1;
 	}
 
+	set_vfio_efd_assigned(intr_handle, 1);
 	return 0;
 }
 
@@ -309,6 +372,7 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
 		RTE_LOG(ERR, EAL,
 			"Error disabling MSI-X interrupts for fd %d\n", intr_handle->fd);
 
+	set_vfio_efd_assigned(intr_handle, 0);
 	return ret;
 }
 
-- 
2.22.0


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [RFC PATCH] vfio: avoid re-installing irq handler
@ 2019-07-15 16:50 Jerin Jacob Kollanukkaran
  2019-07-16  5:58 ` Hyong Youb Kim (hyonkim)
  0 siblings, 1 reply; 6+ messages in thread
From: Jerin Jacob Kollanukkaran @ 2019-07-15 16:50 UTC (permalink / raw)
  To: Hyong Youb Kim, David Marchand, Thomas Monjalon, Ferruh Yigit
  Cc: dev, John Daley, Shahed Shaikh, Nithin Kumar Dabilpuram

> -----Original Message-----
> From: Hyong Youb Kim <hyonkim@cisco.com>
> Sent: Monday, July 15, 2019 9:28 PM
> To: David Marchand <david.marchand@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; John Daley <johndale@cisco.com>; Hyong Youb Kim
> <hyonkim@cisco.com>
> Subject: [EXT] [RFC PATCH] vfio: avoid re-installing irq handler
> 
> A rough patch for the approach mentioned earlier. It is only for discussion.
> http://mails.dpdk.org/archives/dev/2019-July/138113.html
> 
> To try this out, first revert the following then apply.
> commit 89aac60e0be9 ("vfio: fix interrupts race condition")

Yes. This patch has to be to reverted. It changes the existing interrupt behavior and does not address the MSIX case as well.

I think, The clean fix would be to introduce rte_intr_mask() and rte_intr_unmask() by abstracting the INTX and MSIX differences
And let qede driver call it as needed.

Thoughts?

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-16  9:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-15 15:58 [dpdk-dev] [RFC PATCH] vfio: avoid re-installing irq handler Hyong Youb Kim
2019-07-15 16:50 Jerin Jacob Kollanukkaran
2019-07-16  5:58 ` Hyong Youb Kim (hyonkim)
2019-07-16  6:47   ` Jerin Jacob Kollanukkaran
2019-07-16  7:49     ` Hyong Youb Kim (hyonkim)
2019-07-16  9:56       ` Jerin Jacob Kollanukkaran

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).