Skip to contentSkip to conversation

irqfd fixes and enhancements

PATCH Superseded Gregory Haskins 3 participants 8 messages Jun 29, 2009 – Jul 1, 2009

Subsystems (3)

v7 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

v7 cover irqfd fixes and enhancements

(Applies to kvm.git/master:4631e094)

The following is the latest attempt to fix the races in irqfd/eventfd, as well as restore DEASSIGN support. For more details, please read the patch headers. I've restored the slow-work variant of the logic as a separate patch (5/5) so we can get a better idea of the true difference between the workqueue approach and the slow-work style. I am personally in favor of the slow-work approach since it doesnt require a mostly-idle+dedicated thread to hang around. But that is just me. 5/5 can be ignored, folded in to 3/5+4/5 as appropriate, or merged as is per the whim of Avi et. al.

As always, this series has been tested against the kvm-eventfd unit test with both 5/5 applied and unapplied, and everything appears to be functioning properly. You can download this test here:

ftp://ftp.novell.com/dev/ghaskins/kvm-eventfd.tar.bz2

I've included version 4 of Davide's eventfd patch (ported to kvm.git) so that its a complete reviewable series. Note, however, that there may be later versions of his patch to consider for merging, so we should coordinate with him.

-Greg

[Changelog:

v7:
*) Addressed minor-nit feedback from Michael
*) Cleaned up patch headers
*) Re-added separate slow-work feature patch to end for comparison

v6:
*) Removed slow-work in favor of using a dedicated single-thread
workqueue.
*) Condensed cleanup path to always use deferred shutdown *) Saved about 56 lines over v5, with the following diffstat:

 	   include/linux/kvm_host.h |    2 
 	   virt/kvm/eventfd.c       |  248 ++++++++++++++++++-----------------------------
 	   2 files changed, 97 insertions(+), 153 deletions(-)
	v5:
           Untracked..

]

---

Davide Libenzi (1):
eventfd - revised interface and cleanups (4th rev)

Gregory Haskins (4):
KVM: Make irqfd use slow-work for shutdown
KVM: add irqfd DEASSIGN feature
KVM: Fix races in irqfd using new eventfd_kref_get interface
kvm: prepare irqfd for having interrupts disabled during eventfd->release

 drivers/lguest/lg.h          |    2 
 drivers/lguest/lguest_user.c |    4 -
 fs/aio.c                     |   24 +---
 fs/eventfd.c                 |  126 ++++++++++++++++++----
 include/linux/aio.h          |    4 -
 include/linux/eventfd.h      |   35 +++++-
 include/linux/kvm.h          |    2 
 include/linux/kvm_host.h     |    7 +
 virt/kvm/Kconfig             |    1 
 virt/kvm/eventfd.c           |  246 ++++++++++++++++++++++++++++++++----------
 10 files changed, 343 insertions(+), 108 deletions(-)
Signature · 1 line
Signature
#1 lore raw

PATCH v7 1/5 kvm: prepare irqfd for having interrupts disabled during eventfd->release

We need to plug some race conditions on eventfd shutdown. In order to do this, we need to change the context in which the release notification is delivered so that the wqh lock is now held. However, there is currently code in the release callback that assumes it can sleep.

We have a slight chicken and egg problem where we cant fix the race without adding the lock, and we can't add the lock without breaking the sleepy code. Normally we could deal with this by making both changes in an atomic changeset. However, we want to keep the eventfd and kvm specific changes isolated to ease the reviewer burden on upstream eventfd (at the specific request of upstream). Therefore, we have this intermediate patch.

This intermediate patch allows the release() method to work in an atomic context, at the expense of correctness w.r.t. memory-leaks. Today we have a race condition. With this patch applied we leak. Both issues will be resolved later in the series. It is the author's opinion that a leak is better for bisectability than the hang would be should we leave the sleepy code in place after the locking changeover.

