irqfd/eventfd fixes

This patch fixes all known races in irqfd, and paves the way to restore DEASSIGN support. For details of the eventfd races, please see the patch presumably commited immediately prior to this one.

In a nutshell, we use eventfd_kref_get/put() to properly manage the lifetime of the underlying eventfd. We also use careful coordination with our workqueue to ensure that all irqfd objects have terminated before we allow kvm to shutdown. The logic used for shutdown walks all open irqfds and releases them. This logic can be generalized in the future to allow a subset of irqfds to be released, thus allowing DEASSIGN support.

Signed-off-by Gregory Haskins <[email protected]>

---

 virt/kvm/eventfd.c |  144 ++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 110 insertions(+), 34 deletions(-)

virt/kvm/eventfd.c +1 -0

index 9656027..67985cd 100644
@@ -28,6 +28,7 @@
 #include <linux/file.h>
 #include <linux/list.h>
 #include <linux/eventfd.h>
+#include <linux/kref.h>
 
 /*
  * --------------------------------------------------------------------

@@ -36,26 +37,68 @@
  * Credit goes to Avi Kivity for the original idea.

virt/kvm/eventfd.c +8 -1

 };
 
 static void
-irqfd_inject(struct work_struct *work)
+irqfd_release(struct _irqfd *irqfd)
+{
+	eventfd_kref_put(irqfd->eventfd);
+	kfree(irqfd);
+}
+
+static void
+irqfd_work(struct work_struct *work)

virt/kvm/eventfd.c +7 -0

+		/* shutdown the irqfd */
+		struct _irqfd *_irqfd = NULL;
+
+		mutex_lock(&kvm->lock);
+
+		if (!list_empty(&irqfd->list))
+			_irqfd = irqfd;

virt/kvm/eventfd.c +8 -0

 Since the KVM side should be
+		 * holding the reference now, and will block behind a
+		 * flush_work(), lets just let them do the release() for us
+		 */
+		if (!_irqfd)
+			return;
+
+		irqfd_release(_irqfd);
+	}
 }
 

static int

virt/kvm/eventfd.c +1 -8

@@ -65,25 +108,20 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	unsigned long flags = (unsigned long)key;
 
 	/*
-	 * Assume we will be called with interrupts disabled
+	 * called with interrupts disabled
 	 */
-	if (flags & POLLIN)
-		/*
-		 * Defer the IRQ injection until later since we need to
-		 * acquire the kvm->lock to do so.
-		 */
-		schedule_work(&irqfd->inject);
-
 	if (flags & POLLHUP) {

virt/kvm/eventfd.c +12 -6

 	}
 
+	if (flags & (POLLHUP | POLLIN))
+		schedule_work(&irqfd->work);
+
 	return 0;
 }
 

@@ -102,6 +140,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
+	struct kref *kref = NULL;
 	int ret;
 	unsigned int events;
 

@@ -112,7 +151,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
-	INIT_WORK(&irqfd->inject, irqfd_inject);
+	INIT_WORK(&irqfd->work, irqfd_work);
 
 	file = eventfd_fget(fd);
 	if (IS_ERR(file)) {

@@ -133,11 +172,13 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	list_add_tail(&irqfd->list, &kvm->irqfds);
 	mutex_unlock(&kvm->lock);
 
-	/*
-	 * Check if there was an event already queued
-	 */
-	if (events & POLLIN)
-		schedule_work(&irqfd->inject);
+	kref = eventfd_kref_get(file);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	irqfd->eventfd = kref;
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise

virt/kvm/eventfd.c +27 -8

@@ -161,21 +211,47 @@ kvm_irqfd_init(struct kvm *kvm)
 	INIT_LIST_HEAD(&kvm->irqfds);
 }
 
+static struct _irqfd *
+irqfd_pop(struct kvm *kvm)
+{
+	struct _irqfd *irqfd = NULL;
+
+	mutex_lock(&kvm->lock);
+
+	if (!list_empty(&kvm->irqfds)) {
+		irqfd = list_first_entry(&kvm->irqfds, struct _irqfd, list);
+		list_del(&irqfd->list);
+	}
+
+	mutex_unlock(&kvm->lock);
+
+	return irqfd;
+}
+
 void
 kvm_irqfd_release(struct kvm *kvm)
 {
-	struct _irqfd *irqfd, *tmp;
+	struct _irqfd *irqfd;
 
-	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
-		if (irqfd->wqh)
-			remove_wait_queue(irqfd->wqh, &irqfd->wait);
+	while ((irqfd = irqfd_pop(kvm))) {
 
-		flush_work(&irqfd->inject);
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);
 
-		mutex_lock(&kvm->lock);
-		list_del(&irqfd->list);
-		mutex_unlock(&kvm->lock);
+		/*
+		 * We guarantee there will be no more notifications after
+		 * the remove_wait_queue returns.  Now lets make sure we
+		 * synchronize behind any outstanding work items before
+		 * releasing the resources
+		 */
+		flush_work(&irqfd->work);

virt/kvm/eventfd.c +1 -0

+	flush_scheduled_work();
 }

Quoted 12 lines · Gregory Haskins

This patch fixes all known races in irqfd, and paves the way to restore DEASSIGN support. For details of the eventfd races, please see the patch presumably commited immediately prior to this one.

In a nutshell, we use eventfd_kref_get/put() to properly manage the lifetime of the underlying eventfd. We also use careful coordination with our workqueue to ensure that all irqfd objects have terminated before we allow kvm to shutdown. The logic used for shutdown walks all open irqfds and releases them. This logic can be generalized in the future to allow a subset of irqfds to be released, thus allowing DEASSIGN support.

Quoted 2 lines

Signed-off-by Gregory Haskins <[email protected]>

I think this patch is a shade too tricky. Some explanation why below.

But I think irqfd_pop is a good idea.

Here's an alternative design sketch: add a list of irqfds to be shutdown in kvm, and create a single-threaded workqueue. To kill an irqfd, move it from list of live irqfds to list of dead irqfds, then schedule work on a workqueue that walks this list and kills irqfds.

Quoted 21 lines

---

 virt/kvm/eventfd.c |  144 ++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 110 insertions(+), 34 deletions(-)

virt/kvm/eventfd.c +1 -0

index 9656027..67985cd 100644
@@ -28,6 +28,7 @@
 #include <linux/file.h>
 #include <linux/list.h>
 #include <linux/eventfd.h>
+#include <linux/kref.h>
 
 /*
  * --------------------------------------------------------------------

@@ -36,26 +37,68 @@
  * Credit goes to Avi Kivity for the original idea.
  * --------------------------------------------------------------------

virt/kvm/eventfd.c line 48

Quoted 10 lines

virt/kvm/eventfd.c +6 -0

  */
