irqfd fixes and enhancements

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, as it stands today it is not possible to use this feature of eventfd in a race-free way. This patch changes the POLLHUP code to use the locked variant to rectify this problem.

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

CC Davide Libenzi <[email protected]>

---

 fs/eventfd.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

fs/eventfd.c +0 -0

index d9849a1..31d12de 100644
@@ -105,12 +105,7 @@ static int eventfd_release(struct inode *inode, struct file *file)
 {
 	struct eventfd_ctx *ctx = file->private_data;
 

We currently create this wq on module_init, which may be wasteful if the host never creates a guest that uses irqfd. This patch changes the algorithm so that the workqueue is only created when at least one guest is using irqfd. The queue is cleaned up when the last guest using irqfd is shutdown.

To keep things simple, we only check whether the guest has tried to create an irqfd, not whether there are actually irqfds active.

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

---

 include/linux/kvm_host.h |    1 
 virt/kvm/eventfd.c       |  100 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 75 insertions(+), 26 deletions(-)

include/linux/kvm_host.h +1 -0

index 8e04a34..cd1a0f3 100644
@@ -149,6 +149,7 @@ struct kvm {
 	struct {
 		spinlock_t        lock;
 		struct list_head  items;
+		bool              init;
 	} irqfds;
 #endif
 	struct kvm_vm_stat stat;

virt/kvm/eventfd.c +46 -2

index 4092b8d..fcc3469 100644
@@ -49,7 +49,16 @@ struct _irqfd {
 	struct work_struct        shutdown;
 };
 
-static struct workqueue_struct *irqfd_cleanup_wq;
+struct _irqfd_cleanup {
+	struct mutex             lock;
+	int                      refs;
+	struct workqueue_struct *wq;
+};
+
+static struct _irqfd_cleanup irqfd_cleanup = {
+	.lock = __MUTEX_INITIALIZER(irqfd_cleanup.lock),
+	.refs = 0,
+};
 
 static void
 irqfd_inject(struct work_struct *work)

@@ -110,7 +119,7 @@ irqfd_deactivate(struct _irqfd *irqfd)
 
 	list_del_init(&irqfd->list);
 
-	queue_work(irqfd_cleanup_wq, &irqfd->shutdown);
+	queue_work(irqfd_cleanup.wq, &irqfd->shutdown);
 }
 
 /*

@@ -161,6 +170,62 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 	add_wait_queue(wqh, &irqfd->wait);
 }
 
+/*
+ * create a host-wide workqueue for issuing deferred shutdown requests
+ * aggregated from all vm* instances. We need our own isolated single-thread
+ * queue to prevent deadlock against flushing the normal work-queue.
+ */
+static int
+irqfd_cleanup_init(struct kvm *kvm)
+{
+	int ret = 0;
+
+	mutex_lock(&irqfd_cleanup.lock);
+
+	/*
+	 * Check the current init state from within the lock so that we
+	 * sync all users to the thread creation.
+	 */
+	if (kvm->irqfds.init)
+		goto out;
+
+	if (!irqfd_cleanup.refs) {
+		struct workqueue_struct *wq;
+
+		wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
+		if (!wq) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		irqfd_cleanup.wq = wq;
+	}
+
+	irqfd_cleanup.refs++;
+	kvm->irqfds.init = true;
+
+out:

virt/kvm/eventfd.c +9 -24

+}
+
 static int
 kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 {

@@ -170,6 +235,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	int ret;
 	unsigned int events;
 
+	ret = irqfd_cleanup_init(kvm);
+	if (ret < 0)
+		return ret;
+
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
 	if (!irqfd)
 		return -ENOMEM;

@@ -268,7 +337,7 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	 * so that we guarantee there will not be any more interrupts on this
 	 * gsi once this deassign function returns.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
+	flush_workqueue(irqfd_cleanup.wq);
 
 	return 0;
 }

@@ -302,28 +371,7 @@ kvm_irqfd_release(struct kvm *kvm)
 	 * Block until we know all outstanding shutdown jobs have completed
 	 * since we do not take a kvm* reference.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
-
-}
-
-/*
- * create a host-wide workqueue for issuing deferred shutdown requests
- * aggregated from all vm* instances. We need our own isolated single-thread
- * queue to prevent deadlock against flushing the normal work-queue.
- */
-static int __init irqfd_module_init(void)
-{
-	irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
-	if (!irqfd_cleanup_wq)
-		return -ENOMEM;
-
-	return 0;
-}
+	flush_workqueue(irqfd_cleanup.wq);
+	irqfd_cleanup_release(kvm);
 
