irqfd fixes/enhancements

PATCH Superseded Gregory Haskins 3 participants 9 messages Jun 18, 2009 – Jun 21, 2009

Subsystems (2)

VFS KVM

lore

Share & tools

lore

Gregory Haskins

author

2009-06-18 17:44 UTC #0 lore raw

cover irqfd fixes/enhancements

(Applies to kvm.git/master:c27b64a0)

The following series represents my queue of fixes for issues in irqfd, which include enhancements to eventfd. This is a respin of the patches originally proposed yesterday, called "[KVM-RFC PATCH 0/2] eventfd enhancements for irqfd/iosignalfd", which you can find a link to the thread here:

http://www.gossamer-threads.com/lists/linux/kernel/1091852

Aside from introducing a few minor fixes (patches 1/4, 2/4), the general solution for module reference counting has been extracted from the (controversial) SRCU enhancement. We can address the SRCU topic at a later time. For now, fixing the race condition is a higher priority item IMO. Hopefully, this more focused series is more palatable for the reviewers/maintainers involved.

---

Gregory Haskins (4):
eventfd: add module reference counting support for registered notifiers
eventfd: add generalized notifier interface
KVM: fix irqfd error checking
KVM: update inaccurate irqfd comment

 fs/eventfd.c            |   77 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/eventfd.h |   37 +++++++++++++++++++++
 virt/kvm/eventfd.c      |   82 ++++++++++++++++++-----------------------------
 3 files changed, 146 insertions(+), 50 deletions(-)

Gregory Haskins

author

2009-06-18 17:44 UTC #1 lore raw

PATCH 1/4 KVM: update inaccurate irqfd comment

We no longer hold the fget() for the irqfd lifetime, so clean up the comments related to that.

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

---

 virt/kvm/eventfd.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

virt/kvm/eventfd.c +0 -3

index 2c8028c..a0e329f 100644
@@ -156,9 +156,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 
-	/*
-	 * Embed the file* lifetime in the irqfd.
-	 */
 	file = eventfd_fget(fd);
 	if (IS_ERR(file)) {
 		ret = PTR_ERR(file);

Gregory Haskins

author

2009-06-18 17:44 UTC #2 lore raw

PATCH 2/4 KVM: fix irqfd error checking

In reply to Gregory Haskins

Michael Tsirkin pointed out that f_ops->poll() does not return a standard "int" error, yet we are treating it as such. Lets fix this.

Reported-by Michael S. Tsirkin <[email protected]>

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

---

 virt/kvm/eventfd.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

virt/kvm/eventfd.c +8 -6

index a0e329f..a9e7de7 100644
@@ -144,6 +144,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
 	int ret;
+	unsigned int events;
 
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
 	if (!irqfd)

@@ -169,9 +170,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
 	init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
 
-	ret = file->f_op->poll(file, &irqfd->pt);
-	if (ret < 0)
-		goto fail;
+	events = file->f_op->poll(file, &irqfd->pt);
 
 	kvm_get_kvm(kvm);

@@ -180,6 +179,12 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	mutex_unlock(&kvm->lock);
 
 	/*
+	 * Check if there was an event already queued
+	 */
+	if (events & POLLIN)
+		schedule_work(&irqfd->inject);
+
+	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise
 	 * we might race against the POLLHUP
 	 */

@@ -188,9 +193,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	return 0;
 
 fail:
-	if (irqfd->wqh)
-		remove_wait_queue(irqfd->wqh, &irqfd->wait);
-
 	if (file && !IS_ERR(file))
 		fput(file);

Gregory Haskins

author

2009-06-18 17:44 UTC #3 lore raw

PATCH 3/4 eventfd: add generalized notifier interface

In reply to Gregory Haskins

We currently open-code notification registration via the f_ops->poll() method of the eventfd. Lets abstract this into a notification API extension of eventfd, while still using the wait-queue as the underlying mechanism. This will allow us to manage the notification mechanism internal to eventfd without requiring the clients to change. This also gives us the opportunity to implement race-free release() callbacks, which we do later in the series.

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

CC Davide Libenzi <[email protected]>

---

 fs/eventfd.c            |   69 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/eventfd.h |   34 +++++++++++++++++++++
 virt/kvm/eventfd.c      |   75 ++++++++++++++++++-----------------------------
 3 files changed, 132 insertions(+), 46 deletions(-)

fs/eventfd.c +69 -0

index 72f5f8d..f9d7e1d 100644
@@ -245,3 +245,72 @@ SYSCALL_DEFINE1(eventfd, unsigned int, count)
 	return sys_eventfd2(count, 0);
 }
 
