LinuxLists.cc - [KVM-RFC PATCH 0/2] eventfd enhancements for irqfd/iosignalfd

[permalink] [raw]

Subject: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface

irqfd and its underlying implementation, eventfd, currently utilize
the embedded wait-queue in eventfd for signal notification. The nice thing
about this design decision is that it re-uses the existing
eventfd/wait-queue code and it generally works well....with several
limitations.

One of the limitations is that notification callbacks are always called
inside a spin_lock_irqsave critical section. Another limitation is
that it is very difficult to build a system that can recieve release
notification without being racy.

Therefore, we introduce a new registration interface that is SRCU based
instead of wait-queue based, and implement the internal wait-queue
infrastructure in terms of this new interface. We then convert irqfd
to use this new interface instead of the existing wait-queue code.

The end result is that we now have the opportunity to run the interrupt
injection code serially to the callback (when the signal is raised from
process-context, at least) instead of always deferring the injection to a
work-queue.

Signed-off-by: Gregory Haskins <[email protected]>
CC: Paul E. McKenney <[email protected]>
CC: Davide Libenzi <[email protected]>
---

fs/eventfd.c | 115 +++++++++++++++++++++++++++++++++++++++++++----
include/linux/eventfd.h | 30 ++++++++++++
virt/kvm/eventfd.c | 114 +++++++++++++++++++++--------------------------
3 files changed, 188 insertions(+), 71 deletions(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index 72f5f8d..505d5de 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -30,8 +30,47 @@ struct eventfd_ctx {
*/
__u64 count;
unsigned int flags;
+ struct srcu_struct srcu;
+ struct list_head nh;
+ struct eventfd_notifier notifier;
};

+static void _eventfd_wqh_notify(struct eventfd_notifier *en)
+{
+ struct eventfd_ctx *ctx = container_of(en,
+ struct eventfd_ctx,
+ notifier);
+
+ if (waitqueue_active(&ctx->wqh))
+ wake_up_poll(&ctx->wqh, POLLIN);
+}
+
+static void _eventfd_notify(struct eventfd_ctx *ctx)
+{
+ struct eventfd_notifier *en;
+ int idx;
+
+ idx = srcu_read_lock(&ctx->srcu);
+
+ /*
+ * The goal here is to allow the notification to be preemptible
+ * as often as possible. We cannot achieve this with the basic
+ * wqh mechanism because it requires the wqh->lock. Therefore
+ * we have an internal srcu list mechanism of which the wqh is
+ * a client.
+ *
+ * Not all paths will invoke this function in process context.
+ * Callers should check for suitable state before assuming they
+ * can sleep (such as with preemptible()). Paul McKenney assures
+ * me that srcu_read_lock is compatible with in-atomic, as long as
+ * the code within the critical section is also compatible.
+ */
+ list_for_each_entry_rcu(en, &ctx->nh, list)
+ en->ops->signal(en);
+
+ srcu_read_unlock(&ctx->srcu, idx);
+}
+
/*
* Adds "n" to the eventfd counter "count". Returns "n" in case of
* success, or a value lower then "n" in case of coutner overflow.
@@ -51,10 +90,10 @@ int eventfd_signal(struct file *file, int n)
if (ULLONG_MAX - ctx->count < n)
n = (int) (ULLONG_MAX - ctx->count);
ctx->count += n;
- if (waitqueue_active(&ctx->wqh))
- wake_up_locked_poll(&ctx->wqh, POLLIN);
spin_unlock_irqrestore(&ctx->wqh.lock, flags);

+ _eventfd_notify(ctx);
+
return n;
}
EXPORT_SYMBOL_GPL(eventfd_signal);
@@ -62,13 +101,21 @@ EXPORT_SYMBOL_GPL(eventfd_signal);
static int eventfd_release(struct inode *inode, struct file *file)
{
struct eventfd_ctx *ctx = file->private_data;
+ struct eventfd_notifier *en, *tmp;

/*
* No need to hold the lock here, since we are on the file cleanup
- * path and the ones still attached to the wait queue will be
- * serialized by wake_up_locked_poll().
+ * path
*/
- wake_up_locked_poll(&ctx->wqh, POLLHUP);
+ list_for_each_entry_safe(en, tmp, &ctx->nh, list) {
+ list_del(&en->list);
+ if (en->ops->release)
+ en->ops->release(en);
+ }
+
+ synchronize_srcu(&ctx->srcu);
+ cleanup_srcu_struct(&ctx->srcu);
+
kfree(ctx);
return 0;
}
@@ -176,13 +223,13 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c
__remove_wait_queue(&ctx->wqh, &wait);
__set_current_state(TASK_RUNNING);
}
- if (likely(res > 0)) {
+ if (likely(res > 0))
ctx->count += ucnt;
- if (waitqueue_active(&ctx->wqh))
- wake_up_locked_poll(&ctx->wqh, POLLIN);
- }
spin_unlock_irq(&ctx->wqh.lock);

+ if (likely(res > 0))
+ _eventfd_notify(ctx);
+
return res;
}

@@ -209,6 +256,50 @@ struct file *eventfd_fget(int fd)
}
EXPORT_SYMBOL_GPL(eventfd_fget);