-static void __exit irqfd_module_exit(void)
-{
-	destroy_workqueue(irqfd_cleanup_wq);
 }
-
-module_init(irqfd_module_init);
-module_exit(irqfd_module_exit);

Quoted 104 lines · Gregory Haskins

We currently create this wq on module_init, which may be wasteful if the host never creates a guest that uses irqfd. This patch changes the algorithm so that the workqueue is only created when at least one guest is using irqfd. The queue is cleaned up when the last guest using irqfd is shutdown.

To keep things simple, we only check whether the guest has tried to create an irqfd, not whether there are actually irqfds active.

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

---

 include/linux/kvm_host.h |    1 
 virt/kvm/eventfd.c       |  100 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 75 insertions(+), 26 deletions(-)

include/linux/kvm_host.h +1 -0

index 8e04a34..cd1a0f3 100644
@@ -149,6 +149,7 @@ struct kvm {
 	struct {
 		spinlock_t        lock;
 		struct list_head  items;
+		bool              init;
 	} irqfds;
 #endif
 	struct kvm_vm_stat stat;

virt/kvm/eventfd.c +52 -2

index 4092b8d..fcc3469 100644
@@ -49,7 +49,16 @@ struct _irqfd {
 	struct work_struct        shutdown;
 };
 
-static struct workqueue_struct *irqfd_cleanup_wq;
+struct _irqfd_cleanup {
+	struct mutex             lock;
+	int                      refs;
+	struct workqueue_struct *wq;
+};
+
+static struct _irqfd_cleanup irqfd_cleanup = {
+	.lock = __MUTEX_INITIALIZER(irqfd_cleanup.lock),
+	.refs = 0,
+};
 
 static void
 irqfd_inject(struct work_struct *work)

@@ -110,7 +119,7 @@ irqfd_deactivate(struct _irqfd *irqfd)
 
 	list_del_init(&irqfd->list);
 
-	queue_work(irqfd_cleanup_wq, &irqfd->shutdown);
+	queue_work(irqfd_cleanup.wq, &irqfd->shutdown);
 }
 
 /*

@@ -161,6 +170,62 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 	add_wait_queue(wqh, &irqfd->wait);
 }
 
+/*
+ * create a host-wide workqueue for issuing deferred shutdown requests
+ * aggregated from all vm* instances. We need our own isolated single-thread
+ * queue to prevent deadlock against flushing the normal work-queue.
+ */
+static int
+irqfd_cleanup_init(struct kvm *kvm)
+{
+	int ret = 0;
+
+	mutex_lock(&irqfd_cleanup.lock);
+
+	/*
+	 * Check the current init state from within the lock so that we
+	 * sync all users to the thread creation.
+	 */
+	if (kvm->irqfds.init)
+		goto out;
+
+	if (!irqfd_cleanup.refs) {
+		struct workqueue_struct *wq;
+
+		wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
+		if (!wq) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		irqfd_cleanup.wq = wq;
+	}
+
+	irqfd_cleanup.refs++;
+	kvm->irqfds.init = true;
+
+out:
+	mutex_unlock(&irqfd_cleanup.lock);
+
+	return ret;
+}
+
+static void

virt/kvm/eventfd.c irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,

Quoted 5 lines

virt/kvm/eventfd.c +4 -0

+irqfd_cleanup_release(struct kvm *kvm)
+{
+	if (!kvm->irqfds.init)
+		return;
 

init is checked outside the lock here.
Why?

virt/kvm/eventfd.c irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,

Quoted 11 lines

virt/kvm/eventfd.c +9 -0

+
+	mutex_lock(&irqfd_cleanup.lock);
+
+	if (!(--irqfd_cleanup.refs))
+		destroy_workqueue(irqfd_cleanup.wq);
+
+	mutex_unlock(&irqfd_cleanup.lock);
+
+	kvm->irqfds.init = false;
 

... and cleaned outside the lock as well.

Quoted 47 lines

virt/kvm/eventfd.c +2 -0

+}
+
 static int

kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi) {

virt/kvm/eventfd.c +5 -18

@@ -170,6 +235,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	int ret;
 	unsigned int events;
 
+	ret = irqfd_cleanup_init(kvm);
+	if (ret < 0)
+		return ret;
+
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
 	if (!irqfd)
 		return -ENOMEM;

@@ -268,7 +337,7 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	 * so that we guarantee there will not be any more interrupts on this
 	 * gsi once this deassign function returns.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
+	flush_workqueue(irqfd_cleanup.wq);
 
 	return 0;
 }

@@ -302,28 +371,7 @@ kvm_irqfd_release(struct kvm *kvm)
 	 * Block until we know all outstanding shutdown jobs have completed
 	 * since we do not take a kvm* reference.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
-
-}
-
-/*
- * create a host-wide workqueue for issuing deferred shutdown requests
- * aggregated from all vm* instances. We need our own isolated single-thread
- * queue to prevent deadlock against flushing the normal work-queue.
- */
-static int __init irqfd_module_init(void)
-{
-	irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
-	if (!irqfd_cleanup_wq)
-		return -ENOMEM;
-
-	return 0;
-}

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 329

Quoted 10 lines

virt/kvm/eventfd.c +2 -6

+	flush_workqueue(irqfd_cleanup.wq);
+	irqfd_cleanup_release(kvm);
 
-static void __exit irqfd_module_exit(void)
-{
-	destroy_workqueue(irqfd_cleanup_wq);
 }
-
-module_init(irqfd_module_init);
-module_exit(irqfd_module_exit);

--
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 106 lines · Michael S. Tsirkin

Quoted 104 lines · Gregory Haskins

We currently create this wq on module_init, which may be wasteful if the host never creates a guest that uses irqfd. This patch changes the algorithm so that the workqueue is only created when at least one guest is using irqfd. The queue is cleaned up when the last guest using irqfd is shutdown.

To keep things simple, we only check whether the guest has tried to create an irqfd, not whether there are actually irqfds active.

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

---

 include/linux/kvm_host.h |    1 
 virt/kvm/eventfd.c       |  100 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 75 insertions(+), 26 deletions(-)

include/linux/kvm_host.h +1 -0

index 8e04a34..cd1a0f3 100644
@@ -149,6 +149,7 @@ struct kvm {
 	struct {
 		spinlock_t        lock;
 		struct list_head  items;
+		bool              init;
 	} irqfds;
 #endif
 	struct kvm_vm_stat stat;

virt/kvm/eventfd.c +52 -2

index 4092b8d..fcc3469 100644
@@ -49,7 +49,16 @@ struct _irqfd {
 	struct work_struct        shutdown;
 };
 
-static struct workqueue_struct *irqfd_cleanup_wq;
+struct _irqfd_cleanup {
+	struct mutex             lock;
+	int                      refs;
+	struct workqueue_struct *wq;
+};
+
+static struct _irqfd_cleanup irqfd_cleanup = {
+	.lock = __MUTEX_INITIALIZER(irqfd_cleanup.lock),
+	.refs = 0,
+};
 
 static void
 irqfd_inject(struct work_struct *work)

@@ -110,7 +119,7 @@ irqfd_deactivate(struct _irqfd *irqfd)
 
 	list_del_init(&irqfd->list);
 
-	queue_work(irqfd_cleanup_wq, &irqfd->shutdown);
+	queue_work(irqfd_cleanup.wq, &irqfd->shutdown);
 }
 
 /*

@@ -161,6 +170,62 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 	add_wait_queue(wqh, &irqfd->wait);
 }
 
+/*
+ * create a host-wide workqueue for issuing deferred shutdown requests
+ * aggregated from all vm* instances. We need our own isolated single-thread
+ * queue to prevent deadlock against flushing the normal work-queue.
+ */
+static int
+irqfd_cleanup_init(struct kvm *kvm)
+{
+	int ret = 0;
+
+	mutex_lock(&irqfd_cleanup.lock);
+
+	/*
+	 * Check the current init state from within the lock so that we
+	 * sync all users to the thread creation.
+	 */
+	if (kvm->irqfds.init)
+		goto out;
+
+	if (!irqfd_cleanup.refs) {
+		struct workqueue_struct *wq;
+
+		wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
+		if (!wq) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		irqfd_cleanup.wq = wq;
+	}
+
+	irqfd_cleanup.refs++;
+	kvm->irqfds.init = true;
+
+out:
+	mutex_unlock(&irqfd_cleanup.lock);
+
+	return ret;
+}
+
+static void