+
+enum {
+	irqfd_flags_shutdown,
+};
+
 struct _irqfd {
 	struct kvm               *kvm;
+	struct kref              *eventfd;
 

Yay, kref.

virt/kvm/eventfd.c line 56

Quoted 9 lines

virt/kvm/eventfd.c +2 -1

 	int                       gsi;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
-	struct work_struct        inject;
+	struct work_struct        work;
+	unsigned long             flags;
 

Just make it "int shutdown"?

Quoted 18 lines

virt/kvm/eventfd.c +9 -2

 };
 
 static void
-irqfd_inject(struct work_struct *work)
+irqfd_release(struct _irqfd *irqfd)
+{
+	eventfd_kref_put(irqfd->eventfd);
+	kfree(irqfd);
+}
+
+static void
+irqfd_work(struct work_struct *work)
 {
-	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
+	struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
 	struct kvm *kvm = irqfd->kvm;
 

virt/kvm/eventfd.c line 103

Quoted 6 lines

virt/kvm/eventfd.c +1 -4

-	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);
+	if (!test_bit(irqfd_flags_shutdown, &irqfd->flags)) {
 

Why is it safe to test this bit outside of any lock?

virt/kvm/eventfd.c line 76

Quoted 8 lines

virt/kvm/eventfd.c +6 -0

+		/* Inject an interrupt */
+		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);
+	} else {

Not much shared code here - create a separate showdown work struct? They are cheap ...

Quoted 15 lines

virt/kvm/eventfd.c +14 -0

+		/* shutdown the irqfd */
+		struct _irqfd *_irqfd = NULL;
+
+		mutex_lock(&kvm->lock);
+
+		if (!list_empty(&irqfd->list))
+			_irqfd = irqfd;
+
+		if (_irqfd)
+			list_del(&_irqfd->list);
+
+		mutex_unlock(&kvm->lock);
+
+		/*

virt/kvm/eventfd.c

Quoted 4 lines

virt/kvm/eventfd.c +3 -0

+		 * If the item is not currently on the irqfds list, we know
+		 * we are running concurrently with the KVM side trying to
+		 * remove this item as well.

We do? How? As far as I can see list is only empty after it has been created. Generally, it would be better to either use a flag or use list_empty as an indication of going down, but not both.

Quoted 34 lines

virt/kvm/eventfd.c +11 -11

 Since the KVM side should be
+		 * holding the reference now, and will block behind a
+		 * flush_work(), lets just let them do the release() for us
+		 */
+		if (!_irqfd)
+			return;
+
+		irqfd_release(_irqfd);
+	}
 }
 
 static int

@@ -65,25 +108,20 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	unsigned long flags = (unsigned long)key;
 
 	/*
-	 * Assume we will be called with interrupts disabled
+	 * called with interrupts disabled
 	 */
-	if (flags & POLLIN)
-		/*
-		 * Defer the IRQ injection until later since we need to
-		 * acquire the kvm->lock to do so.
-		 */
-		schedule_work(&irqfd->inject);
-
 	if (flags & POLLHUP) {
 		/*
-		 * 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
+		 * ordering is important: shutdown flag must be visible
+		 * before we schedule

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 121

Quoted 5 lines

virt/kvm/eventfd.c +1 -1

 		 */
 		__remove_wait_queue(irqfd->wqh, &irqfd->wait);
-		irqfd->wqh = NULL;
+		set_bit(irqfd_flags_shutdown, &irqfd->flags);
 

So what happens if a previously scheduled work runs on irqfd and sees this flag?

And note that multiple works can run on irqfd
in parallel.

Quoted 49 lines

virt/kvm/eventfd.c +12 -6

 	}
 
+	if (flags & (POLLHUP | POLLIN))
+		schedule_work(&irqfd->work);
+
 	return 0;
 }

@@ -102,6 +140,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
+	struct kref *kref = NULL;
 	int ret;
 	unsigned int events;
 

@@ -112,7 +151,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
-	INIT_WORK(&irqfd->inject, irqfd_inject);
+	INIT_WORK(&irqfd->work, irqfd_work);
 
 	file = eventfd_fget(fd);
 	if (IS_ERR(file)) {

@@ -133,11 +172,13 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	list_add_tail(&irqfd->list, &kvm->irqfds);
 	mutex_unlock(&kvm->lock);
 
-	/*
-	 * Check if there was an event already queued
-	 */
-	if (events & POLLIN)
-		schedule_work(&irqfd->inject);
+	kref = eventfd_kref_get(file);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	irqfd->eventfd = kref;
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise

@@ -145,9 +186,18 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	 */
 	fput(file);
 

virt/kvm/eventfd.c kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 196

Quoted 4 lines

virt/kvm/eventfd.c +3 -0

+	/*
+	 * Check if there was an event already queued
+	 */
 

This comment seems to confuse more that it clarifies:
queued where? eventfd only counts... Just kill the comment?

Quoted 5 lines

virt/kvm/eventfd.c +3 -0

+	if (events & POLLIN)
+		schedule_work(&irqfd->work);
+
 	return 0;
 

virt/kvm/eventfd.c kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 202

Quoted 7 lines

virt/kvm/eventfd.c +3 -0

 fail:
+	if (kref && !IS_ERR(kref))
+		eventfd_kref_put(kref);
+
 	if (file && !IS_ERR(file))
 		fput(file);

let's add a couple more labels and avoid the kref/file check and the initialization above?

Quoted 48 lines

virt/kvm/eventfd.c +27 -8

@@ -161,21 +211,47 @@ kvm_irqfd_init(struct kvm *kvm)
 	INIT_LIST_HEAD(&kvm->irqfds);
 }
 
+static struct _irqfd *
+irqfd_pop(struct kvm *kvm)
+{
+	struct _irqfd *irqfd = NULL;
+
+	mutex_lock(&kvm->lock);
+
+	if (!list_empty(&kvm->irqfds)) {
+		irqfd = list_first_entry(&kvm->irqfds, struct _irqfd, list);
+		list_del(&irqfd->list);
+	}
+
+	mutex_unlock(&kvm->lock);
+
+	return irqfd;
+}
+
 void
 kvm_irqfd_release(struct kvm *kvm)
 {
-	struct _irqfd *irqfd, *tmp;
+	struct _irqfd *irqfd;
 
-	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
-		if (irqfd->wqh)
-			remove_wait_queue(irqfd->wqh, &irqfd->wait);
+	while ((irqfd = irqfd_pop(kvm))) {
 
-		flush_work(&irqfd->inject);
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);
 
-		mutex_lock(&kvm->lock);
-		list_del(&irqfd->list);
-		mutex_unlock(&kvm->lock);
+		/*
+		 * We guarantee there will be no more notifications after
+		 * the remove_wait_queue returns.  Now lets make sure we
+		 * synchronize behind any outstanding work items before
+		 * releasing the resources
+		 */
+		flush_work(&irqfd->work);
 

virt/kvm/eventfd.c kvm_irqfd_init(struct kvm *kvm)

Quoted 10 lines

virt/kvm/eventfd.c +7 -1

-		kfree(irqfd);
+		irqfd_release(irqfd);
 	}