+static int _eventfd_notifier_register(struct eventfd_ctx *ctx,
+ struct eventfd_notifier *en)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&ctx->wqh.lock, flags);
+ list_add_tail_rcu(&en->list, &ctx->nh);
+ spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+
+ return 0;
+}
+
+int eventfd_notifier_register(struct file *file, struct eventfd_notifier *en)
+{
+ struct eventfd_ctx *ctx = file->private_data;
+
+ if (file->f_op != &eventfd_fops)
+ return -EINVAL;
+
+ return _eventfd_notifier_register(ctx, en);
+}
+EXPORT_SYMBOL_GPL(eventfd_notifier_register);
+
+int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en)
+{
+ struct eventfd_ctx *ctx = file->private_data;
+
+ if (file->f_op != &eventfd_fops)
+ return -EINVAL;
+
+ spin_lock_irq(&ctx->wqh.lock);
+ list_del_rcu(&en->list);
+ spin_unlock_irq(&ctx->wqh.lock);
+
+ synchronize_srcu(&ctx->srcu);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_notifier_unregister);
+
+static const struct eventfd_notifier_ops eventfd_wqh_ops = {
+ .signal = _eventfd_wqh_notify,
+};
+
SYSCALL_DEFINE2(eventfd2, unsigned int, count, int, flags)
{
int fd;
@@ -229,6 +320,12 @@ SYSCALL_DEFINE2(eventfd2, unsigned int, count, int, flags)
ctx->count = count;
ctx->flags = flags;

+ init_srcu_struct(&ctx->srcu);
+ INIT_LIST_HEAD(&ctx->nh);
+
+ eventfd_notifier_init(&ctx->notifier, &eventfd_wqh_ops);
+ _eventfd_notifier_register(ctx, &ctx->notifier);
+
/*
* When we call this, the initialization must be complete, since
* anon_inode_getfd() will install the fd.
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index f45a8ae..0218cf6 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -8,6 +8,28 @@
#ifndef _LINUX_EVENTFD_H
#define _LINUX_EVENTFD_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 {
+ struct list_head list;
+ 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));
+ INIT_LIST_HEAD(&en->list);
+ en->ops = ops;
+}
+
#ifdef CONFIG_EVENTFD

/* For O_CLOEXEC and O_NONBLOCK */
@@ -29,12 +51,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 */

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 2c8028c..efc3d77 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -43,33 +43,11 @@ struct _irqfd {
struct kvm *kvm;
int gsi;
struct list_head list;
- poll_table pt;
- wait_queue_head_t *wqh;
- wait_queue_t wait;
struct work_struct inject;
+ struct eventfd_notifier notifier;
};

static void
-irqfd_inject(struct work_struct *work)
-{
- struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
- struct kvm *kvm;
- int idx;
-
- idx = srcu_read_lock(&irqfd->srcu);
-
- kvm = rcu_dereference(irqfd->kvm);
- if (kvm) {
- mutex_lock(&kvm->irq_lock);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
- mutex_unlock(&kvm->irq_lock);
- }
-
- srcu_read_unlock(&irqfd->srcu, idx);
-}
-
-static void
irqfd_disconnect(struct _irqfd *irqfd)
{
struct kvm *kvm;
@@ -97,47 +75,67 @@ irqfd_disconnect(struct _irqfd *irqfd)
kvm_put_kvm(kvm);
}

-static int
-irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
+static void
+_irqfd_inject(struct _irqfd *irqfd)
{
- struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
- unsigned long flags = (unsigned long)key;
-
- if (flags & POLLIN)
- /*
- * The POLLIN wake_up is called with interrupts disabled.
- * Therefore we need to defer the IRQ injection until later
- * since we need to acquire the kvm->lock to do so.
- */
- schedule_work(&irqfd->inject);
+ struct kvm *kvm;
+ int idx;

- 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(irqfd->wqh, &irqfd->wait);
- flush_work(&irqfd->inject);
- irqfd_disconnect(irqfd);
+ idx = srcu_read_lock(&irqfd->srcu);

- cleanup_srcu_struct(&irqfd->srcu);
- kfree(irqfd);
+ kvm = rcu_dereference(irqfd->kvm);
+ if (kvm) {
+ mutex_lock(&kvm->irq_lock);
+ kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
+ kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+ mutex_unlock(&kvm->irq_lock);
}

- return 0;
+ srcu_read_unlock(&irqfd->srcu, idx);
+}
+
+static void
+irqfd_inject(struct work_struct *work)
+{
+ struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
+
+ _irqfd_inject(irqfd);
+}
+
+static void
+irqfd_eventfd_signal(struct eventfd_notifier *notifier)
+{
+ struct _irqfd *irqfd = container_of(notifier, struct _irqfd, notifier);
+
+ /*
+ * Our signal may or may not be called from atomic context. We can
+ * detect if this can safely sleep by checking preemptible() on
+ * CONFIG_PREEMPT kernels. CONFIG_PREEMPT=n, or in_atomic() will fail
+ * this check and therefore defer the injection via a work-queue
+ */
+ if (preemptible())
+ _irqfd_inject(irqfd);
+ else
+ schedule_work(&irqfd->inject);
}

static void
-irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
- poll_table *pt)
+irqfd_eventfd_release(struct eventfd_notifier *notifier)
{
- struct _irqfd *irqfd = container_of(pt, struct _irqfd, pt);
+ struct _irqfd *irqfd = container_of(notifier, struct _irqfd, notifier);

- irqfd->wqh = wqh;
- add_wait_queue(wqh, &irqfd->wait);
+ flush_work(&irqfd->inject);
+ irqfd_disconnect(irqfd);
+
+ cleanup_srcu_struct(&irqfd->srcu);
+ kfree(irqfd);
}

+static const struct eventfd_notifier_ops irqfd_notifier_ops = {
+ .signal = irqfd_eventfd_signal,
+ .release = irqfd_eventfd_release,
+};
+
int
kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
{
@@ -165,14 +163,9 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
goto fail;
}

- /*
- * Install our own custom wake-up handling so we are notified via
- * a callback whenever someone signals the underlying eventfd
- */
- init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
- init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
+ eventfd_notifier_init(&irqfd->notifier, &irqfd_notifier_ops);

- ret = file->f_op->poll(file, &irqfd->pt);
+ ret = eventfd_notifier_register(file, &irqfd->notifier);
if (ret < 0)
goto fail;

@@ -191,9 +184,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);

2009-06-16 02:30:52

[permalink] [raw]

Subject: [KVM-RFC PATCH 2/2] eventfd: add module reference counting support for registered notifiers

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(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index 505d5de..babedb3 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -108,9 +108,12 @@ static int eventfd_release(struct inode *inode, struct file *file)
* path
*/
list_for_each_entry_safe(en, tmp, &ctx->nh, list) {
+ struct module *owner = en->owner;
+
list_del(&en->list);
if (en->ops->release)
en->ops->release(en);
+ module_put(owner);
}

synchronize_srcu(&ctx->srcu);
@@ -261,6 +264,9 @@ static int _eventfd_notifier_register(struct eventfd_ctx *ctx,
{
unsigned long flags;

+ if (!try_module_get(en->owner))
+ return -EINVAL;
+
spin_lock_irqsave(&ctx->wqh.lock, flags);
list_add_tail_rcu(&en->list, &ctx->nh);
spin_unlock_irqrestore(&ctx->wqh.lock, flags);
@@ -292,6 +298,8 @@ int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en)

synchronize_srcu(&ctx->srcu);

+ module_put(en->owner);
+
return 0;
}
EXPORT_SYMBOL_GPL(eventfd_notifier_unregister);
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index 0218cf6..f534bcd 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -9,6 +9,7 @@
#define _LINUX_EVENTFD_H

#include <linux/list.h>
+#include <linux/module.h>

struct eventfd_notifier;

@@ -18,6 +19,7 @@ struct eventfd_notifier_ops {
};

struct eventfd_notifier {
+ struct module *owner;
struct list_head list;
const struct eventfd_notifier_ops *ops;
};
@@ -26,6 +28,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;
INIT_LIST_HEAD(&en->list);
en->ops = ops;
}

2009-06-16 14:03:17

[permalink] [raw]

Subject: Re: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface

On Wed, Jun 17, 2009 at 04:21:19PM -0700, Davide Libenzi wrote:
> The interface is just ugly IMO. You have eventfd_signal() that can sleep,
> or not, depending on the registered ->signal() function implementations.
> This is pretty bad, a lot worse than the theoretical us spent in the
> schedule_work() processing.

I agree. How about the idea of introducing eventfd_signal_from_task
which can sleep? Does this sound same?

--
MST

2009-06-18 09:03:42

by Avi Kivity

[permalink] [raw]

Subject: Re: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface

On 06/16/2009 05:40 PM, Gregory Haskins wrote:
>
> Something else to consider: For iosignalfd, we can assume we will
> always be called from vcpu process context so we might not really need
> official affirmation from the system. For irqfd, we cannot predict who
> may be injecting the interrupt (for instance, it might be a
> PCI-passthrough hard-irq context). I am not sure if this knowledge
> actually helps to simplify the problem space, but I thought I should
> mention it.
>

We can't assume anything. Userspace might hand out that eventfd to anyone.

--
error compiling committee.c: too many arguments to function

2009-06-18 11:44:19

[permalink] [raw]

Subject: Re: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface

Avi Kivity wrote:
> On 06/16/2009 05:40 PM, Gregory Haskins wrote:
>>
>> Something else to consider: For iosignalfd, we can assume we will
>> always be called from vcpu process context so we might not really need
>> official affirmation from the system. For irqfd, we cannot predict who
>> may be injecting the interrupt (for instance, it might be a
>> PCI-passthrough hard-irq context). I am not sure if this knowledge
>> actually helps to simplify the problem space, but I thought I should
>> mention it.
>>
>
> We can't assume anything. Userspace might hand out that eventfd to
> anyone.