Quoted 9 lines

virt/kvm/eventfd.c irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,

Quoted 5 lines

virt/kvm/eventfd.c +4 -0

+irqfd_cleanup_release(struct kvm *kvm)
+{
+	if (!kvm->irqfds.init)
+		return;
 

init is checked outside the lock here.
Why?

Guest is shutting down via vmfd->f_ops->release() and is already guaranteed to be single-threaded, so proper locking is not really important. Probably should document that, though ;)

Quoted 4 lines

Quoted 4 lines

virt/kvm/eventfd.c +4 -0

+
+	mutex_lock(&irqfd_cleanup.lock);
+
+	if (!(--irqfd_cleanup.refs))

Quoted 10 lines

virt/kvm/eventfd.c irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,

Quoted 6 lines

virt/kvm/eventfd.c +5 -0

+		destroy_workqueue(irqfd_cleanup.wq);
+
+	mutex_unlock(&irqfd_cleanup.lock);
+
+	kvm->irqfds.init = false;
 

... and cleaned outside the lock as well.

Ditto

Quoted 63 lines

Quoted 47 lines

virt/kvm/eventfd.c +2 -0

+}
+
 static int

kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi) {

virt/kvm/eventfd.c +5 -18

@@ -170,6 +235,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	int ret;
 	unsigned int events;
 
+	ret = irqfd_cleanup_init(kvm);
+	if (ret < 0)
+		return ret;
+
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
 	if (!irqfd)
 		return -ENOMEM;

@@ -268,7 +337,7 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	 * so that we guarantee there will not be any more interrupts on this
 	 * gsi once this deassign function returns.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
+	flush_workqueue(irqfd_cleanup.wq);
 
 	return 0;
 }

@@ -302,28 +371,7 @@ kvm_irqfd_release(struct kvm *kvm)
 	 * Block until we know all outstanding shutdown jobs have completed
 	 * since we do not take a kvm* reference.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
-
-}
-
-/*
- * create a host-wide workqueue for issuing deferred shutdown requests
- * aggregated from all vm* instances. We need our own isolated single-thread
- * queue to prevent deadlock against flushing the normal work-queue.
- */
-static int __init irqfd_module_init(void)
-{
-	irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
-	if (!irqfd_cleanup_wq)
-		return -ENOMEM;
-
-	return 0;
-}

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 329

Quoted 10 lines

virt/kvm/eventfd.c +2 -6

+	flush_workqueue(irqfd_cleanup.wq);
+	irqfd_cleanup_release(kvm);
 
-static void __exit irqfd_module_exit(void)
-{
-	destroy_workqueue(irqfd_cleanup_wq);
 }
-
-module_init(irqfd_module_init);
-module_exit(irqfd_module_exit);

--
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 119 lines · Gregory Haskins

Quoted 117 lines · Michael S. Tsirkin

Quoted 100 lines · Gregory Haskins

We currently create this wq on module_init, which may be wasteful if the host never creates a guest that uses irqfd. This patch changes the algorithm so that the workqueue is only created when at least one guest is using irqfd. The queue is cleaned up when the last guest using irqfd is shutdown.

To keep things simple, we only check whether the guest has tried to create an irqfd, not whether there are actually irqfds active.

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

---

 include/linux/kvm_host.h |    1 
 virt/kvm/eventfd.c       |  100 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 75 insertions(+), 26 deletions(-)

include/linux/kvm_host.h +1 -0