+
+	/*
+	 * We need to wait in case there are any outstanding work-items
+	 * in flight that had already removed themselves from the list
+	 * prior to entry to this function
+	 */
 

Looks scary. Why doesn't the flush above cover all cases?

Quoted 4 lines

virt/kvm/eventfd.c +1 -0

+	flush_scheduled_work();

}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 14 lines · Michael S. Tsirkin

Quoted 12 lines · Gregory Haskins

This patch fixes all known races in irqfd, and paves the way to restore DEASSIGN support. For details of the eventfd races, please see the patch presumably commited immediately prior to this one.

In a nutshell, we use eventfd_kref_get/put() to properly manage the lifetime of the underlying eventfd. We also use careful coordination with our workqueue to ensure that all irqfd objects have terminated before we allow kvm to shutdown. The logic used for shutdown walks all open irqfds and releases them. This logic can be generalized in the future to allow a subset of irqfds to be released, thus allowing DEASSIGN support.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Gregory Haskins <[email protected]>

I think this patch is a shade too tricky. Some explanation why below.

But I think irqfd_pop is a good idea.

Yeah, next we can add something like "irqfd_remove(gsi)" in a similar way to do DEASSIGN.

Quoted 5 lines

Here's an alternative design sketch: add a list of irqfds to be shutdown in kvm, and create a single-threaded workqueue. To kill an irqfd, move it from list of live irqfds to list of dead irqfds, then schedule work on a workqueue that walks this list and kills irqfds.

Yeah, I actually thought of that too, and I think that will work. But then I realized flush_schedule_work does the same thing and its much less code. Perhaps it is also much less clear, too ;) At the very least, you have made me realize I need to comment better.

Quoted 36 lines

Quoted 21 lines

---

 virt/kvm/eventfd.c |  144 ++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 110 insertions(+), 34 deletions(-)

virt/kvm/eventfd.c +1 -0

index 9656027..67985cd 100644
@@ -28,6 +28,7 @@
 #include <linux/file.h>
 #include <linux/list.h>
 #include <linux/eventfd.h>
+#include <linux/kref.h>
 
 /*
  * --------------------------------------------------------------------

@@ -36,26 +37,68 @@
  * Credit goes to Avi Kivity for the original idea.
  * --------------------------------------------------------------------

virt/kvm/eventfd.c line 48

Quoted 10 lines

virt/kvm/eventfd.c +6 -0

  */
+
+enum {
+	irqfd_flags_shutdown,
+};
+
 struct _irqfd {
 	struct kvm               *kvm;
+	struct kref              *eventfd;
 

Yay, kref.

Quoted 12 lines

virt/kvm/eventfd.c line 56

Quoted 9 lines

virt/kvm/eventfd.c +2 -1

 	int                       gsi;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
-	struct work_struct        inject;
+	struct work_struct        work;
+	unsigned long             flags;
 

Just make it "int shutdown"?

Yep, that is probably fine but we will have to use an explicit wmb in lieu of a set_bit operation. NBD.

Quoted 18 lines

Quoted 18 lines

virt/kvm/eventfd.c +9 -2

 };
 
 static void
-irqfd_inject(struct work_struct *work)
+irqfd_release(struct _irqfd *irqfd)
+{
+	eventfd_kref_put(irqfd->eventfd);
+	kfree(irqfd);
+}
+
+static void
+irqfd_work(struct work_struct *work)
 {
-	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
+	struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
 	struct kvm *kvm = irqfd->kvm;
 

Quoted 9 lines

virt/kvm/eventfd.c line 103

Quoted 6 lines

virt/kvm/eventfd.c +1 -4

-	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);
+	if (!test_bit(irqfd_flags_shutdown, &irqfd->flags)) {
 

Why is it safe to test this bit outside of any lock?

Because the ordering is guaranteed to set_bit(), schedule_work(). All we need to do is make sure that the work-queue runs at least one more time after the flag has been set. (Of course, I could have screwed up too, but that was my rationale).

Quoted 13 lines

virt/kvm/eventfd.c line 76

Quoted 8 lines

virt/kvm/eventfd.c +6 -0

+		/* Inject an interrupt */
+		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);
+	} else {

Not much shared code here - create a separate showdown work struct? They are cheap ...

We can't because we need to ensure that all inject-jobs complete before release-jobs. Reading the work-queue code, it would be a deadlock for the release-job to do a flush_work(inject-job). Therefore, both workloads are encapsulated into a single job, and we ensure that the job is launched at least one more time after the flag has been set.

Of course, now that I wrote that, I realize it was clear-as-mud in the code and needs some commenting ;)

Quoted 15 lines

Quoted 15 lines

virt/kvm/eventfd.c +14 -0

+		/* shutdown the irqfd */
+		struct _irqfd *_irqfd = NULL;
+
+		mutex_lock(&kvm->lock);
+
+		if (!list_empty(&irqfd->list))
+			_irqfd = irqfd;
+
+		if (_irqfd)
+			list_del(&_irqfd->list);
+
+		mutex_unlock(&kvm->lock);
+
+		/*

Quoted 9 lines

virt/kvm/eventfd.c

Quoted 4 lines

virt/kvm/eventfd.c +3 -0

+		 * If the item is not currently on the irqfds list, we know
+		 * we are running concurrently with the KVM side trying to
+		 * remove this item as well.

We do? How? As far as I can see list is only empty after it has been created. Generally, it would be better to either use a flag or use list_empty as an indication of going down, but not both.

I think you are mis-reading that. list_empty(&irqfd->list) is the individual irqfd list-item, not the kvm->irqfds list itself. This conditional is telling us whether the irqfd in question is on or off the list (its effectively an irqfd-specific flag), not whether the global list is empty. Again, poor commenting on my part.

Quoted 34 lines

Quoted 34 lines

virt/kvm/eventfd.c +11 -11

 Since the KVM side should be
+		 * holding the reference now, and will block behind a
+		 * flush_work(), lets just let them do the release() for us
+		 */
+		if (!_irqfd)
+			return;
+
+		irqfd_release(_irqfd);
+	}
 }
 
 static int