Yeah agreed, and I misspoke (mistyped? ;). It's not that I assume its
from a vcpu. Its that it doesn't matter (at least for vbus).

The reason is that the memory context is established in vbus via a
different channel (i.e. I never look at "current" in the context of the
eventfd callback). The eventfd is just telling me that the previously
established memory context needs to be looked at (e.g. the virtio-ring
changed). Therefore, the origin of the signal doesn't actually matter
(at least w.r.t. safe handling of the request).

Obviously, the question of whether something has really changed in the
memory and whether an errant signal could cause problems is a separate
issue. But we need to be robust against that anyway, for a multitude of
other reasons (e.g. malicious/broken guest scribbling over the
virtio-ring, etc).

-Greg

Attachments:

2009-06-18 14:01:28

Davide Libenzi wrote:
> On Thu, 18 Jun 2009, Gregory Haskins wrote:
>
>
>> Actually there is only one (the tx-thread) aside from the eventfd
>> imposed workqueue one. Incidentally, I would love to get rid of the
>> other thread too, so I am not just picking on eventfd ;). The other is
>> a lot harder since it has to update the virtio-ring and may need to page
>> in guest memory to do so.
>>
>
> No, there is the interface rx softirq too, that makes two.

Actually, I believe you are mistaken. It normally executes the softirq
in interrupt context, not a thread.

But I digress. Lets just shelve the SRCU conversation for another day.
It was my bad for introducing it now prematurely to solve a mostly
unrelated problem: the module-reference thing. I didn't realize the
SRCU change would be so controversial, and I didn't think to split
things apart as I have done today.

But the fact is: I do not see any way to actually use your referenceless
POLLHUP release code in a race free way without doing something like I
propose in 3/4, 4/4. Lets keep the discussion focused on that for now,
if we could.

Later, after we get this thing all built, I will re-run the numbers and
post some results so that Davide may have better proof that the context
switch overhead isn't just lost in the noise. I think that is all he is
asking for anyway, which is understandable.

Regards,
-Greg

Attachments:

2009-06-18 22:09:33

[permalink] [raw]

Subject: Re: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface

On Thu, 18 Jun 2009, Gregory Haskins wrote:

> Davide Libenzi wrote:
> > On Thu, 18 Jun 2009, Gregory Haskins wrote:
> >
> >
> >> Actually there is only one (the tx-thread) aside from the eventfd
> >> imposed workqueue one. Incidentally, I would love to get rid of the
> >> other thread too, so I am not just picking on eventfd ;). The other is
> >> a lot harder since it has to update the virtio-ring and may need to page
> >> in guest memory to do so.
> >>
> >
> > No, there is the interface rx softirq too, that makes two.
>
> Actually, I believe you are mistaken. It normally executes the softirq
> in interrupt context, not a thread.
>
> But I digress. Lets just shelve the SRCU conversation for another day.
> It was my bad for introducing it now prematurely to solve a mostly
> unrelated problem: the module-reference thing. I didn't realize the
> SRCU change would be so controversial, and I didn't think to split
> things apart as I have done today.
>
> But the fact is: I do not see any way to actually use your referenceless
> POLLHUP release code in a race free way without doing something like I
> propose in 3/4, 4/4. Lets keep the discussion focused on that for now,
> if we could.

OK, since I got literally swamped by the amount of talks and patches over
this theoretically simple topic, would you mind 1) posting the global
patch over eventfd 2) describe exactly what races are you talking about
3) explain why this should be any concern of eventfd at all?

- Davide

2009-06-18 22:47:41

Davide Libenzi wrote:
> On Fri, 19 Jun 2009, Gregory Haskins wrote:
>
>
>> eventfd currently emits a POLLHUP wakeup on f_ops->release() to generate a
>> notifier->release() callback. This lets notification 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 the notification API in a race-free way. This patch adds some
>> additional logic to the notification subsystem to rectify this problem.
>>
>> Background:
>> -----------------------
>> Eventfd currently only has one reference count mechanism: fget/fput. This
>> in of itself is normally fine. However, if a client expects to be
>> notified if the eventfd is closed, it cannot hold a fget() reference
>> itself or the underlying f_ops->release() callback will never be invoked
>> by VFS. Therefore we have this somewhat unusual situation where we may
>> hold a pointer to an eventfd object (by virtue of having a waiter registered
>> in its wait-queue), but no reference. This makes it nearly impossible to
>> design a mutual decoupling algorithm: you cannot unhook one side from the
>> other (or vice versa) without racing.
>>
>
> And why is that?
>
> struct xxx {
> struct mutex mtx;
> struct file *file;
> ...
> };
>
> struct file *xxx_get_file(struct xxx *x) {
> struct file *file;
>
> mutex_lock(&x->mtx);
> file = x->file;
> if (!file)
> mutex_unlock(&x->mtx);
> return file;
> }
>
> void xxx_release_file(struct xxx *x) {
> mutex_unlock(&x->mtx);
> }
>
> void handle_POLLHUP(struct xxx *x) {
> struct file *file;
>
> file = xxx_get_file(x);
> if (file) {
> unhook_waitqueue(file, ...);
> x->file = NULL;
> xxx_release_file(x);
> }
> }
>
>
> Every time you need to "use" file, you call xxx_get_file(), and if you get
> NULL, it means it's gone and you handle it accordigly to your IRQ fd
> policies. As soon as you done with the file, you call xxx_release_file().
> Replace "mtx" with the lock that fits your needs.
>

Consider what would happen if the f_ops->release() was preempted inside
the wake_up_locked_polled() after it dereferenced the xxx from the list,
but before it calls the callback(POLLHUP). The xxx object, and/or the
.text for the xxx object may be long gone by the time it comes back
around. Afaict, there is no way to guard against that scenario unless
you do something like 2/3+3/3. Or am I missing something?

-Greg

Attachments:

2009-06-19 21:33:15

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Fri, 19 Jun 2009, Gregory Haskins wrote:

> Davide Libenzi wrote:
> > On Fri, 19 Jun 2009, Gregory Haskins wrote:
> >
> >
> >> eventfd currently emits a POLLHUP wakeup on f_ops->release() to generate a
> >> notifier->release() callback. This lets notification 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 the notification API in a race-free way. This patch adds some
> >> additional logic to the notification subsystem to rectify this problem.
> >>
> >> Background:
> >> -----------------------
> >> Eventfd currently only has one reference count mechanism: fget/fput. This
> >> in of itself is normally fine. However, if a client expects to be
> >> notified if the eventfd is closed, it cannot hold a fget() reference
> >> itself or the underlying f_ops->release() callback will never be invoked
> >> by VFS. Therefore we have this somewhat unusual situation where we may
> >> hold a pointer to an eventfd object (by virtue of having a waiter registered
> >> in its wait-queue), but no reference. This makes it nearly impossible to
> >> design a mutual decoupling algorithm: you cannot unhook one side from the
> >> other (or vice versa) without racing.
> >>
> >
> > And why is that?
> >
> > struct xxx {
> > struct mutex mtx;
> > struct file *file;
> > ...
> > };
> >
> > struct file *xxx_get_file(struct xxx *x) {
> > struct file *file;
> >
> > mutex_lock(&x->mtx);
> > file = x->file;
> > if (!file)
> > mutex_unlock(&x->mtx);
> > return file;
> > }
> >
> > void xxx_release_file(struct xxx *x) {
> > mutex_unlock(&x->mtx);
> > }
> >
> > void handle_POLLHUP(struct xxx *x) {
> > struct file *file;
> >
> > file = xxx_get_file(x);
> > if (file) {
> > unhook_waitqueue(file, ...);
> > x->file = NULL;
> > xxx_release_file(x);
> > }
> > }
> >
> >
> > Every time you need to "use" file, you call xxx_get_file(), and if you get
> > NULL, it means it's gone and you handle it accordigly to your IRQ fd
> > policies. As soon as you done with the file, you call xxx_release_file().
> > Replace "mtx" with the lock that fits your needs.
> >
>
> Consider what would happen if the f_ops->release() was preempted inside
> the wake_up_locked_polled() after it dereferenced the xxx from the list,
> but before it calls the callback(POLLHUP). The xxx object, and/or the
> .text for the xxx object may be long gone by the time it comes back
> around. Afaict, there is no way to guard against that scenario unless
> you do something like 2/3+3/3. Or am I missing something?