index 8e04a34..cd1a0f3 100644
@@ -149,6 +149,7 @@ struct kvm {
 	struct {
 		spinlock_t        lock;
 		struct list_head  items;
+		bool              init;
 	} irqfds;
 #endif
 	struct kvm_vm_stat stat;

virt/kvm/eventfd.c +48 -2

index 4092b8d..fcc3469 100644
@@ -49,7 +49,16 @@ struct _irqfd {
 	struct work_struct        shutdown;
 };
 
-static struct workqueue_struct *irqfd_cleanup_wq;
+struct _irqfd_cleanup {
+	struct mutex             lock;
+	int                      refs;
+	struct workqueue_struct *wq;
+};
+
+static struct _irqfd_cleanup irqfd_cleanup = {
+	.lock = __MUTEX_INITIALIZER(irqfd_cleanup.lock),
+	.refs = 0,
+};
 
 static void
 irqfd_inject(struct work_struct *work)

@@ -110,7 +119,7 @@ irqfd_deactivate(struct _irqfd *irqfd)
 
 	list_del_init(&irqfd->list);
 
-	queue_work(irqfd_cleanup_wq, &irqfd->shutdown);
+	queue_work(irqfd_cleanup.wq, &irqfd->shutdown);
 }
 
 /*

@@ -161,6 +170,62 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 	add_wait_queue(wqh, &irqfd->wait);
 }
 
+/*
+ * create a host-wide workqueue for issuing deferred shutdown requests
+ * aggregated from all vm* instances. We need our own isolated single-thread
+ * queue to prevent deadlock against flushing the normal work-queue.
+ */
+static int
+irqfd_cleanup_init(struct kvm *kvm)
+{
+	int ret = 0;
+
+	mutex_lock(&irqfd_cleanup.lock);
+
+	/*
+	 * Check the current init state from within the lock so that we
+	 * sync all users to the thread creation.
+	 */
+	if (kvm->irqfds.init)
+		goto out;
+
+	if (!irqfd_cleanup.refs) {
+		struct workqueue_struct *wq;
+
+		wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
+		if (!wq) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		irqfd_cleanup.wq = wq;
+	}
+
+	irqfd_cleanup.refs++;
+	kvm->irqfds.init = true;
+
+out:
+	mutex_unlock(&irqfd_cleanup.lock);
+

virt/kvm/eventfd.c irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh, · line 219

Quoted 10 lines

virt/kvm/eventfd.c +8 -0

+	return ret;
+}
+
+static void
+irqfd_cleanup_release(struct kvm *kvm)
+{
+	if (!kvm->irqfds.init)
+		return;
 
 

init is checked outside the lock here.
Why?

Quoted 4 lines

Guest is shutting down via vmfd->f_ops->release() and is already guaranteed to be single-threaded, so proper locking is not really important. Probably should document that, though ;)

Is this an impirtant optimization? Simple locking is better..

Quoted 85 lines

Quoted 16 lines

virt/kvm/eventfd.c irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,

Quoted 12 lines

virt/kvm/eventfd.c +9 -0

+
+	mutex_lock(&irqfd_cleanup.lock);
+
+	if (!(--irqfd_cleanup.refs))
+		destroy_workqueue(irqfd_cleanup.wq);
+
+	mutex_unlock(&irqfd_cleanup.lock);
+
+	kvm->irqfds.init = false;
 

... and cleaned outside the lock as well.

Ditto

Quoted 65 lines

Quoted 48 lines

virt/kvm/eventfd.c +2 -0

+}
+

static int
kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi) {

virt/kvm/eventfd.c +5 -18

@@ -170,6 +235,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	int ret;
 	unsigned int events;
 
+	ret = irqfd_cleanup_init(kvm);
+	if (ret < 0)
+		return ret;
+
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
 	if (!irqfd)
 		return -ENOMEM;

@@ -268,7 +337,7 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	 * so that we guarantee there will not be any more interrupts on this
 	 * gsi once this deassign function returns.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
+	flush_workqueue(irqfd_cleanup.wq);
 
 	return 0;
 }

@@ -302,28 +371,7 @@ kvm_irqfd_release(struct kvm *kvm)
 	 * Block until we know all outstanding shutdown jobs have completed
 	 * since we do not take a kvm* reference.
 	 */
-	flush_workqueue(irqfd_cleanup_wq);
-
-}
-
-/*
- * create a host-wide workqueue for issuing deferred shutdown requests
- * aggregated from all vm* instances. We need our own isolated single-thread
- * queue to prevent deadlock against flushing the normal work-queue.
- */
-static int __init irqfd_module_init(void)
-{
-	irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
-	if (!irqfd_cleanup_wq)
-		return -ENOMEM;
-
-	return 0;
-}

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 329

Quoted 10 lines

virt/kvm/eventfd.c +2 -6