@@ -65,25 +108,20 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	unsigned long flags = (unsigned long)key;
 
 	/*
-	 * Assume we will be called with interrupts disabled
+	 * called with interrupts disabled
 	 */
-	if (flags & POLLIN)
-		/*
-		 * Defer the IRQ injection until later since we need to
-		 * acquire the kvm->lock to do so.
-		 */
-		schedule_work(&irqfd->inject);
-
 	if (flags & POLLHUP) {
 		/*
-		 * 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
+		 * ordering is important: shutdown flag must be visible
+		 * before we schedule

Quoted 8 lines

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 121

Quoted 5 lines

virt/kvm/eventfd.c +1 -1

 		 */
 		__remove_wait_queue(irqfd->wqh, &irqfd->wait);
-		irqfd->wqh = NULL;
+		set_bit(irqfd_flags_shutdown, &irqfd->flags);
 

So what happens if a previously scheduled work runs on irqfd and sees this flag?

My original thought was "thats ok", but now that you mention it I am not so sure. Ill give it some more thought because maybe you are on to something.

Quoted 3 lines

And note that multiple works can run on irqfd
in parallel.

They can? I thought work-queue items were guaranteed to only schedule once? If what you say is true, its broken, I agree, and Ill need to revisit. Let me get back to you.

Quoted 49 lines

Quoted 49 lines

virt/kvm/eventfd.c +12 -6

 	}
 
+	if (flags & (POLLHUP | POLLIN))
+		schedule_work(&irqfd->work);
+
 	return 0;
 }

@@ -102,6 +140,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
+	struct kref *kref = NULL;
 	int ret;
 	unsigned int events;
 

@@ -112,7 +151,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
-	INIT_WORK(&irqfd->inject, irqfd_inject);
+	INIT_WORK(&irqfd->work, irqfd_work);
 
 	file = eventfd_fget(fd);
 	if (IS_ERR(file)) {

@@ -133,11 +172,13 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	list_add_tail(&irqfd->list, &kvm->irqfds);
 	mutex_unlock(&kvm->lock);
 
-	/*
-	 * Check if there was an event already queued
-	 */
-	if (events & POLLIN)
-		schedule_work(&irqfd->inject);
+	kref = eventfd_kref_get(file);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	irqfd->eventfd = kref;
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise

@@ -145,9 +186,18 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	 */
 	fput(file);
 

Quoted 9 lines

virt/kvm/eventfd.c kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 196

Quoted 4 lines

virt/kvm/eventfd.c +3 -0

+	/*
+	 * Check if there was an event already queued
+	 */
 

This comment seems to confuse more that it clarifies:
queued where? eventfd only counts... Just kill the comment?

non-zero values in eventfd are "queued" as a signal. This test just checks if an interrupt was already injected before we registered.

Quoted 5 lines

Quoted 5 lines

virt/kvm/eventfd.c +3 -0

+	if (events & POLLIN)
+		schedule_work(&irqfd->work);
+
 	return 0;
 

Quoted 11 lines

virt/kvm/eventfd.c kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 202

Quoted 7 lines

virt/kvm/eventfd.c +3 -0

 fail:
+	if (kref && !IS_ERR(kref))
+		eventfd_kref_put(kref);
+
 	if (file && !IS_ERR(file))
 		fput(file);

let's add a couple more labels and avoid the kref/file check and the initialization above?

I think that just makes it more confusing, personally. But I will give it some thought.

Quoted 48 lines

Quoted 48 lines

virt/kvm/eventfd.c +27 -8

@@ -161,21 +211,47 @@ kvm_irqfd_init(struct kvm *kvm)
 	INIT_LIST_HEAD(&kvm->irqfds);
 }
 
+static struct _irqfd *
+irqfd_pop(struct kvm *kvm)
+{
+	struct _irqfd *irqfd = NULL;
+
+	mutex_lock(&kvm->lock);
+
+	if (!list_empty(&kvm->irqfds)) {
+		irqfd = list_first_entry(&kvm->irqfds, struct _irqfd, list);
+		list_del(&irqfd->list);
+	}
+
+	mutex_unlock(&kvm->lock);
+
+	return irqfd;
+}
+
 void
 kvm_irqfd_release(struct kvm *kvm)
 {
-	struct _irqfd *irqfd, *tmp;
+	struct _irqfd *irqfd;
 
-	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
-		if (irqfd->wqh)
-			remove_wait_queue(irqfd->wqh, &irqfd->wait);
+	while ((irqfd = irqfd_pop(kvm))) {
 
-		flush_work(&irqfd->inject);
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);
 
-		mutex_lock(&kvm->lock);
-		list_del(&irqfd->list);
-		mutex_unlock(&kvm->lock);
+		/*
+		 * We guarantee there will be no more notifications after
+		 * the remove_wait_queue returns.  Now lets make sure we
+		 * synchronize behind any outstanding work items before
+		 * releasing the resources
+		 */
+		flush_work(&irqfd->work);
 

Quoted 13 lines

virt/kvm/eventfd.c kvm_irqfd_init(struct kvm *kvm)

Quoted 10 lines

virt/kvm/eventfd.c +7 -1

-		kfree(irqfd);
+		irqfd_release(irqfd);
 	}
+
+	/*
+	 * We need to wait in case there are any outstanding work-items
+	 * in flight that had already removed themselves from the list
+	 * prior to entry to this function
+	 */
 

Looks scary. Why doesn't the flush above cover all cases?

The path inside the while() is for when KVM wins the race and finds the item in the list. It atomically removes it, and is responsible for freeing it in a coordinated way. In this case, we must block with the flush_work() before we can irqfd_release() so that we do not yank the memory out from under a running work-item.

The flush_scheduled_work() is for when eventfd wins the race and has already removed itself from the list in the "shutdown" path in the work-item. We want to make sure that kvm_irqfd_release() cannot return until all work-items have exited to prevent something like the kvm.ko module unloading while the work-item is still in flight. Thanks Michael,
-Greg

Quoted 10 lines

Quoted 4 lines

virt/kvm/eventfd.c +1 -0

+	flush_scheduled_work();

}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 35 lines · Gregory Haskins

Quoted 15 lines · Michael S. Tsirkin

Quoted 12 lines · Gregory Haskins

This patch fixes all known races in irqfd, and paves the way to restore DEASSIGN support. For details of the eventfd races, please see the patch presumably commited immediately prior to this one.