Right. Don't you see an easier answer to that, instead of adding 300 lines
of code to eventfd?
For example, turning wake_up_locked() into a nornal wake_up().

- Davide

2009-06-19 21:50:08

Davide Libenzi wrote:
> On Fri, 19 Jun 2009, Davide Libenzi wrote:
>
>
>> On Fri, 19 Jun 2009, Gregory Haskins wrote:
>>
>>
>>> I am fairly confident it is not that simple after having thought about
>>> this issue over the last few days. But I've been wrong in the past.
>>> Propose a patch and I will review it for races/correctness, if you
>>> like. Perhaps a combination of that plus your asymmetrical locking
>>> scheme would work. One of the challenges you will hit is avoiding ABBA
>>> between your "get" lock and the wqh, but good luck!
>>>
>> A patch for what? The eventfd patch is a one-liner.
>>

Yes, understood. What I was trying to gently say is that the one-liner
proposal alone is still broken afaict. However, if there is another
solution that works that you like better than 133-liner I posted, I am
more than willing to help analyze it. In the end, I only care that this
is fixed.

>> It seems hard to believe that the thing cannot be handled on your side.
>> Once the wake_up_locked() is turned into a wake_up(), what other races are
>> there?
>>
>
> AFAICS, the IRQfd code simply registers the callback to ->poll() and waits
> for two events.
> In the POLLIN event, you schedule_work(&irqfd->inject) and there are no
> races there AFAICS (you basically do not care of anything eventfd memory
> related at all).
> For POLLHUP, you do:
>
> spin_lock(irqfd->slock);
> if (irqfd->wqh)
> schedule_work(&irqfd->inject);
> irqfd->wqh = NULL;
> spin_unlock(irqfd->slock);
>
> In your work function you notice the POLLHUP condition and take proper
> action (dunno what it is in your case).
> In your kvm_irqfd_release() function:
>
> spin_lock(irqfd->slock);
> if (irqfd->wqh)
> remove_wait_queue(irqfd->wqh, &irqfd->wait);
> irqfd->wqh = NULL;
> spin_unlock(irqfd->slock);
>
> Any races in there?
>

Yes, for one you have an ABBA deadlock on the irqfd->slock and wqh->lock
(recall wqh has to be locked to fix that other race I mentioned).

(As a hint, I think I fixed 4-5 races with these patches, so there are a
few others still lurking as well)

-Greg

Attachments:

2009-06-20 21:24:03

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Fri, 19 Jun 2009, Gregory Haskins wrote:

> > In the POLLIN event, you schedule_work(&irqfd->inject) and there are no
> > races there AFAICS (you basically do not care of anything eventfd memory
> > related at all).
> > For POLLHUP, you do:
> >
> > spin_lock(irqfd->slock);
> > if (irqfd->wqh)
> > schedule_work(&irqfd->inject);
> > irqfd->wqh = NULL;
> > spin_unlock(irqfd->slock);
> >
> > In your work function you notice the POLLHUP condition and take proper
> > action (dunno what it is in your case).
> > In your kvm_irqfd_release() function:
> >
> > spin_lock(irqfd->slock);
> > if (irqfd->wqh)
> > remove_wait_queue(irqfd->wqh, &irqfd->wait);
> > irqfd->wqh = NULL;
> > spin_unlock(irqfd->slock);
> >
> > Any races in there?
> >
>
> Yes, for one you have an ABBA deadlock on the irqfd->slock and wqh->lock
> (recall wqh has to be locked to fix that other race I mentioned).

Yep, true. How about we go in little steps?
How about the one below?
When you register your poll callback, you get a reference with
eventfd_refget().
At that point we have no more worries about the eventfd memory (namely
"wqh") going away.
Symmetrically, you use eventfd_refput() once you done with it.
So we basically de-couple the internal VFS refcount, from the eventfd
context memory refcount.
Where are the non-solveable races on the IRQfd side, of an approach like
this one?

> Yes, understood. What I was trying to gently say is that the one-liner
> proposal alone is still broken afaict. However, if there is another
> solution that works that you like better than 133-liner I posted, I am
> more than willing to help analyze it. In the end, I only care that this
> is fixed.

Is it so?
What I noticed here, is that you went from "Detailed Mode" (in the emails
where you were pushing the new bits), to "Hint Mode" (as below) when it
was time to see if there were simpler solutions to the problem.

> (As a hint, I think I fixed 4-5 races with these patches, so there are a
> few others still lurking as well)

Not knowing the details of IRQfd, hinting only is not going to help
anything in this case.

- Davide

---
fs/eventfd.c | 37 ++++++++++++++++++++++++++++++++++++-
include/linux/eventfd.h | 6 ++++++
2 files changed, 42 insertions(+), 1 deletion(-)

Index: linux-2.6.mod/fs/eventfd.c
===================================================================
--- linux-2.6.mod.orig/fs/eventfd.c 2009-06-20 13:08:22.000000000 -0700
+++ linux-2.6.mod/fs/eventfd.c 2009-06-20 14:00:23.000000000 -0700
@@ -17,8 +17,10 @@
#include <linux/eventfd.h>
#include <linux/syscalls.h>
#include <linux/module.h>
+#include <linux/kref.h>