+	flush_workqueue(irqfd_cleanup.wq);
+	irqfd_cleanup_release(kvm);
 
-static void __exit irqfd_module_exit(void)
-{
-	destroy_workqueue(irqfd_cleanup_wq);
 }
-
-module_init(irqfd_module_init);
-module_exit(irqfd_module_exit);

--
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/

For the record, I would have rather just used my originally proposed slow-work thread instead of all this ;)

Quoted 3 lines · Gregory Haskins

For the record, I would have rather just used my originally proposed slow-work thread instead of all this ;)

Or just keep the idle thread around, it's not a big deal.

Quoted 11 lines · Michael S. Tsirkin

Quoted 4 lines · Gregory Haskins

For the record, I would have rather just used my originally proposed slow-work thread instead of all this ;)

Or just keep the idle thread around, it's not a big deal.

If everyone is fine with that, lets do that then. It's certainly the simplest solution to just drop this patch.

Avi?

Quoted 14 lines · Gregory Haskins

Quoted 11 lines · Michael S. Tsirkin

Quoted 4 lines · Gregory Haskins

For the record, I would have rather just used my originally proposed slow-work thread instead of all this ;)

Or just keep the idle thread around, it's not a big deal.

Quoted 6 lines

If everyone is fine with that, lets do that then. It's certainly the simplest solution to just drop this patch.

Avi?

Sure. If someone complains we can address it then.

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 6 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

Quoted 3 lines

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

Applied, thanks.

Quoted 13 lines · Avi Kivity

Quoted 11 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 2 lines

Applied, thanks.

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

Quoted 17 lines · Avi Kivity

Quoted 15 lines · Avi Kivity

Quoted 11 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Applied, thanks.

Quoted 3 lines

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

Ugh, apparently this logic has been changed between I acked v7 of the patches and between Avi applied v9. Will have to find the time to redo the review - or maybe just go back to v7? Is on-demand wq creation really that important?

Quoted 13 lines · Avi Kivity

Quoted 11 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 2 lines

Applied, thanks.

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

Quoted 4 lines

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

Avi, I verified and it seems that reverting just the last patch in series should be enough. Comments?

Quoted 15 lines · Avi Kivity

Quoted 13 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 2 lines

Applied, thanks.

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest.

This causes an oops if a guest with no irqfds is created and destroyed:

IP: [<ffffffff8105254a>] flush_workqueue+0x23/0x6e PGD 0

Oops: 0000 [4] SMP
CPU 1
Modules linked in: kvm_intel kvm nfs lockd nfs_acl sco bridge stp bnep

l2cap bluetooth autofs4 sunrpc ipv6 dm_multipath uinput i5000_edac e1000e edac_core iTCO_wdt
iTCO_vendor_support i2c_i801 i2c_core e100 mii floppy pcspkr shpchp
serio_raw ata_generic pata_acpi [last unloaded: kvm]
Pid: 2088, comm: qemu Tainted: G D
2.6.27.19-170.2.35.fc10.x86_64 #1 TYAN Transport GT20-B5372

RIP: 0010:[<ffffffff8105254a>]  [<ffffffff8105254a>]
flush_workqueue+0x23/0x6e
RSP: 0018:ffff8801077d1b08  EFLAGS: 00010292
RAX: ffffffff8156de08 RBX: ffff8801097e8a50 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 000000000000019f RDI: 0000000000000000
RBP: ffff8801077d1b28 R08: 0000000000000000 R09: ffffffff81140027
R10: ffff88012f402340 R11: ffff880125daf820 R12: ffffffff8156de10
R13: 0000000000000000 R14: ffff88012f449cd8 R15: ffff88012f002900
FS:  00007f9b67295950(0000) GS:ffff88012fc04980(0000)

knlGS:0000000000000000
CS: 0010 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000020 CR3: 0000000000201000 CR4: 00000000000026e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process qemu (pid: 2088, threadinfo ffff8801077d0000, task ffff88012984c530)

Stack:  ffff8801077d1b28 ffff8801097e8a50 ffff8801097e8000
ffff8801097e8a68
 ffff8801077d1b58 ffffffffa01c4eec 0000000800000000 ffff8801097e8000
 ffff88012f449cd8 ffff88012c80c600 ffff8801077d1b78 ffffffffa01af34b