In a nutshell, we use eventfd_kref_get/put() to properly manage the lifetime of the underlying eventfd. We also use careful coordination with our workqueue to ensure that all irqfd objects have terminated before we allow kvm to shutdown. The logic used for shutdown walks all open irqfds and releases them. This logic can be generalized in the future to allow a subset of irqfds to be released, thus allowing DEASSIGN support.

Quoted 8 lines

Quoted 3 lines

Signed-off-by Gregory Haskins <[email protected]>

I think this patch is a shade too tricky. Some explanation why below.

But I think irqfd_pop is a good idea.

Yeah, next we can add something like "irqfd_remove(gsi)" in a similar way to do DEASSIGN.

Quoted 6 lines

Here's an alternative design sketch: add a list of irqfds to be shutdown in kvm, and create a single-threaded workqueue. To kill an irqfd, move it from list of live irqfds to list of dead irqfds, then schedule work on a workqueue that walks this list and kills irqfds.

Quoted 5 lines

Yeah, I actually thought of that too, and I think that will work. But then I realized flush_schedule_work does the same thing and its much less code. Perhaps it is also much less clear, too ;) At the very least, you have made me realize I need to comment better.

Not really, it's impossible to document all races one have thought about and avoided.

Quoted 104 lines

Quoted 38 lines

Quoted 24 lines

---

 virt/kvm/eventfd.c |  144 ++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 110 insertions(+), 34 deletions(-)

virt/kvm/eventfd.c +1 -0

index 9656027..67985cd 100644
@@ -28,6 +28,7 @@
 #include <linux/file.h>
 #include <linux/list.h>
 #include <linux/eventfd.h>
+#include <linux/kref.h>
 
 /*
  * --------------------------------------------------------------------

@@ -36,26 +37,68 @@
  * Credit goes to Avi Kivity for the original idea.
  * --------------------------------------------------------------------
  */

virt/kvm/eventfd.c line 49

Quoted 10 lines

virt/kvm/eventfd.c +6 -0

+
+enum {
+	irqfd_flags_shutdown,
+};
+
 struct _irqfd {
 	struct kvm               *kvm;
+	struct kref              *eventfd;
 
 

Yay, kref.

Quoted 13 lines

virt/kvm/eventfd.c line 69

Quoted 10 lines

virt/kvm/eventfd.c +2 -1

 	int                       gsi;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
-	struct work_struct        inject;
+	struct work_struct        work;
+	unsigned long             flags;
 
 

Just make it "int shutdown"?

Yep, that is probably fine but we will have to use an explicit wmb in lieu of a set_bit operation. NBD.

Quoted 19 lines

Quoted 19 lines

virt/kvm/eventfd.c +9 -2

 };
 
 static void
-irqfd_inject(struct work_struct *work)
+irqfd_release(struct _irqfd *irqfd)
+{
+	eventfd_kref_put(irqfd->eventfd);
+	kfree(irqfd);
+}
+
+static void
+irqfd_work(struct work_struct *work)
 {
-	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
+	struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
 	struct kvm *kvm = irqfd->kvm;
 

Quoted 10 lines

virt/kvm/eventfd.c line 70

Quoted 7 lines

virt/kvm/eventfd.c +1 -4

-	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);
+	if (!test_bit(irqfd_flags_shutdown, &irqfd->flags)) {

Why is it safe to test this bit outside of any lock?

Because the ordering is guaranteed to set_bit(), schedule_work(). All we need to do is make sure that the work-queue runs at least one more time after the flag has been set. (Of course, I could have screwed up too, but that was my rationale).

Quoted 14 lines

virt/kvm/eventfd.c line 76

Quoted 10 lines

virt/kvm/eventfd.c +6 -0

+		/* Inject an interrupt */
+		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);
+	} else {

Not much shared code here - create a separate showdown work struct? They are cheap ...

Quoted 6 lines

We can't because we need to ensure that all inject-jobs complete before release-jobs. Reading the work-queue code, it would be a deadlock for the release-job to do a flush_work(inject-job). Therefore, both workloads are encapsulated into a single job, and we ensure that the job is launched at least one more time after the flag has been set.

AFAIK schedule_work does not give you in-order guarantees - it's multithreaded. you will have to create a single-threaded workqueue if you want in order execution.

Quoted 31 lines

Of course, now that I wrote that, I realize it was clear-as-mud in the code and needs some commenting ;)

Quoted 16 lines

Quoted 16 lines

virt/kvm/eventfd.c +14 -0

+		/* shutdown the irqfd */
+		struct _irqfd *_irqfd = NULL;
+
+		mutex_lock(&kvm->lock);
+
+		if (!list_empty(&irqfd->list))
+			_irqfd = irqfd;
+
+		if (_irqfd)
+			list_del(&_irqfd->list);
+
+		mutex_unlock(&kvm->lock);
+
+		/*

Quoted 10 lines

virt/kvm/eventfd.c

Quoted 5 lines

virt/kvm/eventfd.c +3 -0

+		 * If the item is not currently on the irqfds list, we know
+		 * we are running concurrently with the KVM side trying to
+		 * remove this item as well.

We do? How? As far as I can see list is only empty after it has been created. Generally, it would be better to either use a flag or use list_empty as an indication of going down, but not both.

Quoted 6 lines

I think you are mis-reading that. list_empty(&irqfd->list) is the individual irqfd list-item, not the kvm->irqfds list itself. This conditional is telling us whether the irqfd in question is on or off the list (its effectively an irqfd-specific flag), not whether the global list is empty. Again, poor commenting on my part.

Yes, but you do INIT_LIST_HEAD in a single place. Once you add irqfd->list to a list, it won't be empty until you init it again.

Quoted 118 lines

Quoted 36 lines

Quoted 36 lines

Since the KVM side should be

virt/kvm/eventfd.c +11 -11

+		 * holding the reference now, and will block behind a
+		 * flush_work(), lets just let them do the release() for us
+		 */
+		if (!_irqfd)
+			return;
+
+		irqfd_release(_irqfd);
+	}
 }
 
 static int

@@ -65,25 +108,20 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	unsigned long flags = (unsigned long)key;
 
 	/*
-	 * Assume we will be called with interrupts disabled
+	 * called with interrupts disabled
 	 */