struct eventfd_ctx {
+ struct kref kref;
wait_queue_head_t wqh;
/*
* Every time that a write(2) is performed on an eventfd, the
@@ -59,9 +61,19 @@ int eventfd_signal(struct file *file, in
}
EXPORT_SYMBOL_GPL(eventfd_signal);

+static void eventfd_free(struct kref *kref)
+{
+ struct eventfd_ctx *ctx = container_of(kref, struct eventfd_ctx, kref);
+
+ kfree(ctx);
+}
+
static int eventfd_release(struct inode *inode, struct file *file)
{
- kfree(file->private_data);
+ struct eventfd_ctx *ctx = file->private_data;
+
+ wake_up_poll(&ctx->wqh, POLLHUP);
+ kref_put(&ctx->kref, eventfd_free);
return 0;
}

@@ -201,6 +213,28 @@ struct file *eventfd_fget(int fd)
}
EXPORT_SYMBOL_GPL(eventfd_fget);

+int eventfd_refget(struct file *file)
+{
+ struct eventfd_ctx *ctx = file->private_data;
+
+ if (file->f_op != &eventfd_fops)
+ return -EINVAL;
+ kref_get(&ctx->kref);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_refget);
+
+int eventfd_refput(struct file *file)
+{
+ struct eventfd_ctx *ctx = file->private_data;
+
+ if (file->f_op != &eventfd_fops)
+ return -EINVAL;
+ kref_put(&ctx->kref, eventfd_free);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_refput);
+
SYSCALL_DEFINE2(eventfd2, unsigned int, count, int, flags)
{
int fd;
@@ -217,6 +251,7 @@ SYSCALL_DEFINE2(eventfd2, unsigned int,
if (!ctx)
return -ENOMEM;

+ kref_init(&ctx->kref);
init_waitqueue_head(&ctx->wqh);
ctx->count = count;
ctx->flags = flags;
Index: linux-2.6.mod/include/linux/eventfd.h
===================================================================
--- linux-2.6.mod.orig/include/linux/eventfd.h 2009-06-20 13:08:22.000000000 -0700
+++ linux-2.6.mod/include/linux/eventfd.h 2009-06-20 13:59:09.000000000 -0700
@@ -29,12 +29,18 @@

struct file *eventfd_fget(int fd);
int eventfd_signal(struct file *file, int n);
+int eventfd_refget(struct file *file);
+int eventfd_refput(struct file *file);

#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_refget(struct file *file)
+{ return -ENOSYS; }
+static inline int eventfd_refput(struct file *file)
+{ return -ENOSYS; }

#endif /* CONFIG_EVENTFD */

2009-06-20 22:17:29

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Sat, 20 Jun 2009, Davide Libenzi wrote:

> How about the one below?

Maybe with an interface that can be undone w/out a file* :)

- Davide

---
fs/eventfd.c | 34 +++++++++++++++++++++++++++++++++-
include/linux/eventfd.h | 8 ++++++++
2 files changed, 41 insertions(+), 1 deletion(-)

Index: linux-2.6.mod/fs/eventfd.c
===================================================================
--- linux-2.6.mod.orig/fs/eventfd.c 2009-06-20 13:08:22.000000000 -0700
+++ linux-2.6.mod/fs/eventfd.c 2009-06-20 15:07:18.000000000 -0700
@@ -17,8 +17,10 @@
#include <linux/eventfd.h>
#include <linux/syscalls.h>
#include <linux/module.h>
+#include <linux/kref.h>

struct eventfd_ctx {
+ struct kref kref;
wait_queue_head_t wqh;
/*
* Every time that a write(2) is performed on an eventfd, the
@@ -59,9 +61,19 @@ int eventfd_signal(struct file *file, in
}
EXPORT_SYMBOL_GPL(eventfd_signal);

+static void eventfd_free(struct kref *kref)
+{
+ struct eventfd_ctx *ctx = container_of(kref, struct eventfd_ctx, kref);
+
+ kfree(ctx);
+}
+
static int eventfd_release(struct inode *inode, struct file *file)
{
- kfree(file->private_data);
+ struct eventfd_ctx *ctx = file->private_data;
+
+ wake_up_poll(&ctx->wqh, POLLHUP);
+ kref_put(&ctx->kref, eventfd_free);
return 0;
}

@@ -201,6 +213,25 @@ struct file *eventfd_fget(int fd)
}
EXPORT_SYMBOL_GPL(eventfd_fget);

+struct eventfd_kref *eventfd_refget(struct file *file)
+{
+ struct eventfd_ctx *ctx = file->private_data;
+
+ if (file->f_op != &eventfd_fops)
+ return ERR_PTR(-EINVAL);
+ kref_get(&ctx->kref);
+ return (struct eventfd_kref *) ctx;
+}
+EXPORT_SYMBOL_GPL(eventfd_refget);
+
+void eventfd_refput(struct eventfd_kref *ref)
+{
+ struct eventfd_ctx *ctx = (struct eventfd_ctx *) ref;
+
+ kref_put(&ctx->kref, eventfd_free);
+}
+EXPORT_SYMBOL_GPL(eventfd_refput);
+
SYSCALL_DEFINE2(eventfd2, unsigned int, count, int, flags)
{
int fd;
@@ -217,6 +248,7 @@ SYSCALL_DEFINE2(eventfd2, unsigned int,
if (!ctx)
return -ENOMEM;

+ kref_init(&ctx->kref);
init_waitqueue_head(&ctx->wqh);
ctx->count = count;
ctx->flags = flags;
Index: linux-2.6.mod/include/linux/eventfd.h
===================================================================
--- linux-2.6.mod.orig/include/linux/eventfd.h 2009-06-20 13:08:22.000000000 -0700
+++ linux-2.6.mod/include/linux/eventfd.h 2009-06-20 15:06:03.000000000 -0700
@@ -8,6 +8,8 @@
#ifndef _LINUX_EVENTFD_H
#define _LINUX_EVENTFD_H

+struct eventfd_kref;
+
#ifdef CONFIG_EVENTFD

/* For O_CLOEXEC and O_NONBLOCK */
@@ -29,12 +31,18 @@

struct file *eventfd_fget(int fd);
int eventfd_signal(struct file *file, int n);
+struct eventfd_kref *eventfd_refget(struct file *file);
+void eventfd_refput(struct eventfd_kref *ref);

#else /* CONFIG_EVENTFD */

#define eventfd_fget(fd) ERR_PTR(-ENOSYS)
static inline int eventfd_signal(struct file *file, int n)
{ return 0; }
+static inline struct eventfd_kref *eventfd_refget(struct file *file)
+{ return ERR_PTR(-ENOSYS); }
+static inline void eventfd_refput(struct eventfd_kref *ref)
+{ }

#endif /* CONFIG_EVENTFD */

2009-06-20 23:55:15

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Sat, 20 Jun 2009, Davide Libenzi wrote:

> On Sat, 20 Jun 2009, Davide Libenzi wrote:
>
> > How about the one below?
>
> Maybe with an interface that can be undone w/out a file* :)

This is another alternative, based on a low-carb diet of your notifier
patch.
Same concept of de-coupling VFS refcount from eventfd memory context, and
allowing a poll callback register/unregister.
AFAICS, based on my limited knowledge of the IRQfd policies, your
->release() path needs to eventfd_pollcb_unregister() and wait for all
pending works to be done.

- Davide

---
fs/eventfd.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/eventfd.h | 23 ++++++++++++++++
2 files changed, 91 insertions(+), 1 deletion(-)

Index: linux-2.6.mod/fs/eventfd.c
===================================================================
--- linux-2.6.mod.orig/fs/eventfd.c 2009-06-20 16:25:45.000000000 -0700
+++ linux-2.6.mod/fs/eventfd.c 2009-06-20 16:35:22.000000000 -0700
@@ -17,8 +17,10 @@
#include <linux/eventfd.h>
#include <linux/syscalls.h>
#include <linux/module.h>
+#include <linux/kref.h>