Call Trace:
 [<ffffffffa01c4eec>] kvm_irqfd_release+0x7a/0xcc [kvm]
 [<ffffffffa01af34b>] kvm_vm_release+0x18/0x27 [kvm]
 [<ffffffff810c14c7>] __fput+0xca/0x16d
 [<ffffffff810c157f>] fput+0x15/0x17
 [<ffffffff810bea29>] filp_close+0x67/0x72
 [<ffffffff810433ec>] put_files_struct+0x74/0xc8
 [<ffffffff81043488>] exit_files+0x48/0x51
 [<ffffffff81044de9>] do_exit+0x26a/0x8a0
 [<ffffffffa01da330>] ? vmx_vcpu_put+0x9/0xb [kvm_intel]
 [<ffffffff810454a1>] do_group_exit+0x82/0xaf
 [<ffffffff8104eabb>] get_signal_to_deliver+0x2b0/0x2dc
 [<ffffffff81010379>] ? sysret_signal+0x42/0x71
 [<ffffffff8100f45f>] do_notify_resume+0x90/0x93f
 [<ffffffff81060cca>] ? do_futex+0x90/0x973
 [<ffffffffa01ad956>] ? kvm_vcpu_ioctl+0x470/0x485 [kvm]
 [<ffffffff81333801>] ? trace_hardirqs_on_thunk+0x3a/0x3c
 [<ffffffff810616a2>] ? sys_futex+0xf5/0x113
 [<ffffffff81010379>] ? sysret_signal+0x42/0x71
 [<ffffffff81010737>] ptregscall_common+0x67/0xb0

irqfd_cleanup.wq has never been initialized, but is destroyed.

Quoted 15 lines · Avi Kivity

Quoted 13 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 2 lines

Applied, thanks.

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

Quoted 16 lines · Avi Kivity

Quoted 15 lines · Avi Kivity

Quoted 13 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 6 lines

Quoted 2 lines

Applied, thanks.

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

I was able to reproduce this issue. The problem turned out to be that I inadvertently always did a flush_workqueue(), even if the work-queue was never initialized.

The following interdiff applied to the reverted patch has been confirmed to fix the issue:

-------------------

virt/kvm/eventfd.c +0 -0

index fcc3469..52b0e04 100644

virt/kvm/eventfd.c +0 -1

        spin_lock_irq(&kvm->irqfds.lock);
 
        list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds.items, list)
 
---------------------

You can pick up this fix folded into the original v9:5/5 patch here:

git pull
git://git.kernel.org/pub/scm/linux/kernel/git/ghaskins/linux-2.6-hacks.git
for-avi