+static int eventfd_notifier_wakeup(wait_queue_t *wait, unsigned mode,
+				   int sync, void *key)
+{
+	struct eventfd_notifier *en;
+	unsigned long flags = (unsigned long)key;
+
+	en = container_of(wait, struct eventfd_notifier, wait);
+
+	if (flags & POLLIN)
+		/*
+		 * The POLLIN wake_up is called with interrupts disabled.
+		 */
+		en->ops->signal(en);
+
+	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()
+		 */
+		remove_wait_queue(en->wqh, &en->wait);
+		en->ops->release(en);
+	}
+
+	return 0;
+}
+
+static void eventfd_notifier_ptable_enqueue(struct file *file,
+					    wait_queue_head_t *wqh,
+					    poll_table *pt)
+{
+	struct eventfd_notifier *en;
+
+	en = container_of(pt, struct eventfd_notifier, pt);
+
+	en->wqh = wqh;
+	add_wait_queue(wqh, &en->wait);
+}
+
+int eventfd_notifier_register(struct file *file, struct eventfd_notifier *en)
+{
+	unsigned int events;
+
+	if (file->f_op != &eventfd_fops)
+		return -EINVAL;
+
+	/*
+	 * Install our own custom wake-up handling so we are notified via
+	 * a callback whenever someone signals the underlying eventfd
+	 */
+	init_waitqueue_func_entry(&en->wait, eventfd_notifier_wakeup);
+	init_poll_funcptr(&en->pt, eventfd_notifier_ptable_enqueue);
+
+	events = file->f_op->poll(file, &en->pt);
+
+	return (events & POLLIN) ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_notifier_register);
+
+int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en)
+{
+	if (!en->wqh)
+		return -EINVAL;
+
+	remove_wait_queue(en->wqh, &en->wait);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_notifier_unregister);

include/linux/eventfd.h +34 -0

index f45a8ae..802b59d 100644
@@ -8,6 +8,32 @@
 #ifndef _LINUX_EVENTFD_H
 #define _LINUX_EVENTFD_H
 
+#include <linux/wait.h>
+#include <linux/poll.h>
+#include <linux/file.h>
+#include <linux/list.h>
+
+struct eventfd_notifier;
+
+struct eventfd_notifier_ops {
+	void (*signal)(struct eventfd_notifier *en);
+	void (*release)(struct eventfd_notifier *en);
+};
+
+struct eventfd_notifier {
+	poll_table                         pt;
+	wait_queue_head_t                 *wqh;
+	wait_queue_t                       wait;
+	const struct eventfd_notifier_ops *ops;
+};
+
+static inline void eventfd_notifier_init(struct eventfd_notifier *en,
+					 const struct eventfd_notifier_ops *ops)
+{
+	memset(en, 0, sizeof(*en));
+	en->ops = ops;
+}
+
 #ifdef CONFIG_EVENTFD
 
 /* For O_CLOEXEC and O_NONBLOCK */

@@ -29,12 +55,20 @@
 
 struct file *eventfd_fget(int fd);
 int eventfd_signal(struct file *file, int n);
+int eventfd_notifier_register(struct file *file, struct eventfd_notifier *en);
+int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en);
 
 #else /* CONFIG_EVENTFD */
 
 #define eventfd_fget(fd) ERR_PTR(-ENOSYS)
 static inline int eventfd_signal(struct file *file, int n)
 { return 0; }
+static inline int eventfd_notifier_register(struct file *file,
+					    struct eventfd_notifier *en)
+{ return -ENOENT; }
+static inline int eventfd_notifier_unregister(struct file *file,
+					      struct eventfd_notifier *en)
+{ return -ENOENT; }
 
 #endif /* CONFIG_EVENTFD */

virt/kvm/eventfd.c +29 ~~-46~~

118 lines not on this page · raw message

Davide Libenzi

2009-06-18 17:51 UTC #4 lore raw

Re: [KVM PATCH 3/4] eventfd: add generalized notifier interface

Quoted 7 lines · Gregory Haskins

We currently open-code notification registration via the f_ops->poll() method of the eventfd. Lets abstract this into a notification API extension of eventfd, while still using the wait-queue as the underlying mechanism. This will allow us to manage the notification mechanism internal to eventfd without requiring the clients to change. This also gives us the opportunity to implement race-free release() callbacks, which we do later in the series.

Another attempt to push KVM stuff into eventfd. NACK.

- Davide

Gregory Haskins

author

2009-06-18 18:46 UTC #5 lore raw

Re: [KVM PATCH 3/4] eventfd: add generalized notifier interface

Quoted 12 lines · Davide Libenzi

Quoted 8 lines · Gregory Haskins
We currently open-code notification registration via the f_ops->poll() method of the eventfd. Lets abstract this into a notification API extension of eventfd, while still using the wait-queue as the underlying mechanism. This will allow us to manage the notification mechanism internal to eventfd without requiring the clients to change. This also gives us the opportunity to implement race-free release() callbacks, which we do later in the series.