struct eventfd_ctx {
+ struct kref kref;
wait_queue_head_t wqh;
/*
* Every time that a write(2) is performed on an eventfd, the
@@ -59,9 +61,29 @@ int eventfd_signal(struct file *file, in
}
EXPORT_SYMBOL_GPL(eventfd_signal);

+static void eventfd_free(struct kref *kref)
+{
+ struct eventfd_ctx *ctx = container_of(kref, struct eventfd_ctx, kref);
+
+ kfree(ctx);
+}
+
+static void eventfd_get(struct eventfd_ctx *ctx)
+{
+ kref_get(&ctx->kref);
+}
+
+static void eventfd_put(struct eventfd_ctx *ctx)
+{
+ kref_put(&ctx->kref, eventfd_free);
+}
+
static int eventfd_release(struct inode *inode, struct file *file)
{
- kfree(file->private_data);
+ struct eventfd_ctx *ctx = file->private_data;
+
+ wake_up_poll(&ctx->wqh, POLLHUP);
+ eventfd_put(ctx);
return 0;
}

@@ -217,6 +239,7 @@ SYSCALL_DEFINE2(eventfd2, unsigned int,
if (!ctx)
return -ENOMEM;

+ kref_init(&ctx->kref);
init_waitqueue_head(&ctx->wqh);
ctx->count = count;
ctx->flags = flags;
@@ -237,3 +260,47 @@ SYSCALL_DEFINE1(eventfd, unsigned int, c
return sys_eventfd2(count, 0);
}

+static void eventfd_pollcb_ptqueue(struct file *file, wait_queue_head_t *wqh,
+ poll_table *pt)
+{
+ struct eventfd_pollcb *ecb;
+
+ ecb = container_of(pt, struct eventfd_pollcb, pt);
+
+ add_wait_queue(wqh, &ecb->wait);
+}
+
+int eventfd_pollcb_register(struct file *file, struct eventfd_pollcb *ecb,
+ wait_queue_func_t cbf)
+{
+ struct eventfd_ctx *ctx;
+ unsigned int events;
+
+ if (file->f_op != &eventfd_fops)
+ return -EINVAL;
+
+ ctx = file->private_data;
+
+ /*
+ * Install our own custom wake-up handling so we are notified via
+ * a callback whenever someone signals the underlying eventfd.
+ */
+ init_waitqueue_func_entry(&ecb->wait, cbf);
+ init_poll_funcptr(&ecb->pt, eventfd_pollcb_ptqueue);
+
+ events = file->f_op->poll(file, &ecb->pt);
+
+ eventfd_get(ctx);
+ ecb->ctx = ctx;
+
+ return (events & POLLIN) ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_pollcb_register);
+
+void eventfd_pollcb_unregister(struct eventfd_pollcb *ecb)
+{
+ remove_wait_queue(&ecb->ctx->wqh, &ecb->wait);
+ eventfd_put(ecb->ctx);
+}
+EXPORT_SYMBOL_GPL(eventfd_pollcb_unregister);
+
Index: linux-2.6.mod/include/linux/eventfd.h
===================================================================
--- linux-2.6.mod.orig/include/linux/eventfd.h 2009-06-20 16:25:45.000000000 -0700
+++ linux-2.6.mod/include/linux/eventfd.h 2009-06-20 16:38:20.000000000 -0700
@@ -8,6 +8,20 @@
#ifndef _LINUX_EVENTFD_H
#define _LINUX_EVENTFD_H

+#include <linux/wait.h>
+#include <linux/poll.h>
+#include <linux/file.h>
+#include <linux/list.h>
+#include <linux/kref.h>
+
+struct eventfd_ctx;
+
+struct eventfd_pollcb {
+ poll_table pt;
+ struct eventfd_ctx *ctx;
+ wait_queue_t wait;
+};
+
#ifdef CONFIG_EVENTFD

/* For O_CLOEXEC and O_NONBLOCK */
@@ -29,12 +43,21 @@

struct file *eventfd_fget(int fd);
int eventfd_signal(struct file *file, int n);
+int eventfd_pollcb_register(struct file *file, struct eventfd_pollcb *ecb,
+ wait_queue_func_t cbf);
+void eventfd_pollcb_unregister(struct eventfd_pollcb *ecb);

#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_pollcb_register(struct file *file,
+ struct eventfd_pollcb *ecb,
+ wait_queue_func_t cbf)
+{ return -ENOSYS; }
+static inline void eventfd_pollcb_unregister(struct eventfd_pollcb *ecb)
+{ }

#endif /* CONFIG_EVENTFD */

2009-06-21 01:05:46

2009-06-22 18:42:27

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, Jun 22, 2009 at 11:03:22AM -0700, Davide Libenzi wrote:
> In your case of kernel-to-kernel scenario, why would you need eventfd at
> all, if userspace role in that model is simply to create it?

That's not 100% true. We have a mode where userspace is the producer
and/or consumer (migration mode) and we switch between that and
direct kernel-to-kernel communication.

2009-06-22 18:47:20

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, 22 Jun 2009, Gregory Haskins wrote:

> The general thesis is for decoupling of the two subsystems. In order to
> do this, you need some form of polymorphism and an intermediate "handle"
> mechanism which is userspace friendly. File-descriptors already fit
> this role neatly, with the "int fd" being the handle, and the f_ops
> being the polymorphic interface. Eventfd is of course, a subclass of
> this concept in that it has these same general properties but with
> signaling semantics (non-blocking collapsible events, etc).
>
> Say, for example, you wanted disk IO completion events to generate an
> interrupt into a guest. One way to do this would, of course, modify all
> the disk-io code so it knows how to directly inject a KVM guest
> interrupt. While this would work, someone would undoubtedly get flamed
> for such a suggestion ;)
>
> Another way to do it is to treat the AIO eventfd as the hook point.
> IIUC AIO already knows how to be an eventfd producer. KVM, by virtue of
> irqfd, already knows how to be an eventfd consumer. So now kvm can
> consume AIO, or it can consume userspace events equally well, and
> without modification. Neither side needs to know about the other per
> se, other than the details on how to use the eventfd interface.
>
> Don't get me wrong: We expect userspace to use all this stuff too. I
> just expect that we will see all permutations of producer/consumer +
> userspace/kernel combinations, so I want to retain that "all producers
> have left" notification feature set. Today eventfd supports producers
> or consumers in userspace, and producers in the kernel. This new work
> we are doing adds consumer support in the kernel. Kernel to kernel is
> just a natural extension of that.

A file* is the VFS link between userspace and the kernel. Is not a magical
polymorphic interface to be used for whatever kernel side reasons.
Basing a kernel internal API over it is flawed.
On top of that, a single reference count does not put you on cover about
the possible combinations of producers and consumers. For that, you'd need
a pipe-like reference handling logic, that is way far from the eventfd scope.
So please stop making hypothetical cases about interface usages, and
*when* we will have a real case, we'll see what the better handling for it
will be.

- Davide

2009-06-22 18:58:00

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, 22 Jun 2009, Michael S. Tsirkin wrote:

> On Mon, Jun 22, 2009 at 11:03:22AM -0700, Davide Libenzi wrote:
> > In your case of kernel-to-kernel scenario, why would you need eventfd at
> > all, if userspace role in that model is simply to create it?
>
> That's not 100% true. We have a mode where userspace is the producer
> and/or consumer (migration mode) and we switch between that and
> direct kernel-to-kernel communication.

Then you'd need to ask yourself how to handle your complex case inside the
KVM code, so that other eventfd users are not affected by the extra fat
needed to handle your scenarios. Thing that seem to be continuosly tried.
A file* based kernel-to-kernel interface is rather wrong IMO.

- Davide

2009-06-22 19:06:27

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote:
> A file* based kernel-to-kernel interface is rather wrong IMO.

But eventfd_ctx should work fine.

2009-06-22 19:22:46

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