---

 virt/kvm/eventfd.c |   89 ++++++++++++++++------------------------------------
 1 files changed, 27 insertions(+), 62 deletions(-)
virt/kvm/eventfd.c +27 -62
index a9e7de7..9656027 100644
@@ -28,7 +28,6 @@
 #include <linux/file.h>
 #include <linux/list.h>
 #include <linux/eventfd.h>
-#include <linux/srcu.h>
 
 /*
  * --------------------------------------------------------------------
@@ -38,8 +37,6 @@
  * --------------------------------------------------------------------
  */
 struct _irqfd {
-	struct mutex              lock;
-	struct srcu_struct        srcu;
 	struct kvm               *kvm;
 	int                       gsi;
 	struct list_head          list;
@@ -53,48 +50,12 @@ static void
 irqfd_inject(struct work_struct *work)
 {
 	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
-	struct kvm *kvm;
-	int idx;
+	struct kvm *kvm = irqfd->kvm;
 
-	idx = srcu_read_lock(&irqfd->srcu);
-
-	kvm = rcu_dereference(irqfd->kvm);
-	if (kvm) {
-		mutex_lock(&kvm->irq_lock);
-		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
-		mutex_unlock(&kvm->irq_lock);
-	}
-
-	srcu_read_unlock(&irqfd->srcu, idx);
-}
-
-static void
-irqfd_disconnect(struct _irqfd *irqfd)
-{
-	struct kvm *kvm;
-
-	mutex_lock(&irqfd->lock);
-
-	kvm = rcu_dereference(irqfd->kvm);
-	rcu_assign_pointer(irqfd->kvm, NULL);
-
-	mutex_unlock(&irqfd->lock);
-
-	if (!kvm)
-		return;
-
-	mutex_lock(&kvm->lock);
-	list_del(&irqfd->list);
-	mutex_unlock(&kvm->lock);
-
-	/*
-	 * It is important to not drop the kvm reference until the next grace
-	 * period because there might be lockless references in flight up
-	 * until then
-	 */
-	synchronize_srcu(&irqfd->srcu);
-	kvm_put_kvm(kvm);
+	mutex_lock(&kvm->irq_lock);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	mutex_unlock(&kvm->irq_lock);
 }
 
 static int
@@ -103,26 +64,24 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
 	unsigned long flags = (unsigned long)key;
 
+	/*
+	 * Assume we will be called with interrupts disabled
+	 */
 	if (flags & POLLIN)
 		/*
-		 * The POLLIN wake_up is called with interrupts disabled.
-		 * Therefore we need to defer the IRQ injection until later
-		 * since we need to acquire the kvm->lock to do so.
+		 * Defer the IRQ injection until later since we need to
+		 * acquire the kvm->lock to do so.
 		 */
 		schedule_work(&irqfd->inject);
 
 	if (flags & POLLHUP) {
 		/*
-		 * The POLLHUP is called unlocked, so it theoretically should
-		 * be safe to remove ourselves from the wqh using the locked
-		 * variant of remove_wait_queue()
+		 * for now, just remove ourselves from the list and let
+		 * the rest dangle.  We will fix this up later once
+		 * the races in eventfd are fixed
 		 */
-		remove_wait_queue(irqfd->wqh, &irqfd->wait);
-		flush_work(&irqfd->inject);
-		irqfd_disconnect(irqfd);
-
-		cleanup_srcu_struct(&irqfd->srcu);
-		kfree(irqfd);
+		__remove_wait_queue(irqfd->wqh, &irqfd->wait);
+		irqfd->wqh = NULL;
 	}
 
 	return 0;
@@ -150,8 +109,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	if (!irqfd)
 		return -ENOMEM;
 
-	mutex_init(&irqfd->lock);
-	init_srcu_struct(&irqfd->srcu);
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
@@ -172,8 +129,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 
 	events = file->f_op->poll(file, &irqfd->pt);
 
-	kvm_get_kvm(kvm);
-
 	mutex_lock(&kvm->lock);
 	list_add_tail(&irqfd->list, &kvm->irqfds);
 	mutex_unlock(&kvm->lock);
@@ -211,6 +166,16 @@ kvm_irqfd_release(struct kvm *kvm)
 {
 	struct _irqfd *irqfd, *tmp;
 
-	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list)
-		irqfd_disconnect(irqfd);
+	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
+		if (irqfd->wqh)
+			remove_wait_queue(irqfd->wqh, &irqfd->wait);
+
+		flush_work(&irqfd->inject);
+
+		mutex_lock(&kvm->lock);
+		list_del(&irqfd->list);
+		mutex_unlock(&kvm->lock);
+
+		kfree(irqfd);
+	}
 }