-	if (flags & POLLIN)
-		/*
-		 * Defer the IRQ injection until later since we need to
-		 * acquire the kvm->lock to do so.
-		 */
-		schedule_work(&irqfd->inject);
-
 	if (flags & POLLHUP) {
 		/*
-		 * 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
+		 * ordering is important: shutdown flag must be visible
+		 * before we schedule

Quoted 9 lines

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 127

Quoted 6 lines

virt/kvm/eventfd.c +1 -1

 		 */
 		__remove_wait_queue(irqfd->wqh, &irqfd->wait);
-		irqfd->wqh = NULL;
+		set_bit(irqfd_flags_shutdown, &irqfd->flags);
 
 

So what happens if a previously scheduled work runs on irqfd and sees this flag?

My original thought was "thats ok", but now that you mention it I am not so sure. Ill give it some more thought because maybe you are on to something.

Quoted 4 lines

And note that multiple works can run on irqfd
in parallel.

They can? I thought work-queue items were guaranteed to only schedule once? If what you say is true, its broken, I agree, and Ill need to revisit. Let me get back to you.

Quoted 60 lines

Quoted 47 lines

virt/kvm/eventfd.c +3 -0

 	}
 
+	if (flags & (POLLHUP | POLLIN))
+		schedule_work(&irqfd->work);
+
 	return 0;

}

virt/kvm/eventfd.c +9 -6

@@ -102,6 +140,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
+	struct kref *kref = NULL;
 	int ret;
 	unsigned int events;
 

@@ -112,7 +151,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
-	INIT_WORK(&irqfd->inject, irqfd_inject);
+	INIT_WORK(&irqfd->work, irqfd_work);
 
 	file = eventfd_fget(fd);
 	if (IS_ERR(file)) {

@@ -133,11 +172,13 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	list_add_tail(&irqfd->list, &kvm->irqfds);
 	mutex_unlock(&kvm->lock);
 
-	/*
-	 * Check if there was an event already queued
-	 */
-	if (events & POLLIN)
-		schedule_work(&irqfd->inject);
+	kref = eventfd_kref_get(file);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	irqfd->eventfd = kref;
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise

virt/kvm/eventfd.c kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 193

Quoted 8 lines

virt/kvm/eventfd.c +3 -0

@@ -145,9 +186,18 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	 */
 	fput(file);
 
+	/*
+	 * Check if there was an event already queued
+	 */
 
 

This comment seems to confuse more that it clarifies:
queued where? eventfd only counts... Just kill the comment?

Quoted 3 lines

non-zero values in eventfd are "queued" as a signal. This test just checks if an interrupt was already injected before we registered.

After have understood the code I see what you mean, but the comment wasn't helpful and is better left out.

Quoted 113 lines

Quoted 6 lines

Quoted 6 lines

virt/kvm/eventfd.c +3 -0

+	if (events & POLLIN)
+		schedule_work(&irqfd->work);
+
 	return 0;
 

Quoted 12 lines

Quoted 8 lines

virt/kvm/eventfd.c +3 -0

 fail:
+	if (kref && !IS_ERR(kref))
+		eventfd_kref_put(kref);
+
 	if (file && !IS_ERR(file))

fput(file);

let's add a couple more labels and avoid the kref/file check and the initialization above?

I think that just makes it more confusing, personally. But I will give it some thought.

Quoted 49 lines

Quoted 49 lines

virt/kvm/eventfd.c +27 -8

@@ -161,21 +211,47 @@ kvm_irqfd_init(struct kvm *kvm)
 	INIT_LIST_HEAD(&kvm->irqfds);
 }
 
+static struct _irqfd *
+irqfd_pop(struct kvm *kvm)
+{
+	struct _irqfd *irqfd = NULL;
+
+	mutex_lock(&kvm->lock);
+
+	if (!list_empty(&kvm->irqfds)) {
+		irqfd = list_first_entry(&kvm->irqfds, struct _irqfd, list);
+		list_del(&irqfd->list);
+	}
+
+	mutex_unlock(&kvm->lock);
+
+	return irqfd;
+}
+
 void
 kvm_irqfd_release(struct kvm *kvm)
 {
-	struct _irqfd *irqfd, *tmp;
+	struct _irqfd *irqfd;
 
-	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
-		if (irqfd->wqh)
-			remove_wait_queue(irqfd->wqh, &irqfd->wait);
+	while ((irqfd = irqfd_pop(kvm))) {
 
-		flush_work(&irqfd->inject);
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);
 
-		mutex_lock(&kvm->lock);
-		list_del(&irqfd->list);
-		mutex_unlock(&kvm->lock);
+		/*
+		 * We guarantee there will be no more notifications after
+		 * the remove_wait_queue returns.  Now lets make sure we
+		 * synchronize behind any outstanding work items before
+		 * releasing the resources
+		 */
+		flush_work(&irqfd->work);
 

Quoted 14 lines

virt/kvm/eventfd.c kvm_irqfd_init(struct kvm *kvm)

Quoted 11 lines

virt/kvm/eventfd.c +7 -1

-		kfree(irqfd);
+		irqfd_release(irqfd);
 	}
+
+	/*
+	 * We need to wait in case there are any outstanding work-items
+	 * in flight that had already removed themselves from the list
+	 * prior to entry to this function
+	 */
 

Looks scary. Why doesn't the flush above cover all cases?

The path inside the while() is for when KVM wins the race and finds the item in the list. It atomically removes it, and is responsible for freeing it in a coordinated way. In this case, we must block with the flush_work() before we can irqfd_release() so that we do not yank the memory out from under a running work-item.

The flush_scheduled_work() is for when eventfd wins the race and has already removed itself from the list in the "shutdown" path in the work-item. We want to make sure that kvm_irqfd_release() cannot return until all work-items have exited to prevent something like the kvm.ko module unloading while the work-item is still in flight. Thanks Michael,
-Greg

Quoted 13 lines

Quoted 6 lines

virt/kvm/eventfd.c +1 -0

+	flush_scheduled_work();

}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 37 lines · Michael S. Tsirkin

Quoted 35 lines · Gregory Haskins

Quoted 15 lines · Michael S. Tsirkin

Quoted 12 lines · Gregory Haskins

This patch fixes all known races in irqfd, and paves the way to restore DEASSIGN support. For details of the eventfd races, please see the patch presumably commited immediately prior to this one.

In a nutshell, we use eventfd_kref_get/put() to properly manage the lifetime of the underlying eventfd. We also use careful coordination with our workqueue to ensure that all irqfd objects have terminated before we allow kvm to shutdown. The logic used for shutdown walks all open irqfds and releases them. This logic can be generalized in the future to allow a subset of irqfds to be released, thus allowing DEASSIGN support.

Quoted 8 lines

Quoted 3 lines

Signed-off-by Gregory Haskins <[email protected]>

I think this patch is a shade too tricky. Some explanation why below.

But I think irqfd_pop is a good idea.

Yeah, next we can add something like "irqfd_remove(gsi)" in a similar way to do DEASSIGN.

Quoted 6 lines