Davide Libenzi wrote:
> On Mon, 22 Jun 2009, Michael S. Tsirkin wrote:
>
>
>> On Mon, Jun 22, 2009 at 11:03:22AM -0700, Davide Libenzi wrote:
>>
>>> In your case of kernel-to-kernel scenario, why would you need eventfd at
>>> all, if userspace role in that model is simply to create it?
>>>
>> That's not 100% true. We have a mode where userspace is the producer
>> and/or consumer (migration mode) and we switch between that and
>> direct kernel-to-kernel communication.
>>
>
> Then you'd need to ask yourself how to handle your complex case inside the
> KVM code, so that other eventfd users are not affected by the extra fat
> needed to handle your scenarios. Thing that seem to be continuosly tried.
> A file* based kernel-to-kernel interface is rather wrong IMO.
>

Well, I will point out that the interface in question is
eventfd_signal(struct file *), and you were the one that invented it
afaict. Can't help it if we like it :)

BTW: The termination point of that call is incidental. Afterall, it
works the same for kernel-kernel or kernel-userspace. The only relevant
discussion point here is that your proposal breaks POLLHUP for
eventfd_signal() users, and we have a real use case that cares.

Let me ask the question a different way: Lets say we all agree that
eventfd_signal() cannot be file* based to be correct. Is there a way
you can think of that satisfies both the need to get rid of file* AND
still deliver producer-release notification to consumers. Ultimately
that is all I care about.

-Greg

Attachments:

2009-06-22 19:27:22

Davide Libenzi wrote:
> On Mon, 22 Jun 2009, Gregory Haskins wrote:
>
>
>> Michael S. Tsirkin wrote:
>>
>>> On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote:
>>>
>>>
>>>> A file* based kernel-to-kernel interface is rather wrong IMO.
>>>>
>>>>
>>> But eventfd_ctx should work fine.
>>>
>>>
>> Yeah, and I guess we can always just say that qemu can't close the fd or
>> something. Seems hacky, but it might work if Davide insists we need his
>> change.
>>
>
> Continuing here, since there's no reason of having many subthreads talking
> about the same thing.
> Can you make a detailed example of what you're trying to achieve (no Hint
> Mode, please)?
> As it sounds to me, that you need a consumer/producer reference counting,
> to cover your scenario correctly.
>

Well, one of them was already briefly mentioned (the PCI-passthrough
thing). I am not personally working on this part (yet, anyway).

Another example of something I am actually working on as we speak would
be for this thing we are building called "virtual-bus". It is a way to
build/deploy device models directly in the kernel.

In either of these cases, we have this concept of allowing the guest to
notify the host, or vice versa, that something happened. Typically this
would be in reference to some chunk of shared memory, and the signaling
is telling the other side "I changed something, go look".

Without going into a ton of detail (unless, of course, you want it) is
that we are generalizing the signaling infrastructure (irqfd and
iosignalfd) so that something like PCI-passthrough or vbus are not
directly coupled to KVM. They communicate to KVM purely in terms of
(among other things) these irqfd/iosignalfd interfaces.

Using vbus as an example (though others are similar): vbus would
primarily exists as a kernel-model. However, there would be a small
device model in qemu-kem userspace to publish something like a PCI
device that declares its resource requirements to the guest. Some of
those requirements would be things like how many interrupts it needs,
and what IO ranges it supports, etc. When the guest programs the PCI
space, it maps the resources from its own world into the virtual PCI
resources emulated in qemu.

So up in userspace, the vbus pci-device would have an open reference to
the kvm guest (derived from /dev/kvm) and an open reference to a vbus
(derived from /dev/vbus). Lets call these kvmfd, and vbusfd,
respectively. For something like an interrupt, we would hook the point
where the PCI-MSI interrupt is assigned, and would do the following:

gsi = kvm_irq_route_gsi();
fd = eventfd(0, 0);
ioctl(kvmfd, KVM_IRQFD_ASSIGN, {fd, gsi});
ioctl(vbusfd, VBUS_SHMSIGNAL_ASSIGN, {sigid, fd});

So userspace orchestrated the assignment of this one eventfd to a KVM
consumer, and a VBUS producer. The two subsystems do not care about the
details of the other side of the link, per se. VBUS just knows that it
can eventfd_signal() its memory region to tell whomever is listening
that it changed. Likewise, KVM just knows to inject "gsi" when it gets
signalled. You could equally have given "fd" to a userspace thread for
either producer or consumer roles, or any other combination.

If we were doing PCI-passthough, substitute the last SHMSIGNAL_ASSIGN
ioctl call with some PCI_PASSTHROUGH_ASSIGN verb and you get the idea.

The important thing is that once this is established, userspace doesn't
necessarily care about the fd anymore. So now the question is: do we
keep it around for other things? Do we keep it around because we don't
want KVM to see the POLLHUP, or do we address the "release" code so that
it works even if userspace issued close(fd) at this point. I am not
sure what the answer is, but this is the scenario we are concerned with
in this thread. In the example above, vbus is free to produce events on
its eventfd until it gets a SHMSIGNAL_DEASSIGN request.

-Greg

>
>
> - Davide
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Attachments:

2009-06-22 20:29:47

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, Jun 22, 2009 at 03:26:41PM -0400, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote:
> >
> >> A file* based kernel-to-kernel interface is rather wrong IMO.
> >>
> >
> > But eventfd_ctx should work fine.
> >
>
> Yeah, and I guess we can always just say that qemu can't close the fd or
> something. Seems hacky, but it might work if Davide insists we need his
> change.

Yes, I think that's fine.

--
MST

2009-06-22 23:00:23

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, 22 Jun 2009, Gregory Haskins wrote:

> So up in userspace, the vbus pci-device would have an open reference to
> the kvm guest (derived from /dev/kvm) and an open reference to a vbus
> (derived from /dev/vbus). Lets call these kvmfd, and vbusfd,
> respectively. For something like an interrupt, we would hook the point
> where the PCI-MSI interrupt is assigned, and would do the following:
>
> gsi = kvm_irq_route_gsi();
> fd = eventfd(0, 0);
> ioctl(kvmfd, KVM_IRQFD_ASSIGN, {fd, gsi});
> ioctl(vbusfd, VBUS_SHMSIGNAL_ASSIGN, {sigid, fd});
>
> So userspace orchestrated the assignment of this one eventfd to a KVM
> consumer, and a VBUS producer. The two subsystems do not care about the
> details of the other side of the link, per se. VBUS just knows that it
> can eventfd_signal() its memory region to tell whomever is listening
> that it changed. Likewise, KVM just knows to inject "gsi" when it gets
> signalled. You could equally have given "fd" to a userspace thread for
> either producer or consumer roles, or any other combination.
>
> If we were doing PCI-passthough, substitute the last SHMSIGNAL_ASSIGN
> ioctl call with some PCI_PASSTHROUGH_ASSIGN verb and you get the idea.
>
> The important thing is that once this is established, userspace doesn't
> necessarily care about the fd anymore. So now the question is: do we
> keep it around for other things? Do we keep it around because we don't
> want KVM to see the POLLHUP, or do we address the "release" code so that
> it works even if userspace issued close(fd) at this point. I am not
> sure what the answer is, but this is the scenario we are concerned with
> in this thread. In the example above, vbus is free to produce events on
> its eventfd until it gets a SHMSIGNAL_DEASSIGN request.

I see.
The thing remains, that in order to reliably handle generic
producer/consumer scenarios you'd need a reference counting similar to
pipes, where the notion of producer and consumer is very well defined.
In your case, it'd be sufficent to expose an:

int eventfd_wakeup(struct eventfd_ctx *ctx, void *key);