Quoted 1 line

Another attempt to push KVM stuff into eventfd.

?? I do not believe there is a single iota of KVM specific code in that patch. Can you elaborate?

-Greg

Gregory Haskins

author

2009-06-18 18:48 UTC #6 lore raw

Re: [KVM PATCH 3/4] eventfd: add generalized notifier interface

In reply to Davide Libenzi

Quoted 12 lines · Davide Libenzi

Quoted 8 lines · Gregory Haskins
We currently open-code notification registration via the f_ops->poll() method of the eventfd. Lets abstract this into a notification API extension of eventfd, while still using the wait-queue as the underlying mechanism. This will allow us to manage the notification mechanism internal to eventfd without requiring the clients to change. This also gives us the opportunity to implement race-free release() callbacks, which we do later in the series.

Quoted 1 line

Another attempt to push KVM stuff into eventfd.

Also, could you please detail how you expect someone to use the POLLHUP release in a race free way without patches 3/4 + 4/4? That is not a KVM specific problem either, afaict.

-Greg

Gregory Haskins

author

2009-06-18 17:44 UTC #7 lore raw

PATCH 4/4 eventfd: add module reference counting support for registered notifiers

In reply to Gregory Haskins

Michael Tsirkin found a race condition in the irqfd code where we may allow the underlying eventfd object to race with the rmmod of kvm.ko.

Since we now use eventfd_notifier for irqfd, lets add a struct module *owner field to properly maintain references to our registered signal handlers.

Found-by Michael S. Tsirkin <[email protected]>

CC Davide Libenzi <[email protected]>

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

---

 fs/eventfd.c            |    8 ++++++++
 include/linux/eventfd.h |    3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)

fs/eventfd.c +8 -0

index f9d7e1d..4a073ee 100644
@@ -260,6 +260,8 @@ static int eventfd_notifier_wakeup(wait_queue_t *wait, unsigned mode,
 		en->ops->signal(en);
 
 	if (flags & POLLHUP) {
+		struct module *owner = en->owner;
+
 		/*
 		 * The POLLHUP is called unlocked, so it theoretically should
 		 * be safe to remove ourselves from the wqh using the locked

@@ -267,6 +269,8 @@ static int eventfd_notifier_wakeup(wait_queue_t *wait, unsigned mode,
 		 */
 		remove_wait_queue(en->wqh, &en->wait);
 		en->ops->release(en);
+
+		module_put(owner);
 	}
 
 	return 0;

@@ -291,6 +295,9 @@ int eventfd_notifier_register(struct file *file, struct eventfd_notifier *en)
 	if (file->f_op != &eventfd_fops)
 		return -EINVAL;
 
+	if (!try_module_get(en->owner))
+		return -EINVAL;
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -310,6 +317,7 @@ int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en)
 		return -EINVAL;
 
 	remove_wait_queue(en->wqh, &en->wait);
+	module_put(en->owner);
 
 	return 0;
 }

include/linux/eventfd.h +3 -0

index 802b59d..7e015f0 100644
@@ -12,6 +12,7 @@
 #include <linux/poll.h>
 #include <linux/file.h>
 #include <linux/list.h>
+#include <linux/module.h>
 
 struct eventfd_notifier;

@@ -21,6 +22,7 @@ struct eventfd_notifier_ops {
 };
 
 struct eventfd_notifier {
+	struct module                     *owner;
 	poll_table                         pt;
 	wait_queue_head_t                 *wqh;
 	wait_queue_t                       wait;

@@ -31,6 +33,7 @@ static inline void eventfd_notifier_init(struct eventfd_notifier *en,
 					 const struct eventfd_notifier_ops *ops)
 {
 	memset(en, 0, sizeof(*en));
+	en->owner = THIS_MODULE;
 	en->ops = ops;
 }

Avi Kivity

2009-06-21 12:56 UTC #8 lore raw

In reply to Gregory Haskins

Quoted 8 lines · Gregory Haskins

(Applies to kvm.git/master:c27b64a0)
The following series represents my queue of fixes for issues in irqfd, which include enhancements to eventfd. This is a respin of the patches originally proposed yesterday, called "[KVM-RFC PATCH 0/2] eventfd enhancements for irqfd/iosignalfd", which you can find a link to the thread here:

Quoted 9 lines

http://www.gossamer-threads.com/lists/linux/kernel/1091852
Aside from introducing a few minor fixes (patches 1/4, 2/4), the general solution for module reference counting has been extracted from the (controversial) SRCU enhancement. We can address the SRCU topic at a later time. For now, fixing the race condition is a higher priority item IMO. Hopefully, this more focused series is more palatable for the reviewers/maintainers involved.

Applied 1-2, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function