#2 lore raw

PATCH v7 2/5 eventfd - revised interface and cleanups (4th rev)

In reply to Gregory Haskins

From: Davide Libenzi <[email protected]>

The following patch changes the eventfd interface to de-couple the eventfd memory context, from the file pointer instance.
Without such change, there is no clean way to racely free handle the POLLHUP event sent when the last instance of the file* goes away. Also, now the internal eventfd APIs are using the eventfd context instead of the file*.

Gregory, none of the APIs changed, so your patches do not need to be rebased over this one. The last three revisions just updated comments.

Andrew, I'm not posting the "AIO select EVENTFD" patch at this time. Will eventually try another push in your skull once I come back from vacation ;)

[gmh: resolved merge conflict against prior POLLHUP patch ]

---

 drivers/lguest/lg.h          |    2 -
 drivers/lguest/lguest_user.c |    4 +
 fs/aio.c                     |   24 ++------
 fs/eventfd.c                 |  126 ++++++++++++++++++++++++++++++++++++------
 include/linux/aio.h          |    4 +
 include/linux/eventfd.h      |   35 +++++++++---
 6 files changed, 147 insertions(+), 48 deletions(-)
drivers/lguest/lg.h +1 -1
index d4e8979..9c31382 100644
@@ -82,7 +82,7 @@ struct lg_cpu {
 
 struct lg_eventfd {
 	unsigned long addr;
-	struct file *event;
+	struct eventfd_ctx *event;
 };
 
 struct lg_eventfd_map {
drivers/lguest/lguest_user.c +2 -2
index 32e2971..9f9a295 100644
@@ -50,7 +50,7 @@ static int add_eventfd(struct lguest *lg, unsigned long addr, int fd)
 
 	/* Now append new entry. */
 	new->map[new->num].addr = addr;
-	new->map[new->num].event = eventfd_fget(fd);
+	new->map[new->num].event = eventfd_ctx_fdget(fd);
 	if (IS_ERR(new->map[new->num].event)) {
 		kfree(new);
 		return PTR_ERR(new->map[new->num].event);
@@ -357,7 +357,7 @@ static int close(struct inode *inode, struct file *file)
 
 	/* Release any eventfds they registered. */
 	for (i = 0; i < lg->eventfds->num; i++)
-		fput(lg->eventfds->map[i].event);
+		eventfd_ctx_put(lg->eventfds->map[i].event);
 	kfree(lg->eventfds);
 
 	/* If lg->dead doesn't contain an error code it will be NULL or a
fs/aio.c +7 -17
index 76da125..d065b2c 100644
@@ -485,6 +485,8 @@ static inline void really_put_req(struct kioctx *ctx, struct kiocb *req)
 {
 	assert_spin_locked(&ctx->ctx_lock);
 
+	if (req->ki_eventfd != NULL)
+		eventfd_ctx_put(req->ki_eventfd);
 	if (req->ki_dtor)
 		req->ki_dtor(req);
 	if (req->ki_iovec != &req->ki_inline_vec)
@@ -509,8 +511,6 @@ static void aio_fput_routine(struct work_struct *data)
 		/* Complete the fput(s) */
 		if (req->ki_filp != NULL)
 			__fput(req->ki_filp);
-		if (req->ki_eventfd != NULL)
-			__fput(req->ki_eventfd);
 
 		/* Link the iocb into the context's free list */
 		spin_lock_irq(&ctx->ctx_lock);
@@ -528,8 +528,6 @@ static void aio_fput_routine(struct work_struct *data)
  */
 static int __aio_put_req(struct kioctx *ctx, struct kiocb *req)
 {
-	int schedule_putreq = 0;
-
 	dprintk(KERN_DEBUG "aio_put(%p): f_count=%ld\n",
 		req, atomic_long_read(&req->ki_filp->f_count));
 
@@ -549,24 +547,16 @@ static int __aio_put_req(struct kioctx *ctx, struct kiocb *req)
 	 * we would not be holding the last reference to the file*, so
 	 * this function will be executed w/out any aio kthread wakeup.
 	 */
-	if (unlikely(atomic_long_dec_and_test(&req->ki_filp->f_count)))
-		schedule_putreq++;
-	else
-		req->ki_filp = NULL;
-	if (req->ki_eventfd != NULL) {
-		if (unlikely(atomic_long_dec_and_test(&req->ki_eventfd->f_count)))
-			schedule_putreq++;
-		else
-			req->ki_eventfd = NULL;
-	}
-	if (unlikely(schedule_putreq)) {
+	if (unlikely(atomic_long_dec_and_test(&req->ki_filp->f_count))) {
 		get_ioctx(ctx);
 		spin_lock(&fput_lock);
 		list_add(&req->ki_list, &fput_head);
 		spin_unlock(&fput_lock);
 		queue_work(aio_wq, &fput_work);
-	} else
+	} else {
+		req->ki_filp = NULL;
 		really_put_req(ctx, req);
+	}
 	return 1;
 }
 
@@ -1622,7 +1612,7 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 		 * an eventfd() fd, and will be signaled for each completed
 		 * event using the eventfd_signal() function.
 		 */
-		req->ki_eventfd = eventfd_fget((int) iocb->aio_resfd);
+		req->ki_eventfd = eventfd_ctx_fdget((int) iocb->aio_resfd);
 		if (IS_ERR(req->ki_eventfd)) {
 			ret = PTR_ERR(req->ki_eventfd);
 			req->ki_eventfd = NULL;
fs/eventfd.c +108 -18

178 lines not on this page · raw message

include/linux/aio.h +2 -2
index b16a957..47f7d93 100644
@@ -121,9 +121,9 @@ struct kiocb {
 
 	/*
 	 * If the aio_resfd field of the userspace iocb is not zero,
-	 * this is the underlying file* to deliver event to.
+	 * this is the underlying eventfd context to deliver events to.
 	 */
-	struct file		*ki_eventfd;
+	struct eventfd_ctx	*ki_eventfd;
 };
 
 #define is_sync_kiocb(iocb)	((iocb)->ki_key == KIOCB_SYNC_KEY)
include/linux/eventfd.h +27 -8
index f45a8ae..3b85ba6 100644
@@ -8,10 +8,8 @@
 #ifndef _LINUX_EVENTFD_H
 #define _LINUX_EVENTFD_H
 
-#ifdef CONFIG_EVENTFD
-
-/* For O_CLOEXEC and O_NONBLOCK */
 #include <linux/fcntl.h>
+#include <linux/file.h>
 
 /*
  * CAREFUL: Check include/asm-generic/fcntl.h when defining
@@ -27,16 +25,37 @@
 #define EFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
 #define EFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS | EFD_SEMAPHORE)
 
+#ifdef CONFIG_EVENTFD
+
+struct eventfd_ctx *eventfd_ctx_get(struct eventfd_ctx *ctx);
+void eventfd_ctx_put(struct eventfd_ctx *ctx);
 struct file *eventfd_fget(int fd);
-int eventfd_signal(struct file *file, int n);
+struct eventfd_ctx *eventfd_ctx_fdget(int fd);
+struct eventfd_ctx *eventfd_ctx_fileget(struct file *file);
+int eventfd_signal(struct eventfd_ctx *ctx, int n);
 
 #else /* CONFIG_EVENTFD */
 
-#define eventfd_fget(fd) ERR_PTR(-ENOSYS)
-static inline int eventfd_signal(struct file *file, int n)
-{ return 0; }
+/*
+ * Ugly ugly ugly error layer to support modules that uses eventfd but
+ * pretend to work in !CONFIG_EVENTFD configurations. Namely, AIO.
+ */
+static inline struct eventfd_ctx *eventfd_ctx_fdget(int fd)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline int eventfd_signal(struct eventfd_ctx *ctx, int n)
+{
+	return -ENOSYS;
+}
+
+static inline void eventfd_ctx_put(struct eventfd_ctx *ctx)
+{
+
+}
 
-#endif /* CONFIG_EVENTFD */
+#endif
 
 #endif /* _LINUX_EVENTFD_H */
 
#3 lore raw

PATCH v7 3/5 KVM: Fix races in irqfd using new eventfd_kref_get interface

In reply to Gregory Haskins

eventfd currently emits a POLLHUP wakeup on f_ops->release() to generate a "release" callback. This lets eventfd clients know if the eventfd is about to go away and is very useful particularly for in-kernel clients. However, until recently it is not possible to use this feature of eventfd in a race-free way. This patch utilizes a new eventfd interface to rectify the problem.

---

 include/linux/kvm_host.h |    5 +-
 virt/kvm/eventfd.c       |  153 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 129 insertions(+), 29 deletions(-)
include/linux/kvm_host.h +4 -1
index 2451f48..5a90c6a 100644
@@ -141,7 +141,10 @@ struct kvm {
 	struct kvm_io_bus mmio_bus;
 	struct kvm_io_bus pio_bus;
 #ifdef CONFIG_HAVE_KVM_EVENTFD
-	struct list_head irqfds;
+	struct {
+		spinlock_t        lock;
+		struct list_head  items;
+	} irqfds;
 #endif
 	struct kvm_vm_stat stat;
 	struct kvm_arch arch;
virt/kvm/eventfd.c +125 -28

231 lines not on this page · raw message

#4 lore raw

PATCH v7 4/5 KVM: add irqfd DEASSIGN feature

In reply to Gregory Haskins

DEASSIGN allows us to optionally disassociate an IRQFD from its underlying eventfd without destroying the eventfd in the process. This is useful for conditions like live-migration which may have an eventfd associated with a device and an IRQFD. We need to be able to decouple the guest from the event source while not perturbing the event source itself.

CC Michael S. Tsirkin <[email protected]>

---

 include/linux/kvm.h |    2 ++
 virt/kvm/eventfd.c  |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 2 deletions(-)
include/linux/kvm.h +2 -0
index 38ff31e..6710518 100644
@@ -490,6 +490,8 @@ struct kvm_x86_mce {
 };
 #endif
 
+#define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+
 struct kvm_irqfd {
 	__u32 fd;
 	__u32 gsi;
virt/kvm/eventfd.c +44 -2

58 lines not on this page · raw message

#5 lore raw

PATCH v7 5/5 KVM: Make irqfd use slow-work for shutdown

In reply to Gregory Haskins

This patch eliminates the mostly idle, dedicated work-queue in favor of utilizing the slow-work thread pool. The downside to this approach is that we add ~30 lines of complexity to irqfd to get rid of the thread, but this may be a worthwhile tradeoff.

Note that a race is known to exist: the slow-work thread may race with module removal. We are currently working with slow-work upstream to fix this issue as well.

---

 include/linux/kvm_host.h |    2 +
 virt/kvm/Kconfig         |    1 
 virt/kvm/eventfd.c       |  100 +++++++++++++++++++++++++++++-----------------
 3 files changed, 65 insertions(+), 38 deletions(-)
include/linux/kvm_host.h +2 -0
index 5a90c6a..d94ee72 100644
@@ -144,6 +144,8 @@ struct kvm {
 	struct {
 		spinlock_t        lock;
 		struct list_head  items;
+		atomic_t          outstanding;
+		wait_queue_head_t wqh;
 	} irqfds;
 #endif
 	struct kvm_vm_stat stat;
virt/kvm/Kconfig +1 -0
index daece36..ab7848a 100644
@@ -9,6 +9,7 @@ config HAVE_KVM_IRQCHIP
 config HAVE_KVM_EVENTFD
        bool
        select EVENTFD
+       select SLOW_WORK
 
 config KVM_APIC_ARCHITECTURE
        bool
virt/kvm/eventfd.c +62 -38

167 lines not on this page · raw message

#6 lore raw
In reply to Gregory Haskins
Quoted 11 lines · Gregory Haskins

(Applies to kvm.git/master:4631e094)

The following is the latest attempt to fix the races in irqfd/eventfd, as well as restore DEASSIGN support. For more details, please read the patch headers. I've restored the slow-work variant of the logic as a separate patch (5/5) so we can get a better idea of the true difference between the workqueue approach and the slow-work style. I am personally in favor of the slow-work approach since it doesnt require a mostly-idle+dedicated thread to hang around. But that is just me. 5/5 can be ignored, folded in to 3/5+4/5 as appropriate, or merged as is per the whim of Avi et. al.

I think a good compromise would be to create the workqueue when the first VM is launched.

Quoted 11 lines

As always, this series has been tested against the kvm-eventfd unit test with both 5/5 applied and unapplied, and everything appears to be functioning properly. You can download this test here:

ftp://ftp.novell.com/dev/ghaskins/kvm-eventfd.tar.bz2

I've included version 4 of Davide's eventfd patch (ported to kvm.git) so that its a complete reviewable series. Note, however, that there may be later versions of his patch to consider for merging, so we should coordinate with him.

Davide's patch was merged upstream. Please take a look to make sure this patchset is compatible with what was merged.

Patchset looks good to me, but I'd appreciate an ack from Michael (or anyone else who's interested).

Signature · 1 line
error compiling committee.c: too many arguments to function
#7 lore raw
Quoted 28 lines · Avi Kivity
Quoted 11 lines · Gregory Haskins

(Applies to kvm.git/master:4631e094)

The following is the latest attempt to fix the races in irqfd/eventfd, as well as restore DEASSIGN support. For more details, please read the patch headers. I've restored the slow-work variant of the logic as a separate patch (5/5) so we can get a better idea of the true difference between the workqueue approach and the slow-work style. I am personally in favor of the slow-work approach since it doesnt require a mostly-idle+dedicated thread to hang around. But that is just me. 5/5 can be ignored, folded in to 3/5+4/5 as appropriate, or merged as is per the whim of Avi et. al.

I think a good compromise would be to create the workqueue when the first VM is launched.

Quoted 11 lines

As always, this series has been tested against the kvm-eventfd unit test with both 5/5 applied and unapplied, and everything appears to be functioning properly. You can download this test here:

ftp://ftp.novell.com/dev/ghaskins/kvm-eventfd.tar.bz2

I've included version 4 of Davide's eventfd patch (ported to kvm.git) so that its a complete reviewable series. Note, however, that there may be later versions of his patch to consider for merging, so we should coordinate with him.

Quoted 5 lines

Davide's patch was merged upstream. Please take a look to make sure this patchset is compatible with what was merged.

Patchset looks good to me, but I'd appreciate an ack from Michael (or anyone else who's interested).

For patches 1,3,4:

As for 5: IMHO, let's keep it simple meanwhile.

Signature · 1 line
MST