So that each end can signal, in an file* f_count independent way, when
they're about to leave. Say with a new status bit.
The problem, that I felt coming since when I introduced the key-based
wakeups, is that right now the "key" start to become a little restrictive
in terms of amount of information carried by it.
If there are other key-aware waiters on "wqh", your new bit cannot
conflict with existing ones, and you (and future users of the interface)
will be polluting the global bit namespace.
Probably turning "key" into a pointer to a structure like:

struct wakeup_key {
unsigned long type;
void *key;
};

So the current:

wake_up_poll(wq, POLLIN);

Would become:

key.type = KT_POLLMASK;
key.key = POLLIN;
wake_up_key(wq, &key);

At that point the waiters (like poll/epoll) can simply discard the wakeup
types they are not interested into (poll/epoll would care only about
KT_POLLMASK), and something more than a simple bitmask can be carried by
the wakups. Like:

key.type = KT_SOMEDATA;
key.key = &data;
wake_up_key(wq, &key);

Of course, we could build another layer on top, to fit this model, but
then we'd have the dual citizenship among normal wakeups and new-interface
wakeups/signaling.
Another thing. Now that the interface exposes the eventfd_ctx context
directly, the pollcb register/unregister does not have any reason to be
inside eventfd (they're totally generic bits at that point), so my thought
was about to drop them.

- Davide

2009-06-23 01:04:03

Davide Libenzi wrote:
> On Mon, 22 Jun 2009, Gregory Haskins wrote:
>
>
>> Davide Libenzi wrote:
>>
>>> On Mon, 22 Jun 2009, Gregory Haskins wrote:
>>>
>>>
>>>
>>>> So up in userspace, the vbus pci-device would have an open reference to
>>>> the kvm guest (derived from /dev/kvm) and an open reference to a vbus
>>>> (derived from /dev/vbus). Lets call these kvmfd, and vbusfd,
>>>> respectively. For something like an interrupt, we would hook the point
>>>> where the PCI-MSI interrupt is assigned, and would do the following:
>>>>
>>>> gsi = kvm_irq_route_gsi();
>>>> fd = eventfd(0, 0);
>>>> ioctl(kvmfd, KVM_IRQFD_ASSIGN, {fd, gsi});
>>>> ioctl(vbusfd, VBUS_SHMSIGNAL_ASSIGN, {sigid, fd});
>>>>
>>>> So userspace orchestrated the assignment of this one eventfd to a KVM
>>>> consumer, and a VBUS producer. The two subsystems do not care about the
>>>> details of the other side of the link, per se. VBUS just knows that it
>>>> can eventfd_signal() its memory region to tell whomever is listening
>>>> that it changed. Likewise, KVM just knows to inject "gsi" when it gets
>>>> signalled. You could equally have given "fd" to a userspace thread for
>>>> either producer or consumer roles, or any other combination.
>>>>
>>>> If we were doing PCI-passthough, substitute the last SHMSIGNAL_ASSIGN
>>>> ioctl call with some PCI_PASSTHROUGH_ASSIGN verb and you get the idea.
>>>>
>>>> The important thing is that once this is established, userspace doesn't
>>>> necessarily care about the fd anymore. So now the question is: do we
>>>> keep it around for other things? Do we keep it around because we don't
>>>> want KVM to see the POLLHUP, or do we address the "release" code so that
>>>> it works even if userspace issued close(fd) at this point. I am not
>>>> sure what the answer is, but this is the scenario we are concerned with
>>>> in this thread. In the example above, vbus is free to produce events on
>>>> its eventfd until it gets a SHMSIGNAL_DEASSIGN request.
>>>>
>>>>
>>> I see.
>>> The thing remains, that in order to reliably handle generic
>>> producer/consumer scenarios you'd need a reference counting similar to
>>> pipes, where the notion of producer and consumer is very well defined.
>>>
>>>
>> I see your point.
>>
>> Well, I think the more important thing here is that we address the
>> races, and add support for DEASSIGN. We can do both of those things
>> with any of the patches that you and I have been kicking around. So
>> what I propose is that we move forward with whatever patch you bless as
>> proper for now. This producer-release issue is pretty minor in the
>> grand scheme of things. We can always just have userspace hold the fd.
>>
>
> Is it a real problem? Can it be decently handled on the KVM side?
> Reason I'm asking, is that I wouldn't want to change the interface, only
> to find it unsuitable a few weeks later.
>

To be honest, I am not sure. I would guess its not a *huge* deal,
though it was obviously enough of a concern to at least discuss it. I
can definitely say that I think the other issues which are being fixed
are substantially more important.

>
>
>
>
>> I can either take in the last one you sent, or it sounds like you wanted
>> to possibly do another round of cleanup? Whatever the case may be, let
>> me know and we can coordinate with Andrew/Avi on what tree to pull it
>> into. It sounds like riding in kvm.git is the perhaps the most logical.
>>
>
> Yes, I'd like to drop the pollcb bits (that you can implement KVM-side),
> at least.
> And yes, going kvm.git is probably the best path.
>

Sounds good. Thanks for your patience, Davide. I think we are almost
there! ;)

-Greg

>
>
> - Davide
>
>
>

Attachments:

2009-06-23 03:25:34

by Rusty Russell

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, 22 Jun 2009 09:24:20 am Davide Libenzi wrote:
> I actually ended up exposing the eventfd context anyway, since IMO is a
> better option instead of holding references to the eventfd file (that
> makes VFS people uneasy).

lguest is a special case since we don't let them close the fds, except by
closing /dev/lguest.

Lguest change seems fine, but:

> Index: linux-2.6.mod/drivers/lguest/lg.h
> ===================================================================
> --- linux-2.6.mod.orig/drivers/lguest/lg.h 2009-06-21 15:55:17.000000000
> -0700 +++ linux-2.6.mod/drivers/lguest/lg.h 2009-06-21 15:56:00.000000000
> -0700 @@ -80,9 +80,11 @@ struct lg_cpu {
> struct lg_cpu_arch arch;
> };
>
> +struct eventfd_ctx;
> +
> struct lg_eventfd {
> unsigned long addr;
> - struct file *event;
> + struct eventfd_ctx *event;
> };

The first 'struct eventfd_ctx;' line is not required.

Thanks,
Rusty.

2009-06-23 14:35:50

[permalink] [raw]

Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

On Mon, 22 Jun 2009, Gregory Haskins wrote:

> To be honest, I am not sure. I would guess its not a *huge* deal,
> though it was obviously enough of a concern to at least discuss it. I
> can definitely say that I think the other issues which are being fixed
> are substantially more important.

Ok then, will repost the revised patch later today.

- Davide

2009-06-23 14:37:30

Michael S. Tsirkin wrote:
> On Thu, Jun 25, 2009 at 01:32:22PM -0400, Gregory Haskins wrote:
>
>> Please take a close look at it and consider for merging, if you would.
>>
>
> With all the incremental patching, I kind of lost track of what the
> complete file would look like. Is there a git tree I could pull?
>
>

Ask and ye shall receive :)

git pull
ssh://master.kernel.org/pub/scm/linux/kernel/git/ghaskins/linux-2.6-hacks.git
kvm/irqfd

If you dont have a kernel.org account, it should rsync to the
gitweb/git: server soon, in which case
s|ssh://master|git://git should get you the proper path.

Note that there is one additional race that I found that is not included
in that tree in the slow-work infrastructure. You can find my latest
submission to David Howells to resolve that issue here:

http://patchwork.kernel.org/patch/32287/

Thanks Michael!
-Greg

Attachments:

2009-06-26 11:24:21