Here's an alternative design sketch: add a list of irqfds to be shutdown in kvm, and create a single-threaded workqueue. To kill an irqfd, move it from list of live irqfds to list of dead irqfds, then schedule work on a workqueue that walks this list and kills irqfds.

Quoted 9 lines

Quoted 5 lines

Yeah, I actually thought of that too, and I think that will work. But then I realized flush_schedule_work does the same thing and its much less code. Perhaps it is also much less clear, too ;) At the very least, you have made me realize I need to comment better.

Not really, it's impossible to document all races one have thought about and avoided.

Heh, that is a very astute observation.

Quoted 111 lines

Quoted 110 lines

Quoted 38 lines

Quoted 24 lines

---

 virt/kvm/eventfd.c |  144 ++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 110 insertions(+), 34 deletions(-)

virt/kvm/eventfd.c +1 -0

index 9656027..67985cd 100644
@@ -28,6 +28,7 @@
 #include <linux/file.h>
 #include <linux/list.h>
 #include <linux/eventfd.h>
+#include <linux/kref.h>
 
 /*
  * --------------------------------------------------------------------

@@ -36,26 +37,68 @@
  * Credit goes to Avi Kivity for the original idea.
  * --------------------------------------------------------------------
  */

virt/kvm/eventfd.c line 49

Quoted 10 lines

virt/kvm/eventfd.c +6 -0

+
+enum {
+	irqfd_flags_shutdown,
+};
+
 struct _irqfd {
 	struct kvm               *kvm;
+	struct kref              *eventfd;
 
 

Yay, kref.

Quoted 13 lines

virt/kvm/eventfd.c line 69

Quoted 10 lines

virt/kvm/eventfd.c +2 -1

 	int                       gsi;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
-	struct work_struct        inject;
+	struct work_struct        work;
+	unsigned long             flags;
 
 

Just make it "int shutdown"?

Yep, that is probably fine but we will have to use an explicit wmb in lieu of a set_bit operation. NBD.

Quoted 19 lines

Quoted 19 lines

virt/kvm/eventfd.c +9 -2

 };
 
 static void
-irqfd_inject(struct work_struct *work)
+irqfd_release(struct _irqfd *irqfd)
+{
+	eventfd_kref_put(irqfd->eventfd);
+	kfree(irqfd);
+}
+
+static void
+irqfd_work(struct work_struct *work)
 {
-	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
+	struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
 	struct kvm *kvm = irqfd->kvm;
 

Quoted 10 lines

virt/kvm/eventfd.c line 70

Quoted 7 lines

virt/kvm/eventfd.c +1 -4

-	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);
+	if (!test_bit(irqfd_flags_shutdown, &irqfd->flags)) {

Why is it safe to test this bit outside of any lock?

Because the ordering is guaranteed to set_bit(), schedule_work(). All we need to do is make sure that the work-queue runs at least one more time after the flag has been set. (Of course, I could have screwed up too, but that was my rationale).

Quoted 14 lines

virt/kvm/eventfd.c line 76

Quoted 10 lines

virt/kvm/eventfd.c +6 -0

+		/* Inject an interrupt */
+		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);
+	} else {

Not much shared code here - create a separate showdown work struct? They are cheap ...

We can't because we need to ensure that all inject-jobs complete before release-jobs. Reading the work-queue code, it would be a deadlock for the release-job to do a flush_work(inject-job). Therefore, both workloads are encapsulated into a single job, and we ensure that the job is launched at least one more time after the flag has been set.

Quoted 4 lines

AFAIK schedule_work does not give you in-order guarantees - it's multithreaded. you will have to create a single-threaded workqueue if you want in order execution.

Right, that was my understanding as well. Thats why I do both tasks from a single work-item ;)

Quoted 38 lines

Quoted 37 lines

Of course, now that I wrote that, I realize it was clear-as-mud in the code and needs some commenting ;)

Quoted 16 lines

Quoted 16 lines

virt/kvm/eventfd.c +14 -0

+		/* shutdown the irqfd */
+		struct _irqfd *_irqfd = NULL;
+
+		mutex_lock(&kvm->lock);
+
+		if (!list_empty(&irqfd->list))
+			_irqfd = irqfd;
+
+		if (_irqfd)
+			list_del(&_irqfd->list);
+
+		mutex_unlock(&kvm->lock);
+
+		/*

Quoted 10 lines

virt/kvm/eventfd.c

Quoted 5 lines

virt/kvm/eventfd.c +3 -0

+		 * If the item is not currently on the irqfds list, we know
+		 * we are running concurrently with the KVM side trying to
+		 * remove this item as well.

We do? How? As far as I can see list is only empty after it has been created. Generally, it would be better to either use a flag or use list_empty as an indication of going down, but not both.

I think you are mis-reading that. list_empty(&irqfd->list) is the individual irqfd list-item, not the kvm->irqfds list itself. This conditional is telling us whether the irqfd in question is on or off the list (its effectively an irqfd-specific flag), not whether the global list is empty. Again, poor commenting on my part.

Quoted 3 lines

Yes, but you do INIT_LIST_HEAD in a single place. Once you add irqfd->list to a list, it won't be empty until you init it again.

Good point. I need list_del_init() and then it would work, right?

Quoted 122 lines

Quoted 121 lines

Quoted 36 lines

Quoted 36 lines

Since the KVM side should be

virt/kvm/eventfd.c +11 -11

+		 * holding the reference now, and will block behind a
+		 * flush_work(), lets just let them do the release() for us
+		 */
+		if (!_irqfd)
+			return;
+
+		irqfd_release(_irqfd);
+	}
 }
 
 static int

@@ -65,25 +108,20 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	unsigned long flags = (unsigned long)key;
 
 	/*
-	 * Assume we will be called with interrupts disabled
+	 * called with interrupts disabled
 	 */
-	if (flags & POLLIN)
-		/*
-		 * Defer the IRQ injection until later since we need to
-		 * acquire the kvm->lock to do so.
-		 */
-		schedule_work(&irqfd->inject);
-
 	if (flags & POLLHUP) {
 		/*
-		 * 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
+		 * ordering is important: shutdown flag must be visible
+		 * before we schedule

Quoted 9 lines

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 127

Quoted 6 lines

virt/kvm/eventfd.c +1 -1

 		 */
 		__remove_wait_queue(irqfd->wqh, &irqfd->wait);
-		irqfd->wqh = NULL;
+		set_bit(irqfd_flags_shutdown, &irqfd->flags);
 
 

So what happens if a previously scheduled work runs on irqfd and sees this flag?

My original thought was "thats ok", but now that you mention it I am not so sure. Ill give it some more thought because maybe you are on to something.

Quoted 4 lines

And note that multiple works can run on irqfd
in parallel.

They can? I thought work-queue items were guaranteed to only schedule once? If what you say is true, its broken, I agree, and Ill need to revisit. Let me get back to you.

Quoted 50 lines

Quoted 50 lines

virt/kvm/eventfd.c +3 -0

 	}
 