Sorry for the sloppy patch in v9. :( Will strive to do better next time.

Regards,
-Greg

Quoted 31 lines · Gregory Haskins

Quoted 18 lines · Avi Kivity

Quoted 16 lines · Avi Kivity

Quoted 14 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 7 lines

Quoted 3 lines

Applied, thanks.

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

I was able to reproduce this issue. The problem turned out to be that I inadvertently always did a flush_workqueue(), even if the work-queue was never initialized.

Quoted 3 lines

The following interdiff applied to the reverted patch has been confirmed to fix the issue:

Could you document the init boolean and its locking rules? The best place to put it would be where the field is declared btw.

Is it true that init === list_empty(&kvm->irqfds.items)? If yes maybe we don't need this field at all.

Quoted 11 lines

-------------------

virt/kvm/eventfd.c +0 -0

index fcc3469..52b0e04 100644
@@ -318,6 +318,9 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
        struct _irqfd *irqfd, *tmp;
        struct eventfd_ctx *eventfd;
 

virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 326

Quoted 7 lines

virt/kvm/eventfd.c +3 -0

+       if (!kvm->irqfds.init)
+               return -ENOENT;
+
        eventfd = eventfd_ctx_fdget(fd);
        if (IS_ERR(eventfd))
                return PTR_ERR(eventfd);

wouldn't it be cleaner to error out in the for each loop if we don't find an entry to deactivate? Might be helpful for apps to get an error if they didn't deassign anything.

Quoted 5 lines

virt/kvm/eventfd.c +0 -0

@@ -360,6 +363,9 @@ kvm_irqfd_release(struct kvm *kvm)
 {
        struct _irqfd *irqfd, *tmp;
 

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 370

Quoted 4 lines

virt/kvm/eventfd.c +3 -0

+       if (!kvm->irqfds.init)
+               return;
+
 

So here, I recall some old comment that flush below was needed even if list is empty. Is this no longer true?

If not it might be cleaner to only flush if list is not empty.

Quoted 33 lines · Michael S. Tsirkin

Quoted 31 lines · Gregory Haskins

Quoted 18 lines · Avi Kivity

Quoted 16 lines · Avi Kivity

Quoted 14 lines · Gregory Haskins

(Applies to kvm.git/master:1f9050fd)

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.

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

Quoted 7 lines

Quoted 3 lines

Applied, thanks.

... and unapplied. There's a refcounting mismatch in irqfd_cleanup: a reference is taken for each irqfd, but dropped for each guest. This causes an oops if a guest with no irqfds is created and destroyed:

I was able to reproduce this issue. The problem turned out to be that I inadvertently always did a flush_workqueue(), even if the work-queue was never initialized.

Quoted 7 lines

Quoted 3 lines

The following interdiff applied to the reverted patch has been confirmed to fix the issue:

Could you document the init boolean and its locking rules? The best place to put it would be where the field is declared btw.

Will do

Quoted 4 lines

Is it true that init === list_empty(&kvm->irqfds.items)? If yes maybe we don't need this field at all.

No,

because its more difficult to maintain the work-queue when referenced against active irqfds (*). So instead, its maintained against guests that use irqfd, whether they have an active irqfd or not. Otherwise you have to contend with the eventfd-side release, which is a little tricky.

(*) I'm sure its not rocket science to get this working, but it was getting more complex than I thought it was worth, so I simplified the model to be per-vm. Note that this design decision/limitation is declared in the patch header.

Quoted 11 lines

Quoted 11 lines

-------------------

virt/kvm/eventfd.c +0 -0

index fcc3469..52b0e04 100644
@@ -318,6 +318,9 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
        struct _irqfd *irqfd, *tmp;
        struct eventfd_ctx *eventfd;
 

Quoted 12 lines

virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 326

Quoted 7 lines

virt/kvm/eventfd.c +3 -0

+       if (!kvm->irqfds.init)
+               return -ENOENT;
+
        eventfd = eventfd_ctx_fdget(fd);
        if (IS_ERR(eventfd))
                return PTR_ERR(eventfd);

wouldn't it be cleaner to error out in the for each loop if we don't find an entry to deactivate? Might be helpful for apps to get an error if they didn't deassign anything.

Again, irqfds.init is somewhat orthogonal to whether the list is populated or not. This check is for sanity (how can you deassign if you didnt assign, etc). Normally this would be a simple BUG_ON() sanity check, but I don't want a malicious/broken userspace to gain an easy attack vector ;)

Quoted 5 lines

Quoted 5 lines

virt/kvm/eventfd.c +0 -0

@@ -360,6 +363,9 @@ kvm_irqfd_release(struct kvm *kvm)
 {
        struct _irqfd *irqfd, *tmp;
 

Quoted 8 lines

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 370

Quoted 4 lines

virt/kvm/eventfd.c +3 -0

+       if (!kvm->irqfds.init)
+               return;
+
 

So here, I recall some old comment that flush below was needed even if list is empty. Is this no longer true?

If you are using irqfd, its true. If irqfds.init == false, you are not using irqfd and thus the flush cannot be needed.

Quoted 3 lines

If not it might be cleaner to only flush if list is not empty.

You have to flush if irqfds.init == true even if the list is empty because you need to be sure that eventfd-side releases complete. They may have already removed themselves from the list, but the work-item is still in flight.

Regards,
-Greg

Quoted 9 lines · Gregory Haskins

Quoted 7 lines · Michael S. Tsirkin

Quoted 2 lines

wouldn't it be cleaner to error out in the for each loop if we don't find an entry to deactivate? Might be helpful for apps to get an error if they didn't deassign anything.

Quoted 6 lines

Again, irqfds.init is somewhat orthogonal to whether the list is populated or not. This check is for sanity (how can you deassign if you didnt assign, etc). Normally this would be a simple BUG_ON() sanity check, but I don't want a malicious/broken userspace to gain an easy attack vector ;)

what I'm saying is that deassign should return an error if it's passed and entry that is not on the list.