+	if (flags & (POLLHUP | POLLIN))
+		schedule_work(&irqfd->work);
+
 	return 0;

}

virt/kvm/eventfd.c +9 -6

@@ -102,6 +140,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
+	struct kref *kref = NULL;
 	int ret;
 	unsigned int events;
 

@@ -112,7 +151,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
-	INIT_WORK(&irqfd->inject, irqfd_inject);
+	INIT_WORK(&irqfd->work, irqfd_work);
 
 	file = eventfd_fget(fd);
 	if (IS_ERR(file)) {

@@ -133,11 +172,13 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	list_add_tail(&irqfd->list, &kvm->irqfds);
 	mutex_unlock(&kvm->lock);
 
-	/*
-	 * Check if there was an event already queued
-	 */
-	if (events & POLLIN)
-		schedule_work(&irqfd->inject);
+	kref = eventfd_kref_get(file);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	irqfd->eventfd = kref;
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise

@@ -145,9 +186,18 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	 */
 	fput(file);
 

Quoted 10 lines

virt/kvm/eventfd.c kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 203

Quoted 5 lines

virt/kvm/eventfd.c +3 -0

+	/*
+	 * Check if there was an event already queued
+	 */
 
 

This comment seems to confuse more that it clarifies:
queued where? eventfd only counts... Just kill the comment?

non-zero values in eventfd are "queued" as a signal. This test just checks if an interrupt was already injected before we registered.

Quoted 3 lines

After have understood the code I see what you mean, but the comment wasn't helpful and is better left out.

Ok. What if I say "Check if an interrupt is already pending before we registered the callback" ;)

Quoted 116 lines

Quoted 113 lines

Quoted 6 lines

Quoted 6 lines

virt/kvm/eventfd.c +3 -0

+	if (events & POLLIN)
+		schedule_work(&irqfd->work);
+
 	return 0;
 

Quoted 12 lines

Quoted 8 lines

virt/kvm/eventfd.c +3 -0

 fail:
+	if (kref && !IS_ERR(kref))
+		eventfd_kref_put(kref);
+
 	if (file && !IS_ERR(file))

fput(file);

let's add a couple more labels and avoid the kref/file check and the initialization above?

I think that just makes it more confusing, personally. But I will give it some thought.

Quoted 49 lines

Quoted 49 lines

virt/kvm/eventfd.c +27 -8

@@ -161,21 +211,47 @@ kvm_irqfd_init(struct kvm *kvm)
 	INIT_LIST_HEAD(&kvm->irqfds);
 }
 
+static struct _irqfd *
+irqfd_pop(struct kvm *kvm)
+{
+	struct _irqfd *irqfd = NULL;
+
+	mutex_lock(&kvm->lock);
+
+	if (!list_empty(&kvm->irqfds)) {
+		irqfd = list_first_entry(&kvm->irqfds, struct _irqfd, list);
+		list_del(&irqfd->list);
+	}
+
+	mutex_unlock(&kvm->lock);
+
+	return irqfd;
+}
+
 void
 kvm_irqfd_release(struct kvm *kvm)
 {
-	struct _irqfd *irqfd, *tmp;
+	struct _irqfd *irqfd;
 
-	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
-		if (irqfd->wqh)
-			remove_wait_queue(irqfd->wqh, &irqfd->wait);
+	while ((irqfd = irqfd_pop(kvm))) {
 
-		flush_work(&irqfd->inject);
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);
 
-		mutex_lock(&kvm->lock);
-		list_del(&irqfd->list);
-		mutex_unlock(&kvm->lock);
+		/*
+		 * We guarantee there will be no more notifications after
+		 * the remove_wait_queue returns.  Now lets make sure we
+		 * synchronize behind any outstanding work items before
+		 * releasing the resources
+		 */
+		flush_work(&irqfd->work);
 

Quoted 14 lines

virt/kvm/eventfd.c kvm_irqfd_init(struct kvm *kvm)

Quoted 11 lines

virt/kvm/eventfd.c +7 -1

-		kfree(irqfd);
+		irqfd_release(irqfd);
 	}
+
+	/*
+	 * We need to wait in case there are any outstanding work-items
+	 * in flight that had already removed themselves from the list
+	 * prior to entry to this function
+	 */
 

Looks scary. Why doesn't the flush above cover all cases?

The path inside the while() is for when KVM wins the race and finds the item in the list. It atomically removes it, and is responsible for freeing it in a coordinated way. In this case, we must block with the flush_work() before we can irqfd_release() so that we do not yank the memory out from under a running work-item.

The flush_scheduled_work() is for when eventfd wins the race and has already removed itself from the list in the "shutdown" path in the work-item. We want to make sure that kvm_irqfd_release() cannot return until all work-items have exited to prevent something like the kvm.ko module unloading while the work-item is still in flight. Thanks Michael,
-Greg

Quoted 13 lines

Quoted 6 lines

virt/kvm/eventfd.c +1 -0

+	flush_scheduled_work();

}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Not really, it's impossible to document all races one have thought about and avoided.

Quoted 2 lines · Michael S. Tsirkin

Not really, it's impossible to document all races one have thought about and avoided.

Well, when some new code has non-trivial locking/racing logics, you better document it as clearly as possible, akpm announced time ago.

Quoted 5 lines · Davide Libenzi

Quoted 2 lines · Michael S. Tsirkin

Not really, it's impossible to document all races one have thought about and avoided.

Quoted 2 lines

Well, when some new code has non-trivial locking/racing logics, you better document it as clearly as possible, akpm announced time ago.

Absolutely. But IMO documentation in the form of "fields A B C are protected by lock X" in a single place near field declaration is better than "we take lock X here to protect against race with user thread using fields A B C" sprinkled all over the code.

Quoted 4 lines

Absolutely. But IMO documentation in the form of "fields A B C are protected by lock X" in a single place near field declaration is better than "we take lock X here to protect against race with user thread using fields A B C" sprinkled all over the code.

Exactly what akpm suggested.

Quoted 4 lines · Michael S. Tsirkin

And note that multiple works can run on irqfd
in parallel.

They can? I thought work-queue items were guaranteed to only schedule once? If what you say is true, its broken, I agree, and Ill need to revisit. Let me get back to you.