Gregory Haskins wrote:
> eventfd currently emits a POLLHUP wakeup on f_ops->release() to generate a
> "release" callback. This lets eventfd clients know if the eventfd is about
> to go away and is very useful particularly for in-kernel clients. However,
> until recently it is not possible to use this feature of eventfd in a
> race-free way. This patch utilizes a new eventfd interface to rectify
> the problem.
>
> Note that one final race is known to exist: the slow-work thread may race
> with module removal. We are currently working with slow-work upstream
> to fix this issue as well. Since the code prior to this patch also
> races with module_put(), we are not making anything worse, but rather
> shifting the cause of the race. Once the slow-work code is patched we
> will be fixing the last remaining issue.
>
> Signed-off-by: Gregory Haskins <ghaskins@xxxxxxxxxx>
> ---
>
> include/linux/kvm_host.h | 7 +-
> virt/kvm/Kconfig | 1
> virt/kvm/eventfd.c | 199 ++++++++++++++++++++++++++++++++++++++++------
> 3 files changed, 179 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2451f48..d94ee72 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -141,7 +141,12 @@ struct kvm {
> struct kvm_io_bus mmio_bus;
> struct kvm_io_bus pio_bus;
> #ifdef CONFIG_HAVE_KVM_EVENTFD
> - struct list_head irqfds;
> + struct {
> + spinlock_t lock;
> + struct list_head items;
> + atomic_t outstanding;
> + wait_queue_head_t wqh;
> + } irqfds;
> #endif
> struct kvm_vm_stat stat;
> struct kvm_arch arch;
> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> index daece36..ab7848a 100644
> --- a/virt/kvm/Kconfig
> +++ b/virt/kvm/Kconfig
> @@ -9,6 +9,7 @@ config HAVE_KVM_IRQCHIP
> config HAVE_KVM_EVENTFD
> bool
> select EVENTFD
> + select SLOW_WORK
>
> config KVM_APIC_ARCHITECTURE
> bool
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 9656027..ca21e8a 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -28,6 +28,7 @@
> #include <linux/file.h>
> #include <linux/list.h>
> #include <linux/eventfd.h>
> +#include <linux/slow-work.h>
>
> /*
> * --------------------------------------------------------------------
> @@ -36,17 +37,36 @@
> * Credit goes to Avi Kivity for the original idea.
> * --------------------------------------------------------------------
> */
> +
> struct _irqfd {
> struct kvm *kvm;
> + struct eventfd_ctx *eventfd;
> int gsi;
> struct list_head list;
> poll_table pt;
> wait_queue_head_t *wqh;
> wait_queue_t wait;
> struct work_struct inject;
> + struct slow_work shutdown;
> + int active:1;
> };
>
> static void
> +irqfd_release(struct _irqfd *irqfd)
> +{
> + eventfd_ctx_put(irqfd->eventfd);
> + kfree(irqfd);
> +}
> +
> +/* assumes kvm->irqfds.lock is held */
> +static void
> +irqfd_deactivate(struct _irqfd *irqfd)
> +{
> + irqfd->active = false;
> + list_del(&irqfd->list);
> +}
> +
> +static void
> irqfd_inject(struct work_struct *work)
> {
> struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
> @@ -58,6 +78,55 @@ irqfd_inject(struct work_struct *work)
> mutex_unlock(&kvm->irq_lock);
> }
>
> +static struct _irqfd *
> +work_to_irqfd(struct slow_work *work)
> +{
> + return container_of(work, struct _irqfd, shutdown);
> +}
> +
> +static int
> +irqfd_shutdown_get_ref(struct slow_work *work)
> +{
> + struct _irqfd *irqfd = work_to_irqfd(work);
> + struct kvm *kvm = irqfd->kvm;
> +
> + atomic_inc(&kvm->irqfds.outstanding);
> +
> + return 0;
> +}
> +
> +static void
> +irqfd_shutdown_put_ref(struct slow_work *work)
> +{
> + struct _irqfd *irqfd = work_to_irqfd(work);
> + struct kvm *kvm = irqfd->kvm;
> +
> + irqfd_release(irqfd);
> +
> + if (atomic_dec_and_test(&kvm->irqfds.outstanding))
> + wake_up(&kvm->irqfds.wqh);
> +}
> +
> +static void
> +irqfd_shutdown_execute(struct slow_work *work)
> +{
> + struct _irqfd *irqfd = work_to_irqfd(work);
> +
> + /*
> + * Ensure there are no outstanding "inject" work-items before we blow
> + * away our state. Once this job completes, the slow_work
> + * infrastructure will drop the irqfd object completely via put_ref
> + */
> + flush_work(&irqfd->inject);
> +}
> +
> +const static struct slow_work_ops irqfd_shutdown_work_ops = {
> + .get_ref = irqfd_shutdown_get_ref,
> + .put_ref = irqfd_shutdown_put_ref,
> + .execute = irqfd_shutdown_execute,
> +};
> +
> +
> static int
> irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
> {
> @@ -65,25 +134,39 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
> unsigned long flags = (unsigned long)key;
>
> /*
> - * Assume we will be called with interrupts disabled
> + * Called with interrupts disabled
> */
> if (flags & POLLIN)
> - /*
> - * Defer the IRQ injection until later since we need to
> - * acquire the kvm->lock to do so.
> - */
> + /* An event has been signaled, inject an interrupt */
> schedule_work(&irqfd->inject);
>
> if (flags & POLLHUP) {
> - /*
> - * for now, just remove ourselves from the list and let
> - * the rest dangle. We will fix this up later once
> - * the races in eventfd are fixed
> - */
> + /* The eventfd is closing, detach from KVM */
> + struct kvm *kvm = irqfd->kvm;
> + unsigned long flags;
> +
> __remove_wait_queue(irqfd->wqh, &irqfd->wait);
> - irqfd->wqh = NULL;
> +
> + spin_lock_irqsave(&kvm->irqfds.lock, flags);
> +
> + if (irqfd->active) {
> + /*
> + * If the item is still active we can be sure that
> + * no-one else is trying to shutdown this object at
> + * the same time.
> + *
> + * Defer the shutdown to a thread so we can flush
> + * all remaining inject jobs. We use a slow-work
> + * item to prevent a deadlock against the work-queue
> + */
> + irqfd_deactivate(irqfd);
> + slow_work_enqueue(&irqfd->shutdown);
> + }
> +
> + spin_unlock_irqrestore(&kvm->irqfds.lock, flags);
> }
>
> +
> return 0;
> }
>
> @@ -102,6 +185,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
> {
> struct _irqfd *irqfd;
> struct file *file = NULL;
> + struct eventfd_ctx *eventfd = NULL;
> int ret;
> unsigned int events;
>
> @@ -113,6 +197,8 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
> irqfd->gsi = gsi;
> INIT_LIST_HEAD(&irqfd->list);
> INIT_WORK(&irqfd->inject, irqfd_inject);
> + slow_work_init(&irqfd->shutdown, &irqfd_shutdown_work_ops);
> + irqfd->active = true;
>
> file = eventfd_fget(fd);
> if (IS_ERR(file)) {
> @@ -129,12 +215,21 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
>
> events = file->f_op->poll(file, &irqfd->pt);
>
> - mutex_lock(&kvm->lock);
> - list_add_tail(&irqfd->list, &kvm->irqfds);
> - mutex_unlock(&kvm->lock);
> + spin_lock_irq(&kvm->irqfds.lock);
> + list_add_tail(&irqfd->list, &kvm->irqfds.items);
> + spin_unlock_irq(&kvm->irqfds.lock);
> +
> + eventfd = eventfd_ctx_fileget(file);
> + if (IS_ERR(file)) {
> + ret = PTR_ERR(file);
> + goto fail;
> + }
> +
> + irqfd->eventfd = eventfd;
>
<sigh> I just noticed this while doing a self review: I need to assign
the eventfd context *before* putting the item on the list. Not sure why
I even did that. I suspect I re-arranged the code at the last minute
and didn't notice what a dumb thing I was doing.
So this will need at least a v6, but I will wait to hear if there are
any other comments from Michael et. al.
-Greg
>
> /*
> - * Check if there was an event already queued
> + * Check if there was an event already pending on the eventfd
> + * before we registered, and trigger it as if we didn't miss it.
> */
> if (events & POLLIN)
> schedule_work(&irqfd->inject);
> @@ -148,6 +243,9 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
> return 0;
>
> fail:
> + if (eventfd && !IS_ERR(eventfd))
> + eventfd_ctx_put(eventfd);
> +
> if (file && !IS_ERR(file))
> fput(file);
>
> @@ -158,24 +256,71 @@ fail:
> void
> kvm_irqfd_init(struct kvm *kvm)
> {
> - INIT_LIST_HEAD(&kvm->irqfds);
> + slow_work_register_user();
> +
> + spin_lock_init(&kvm->irqfds.lock);
> + INIT_LIST_HEAD(&kvm->irqfds.items);
> + atomic_set(&kvm->irqfds.outstanding, 0);
> + init_waitqueue_head(&kvm->irqfds.wqh);
> +}
> +
> +static struct _irqfd *
> +irqfd_pop(struct kvm *kvm)
> +{
> + struct _irqfd *irqfd = NULL;
> +
> + spin_lock_irq(&kvm->irqfds.lock);
> +
> + if (!list_empty(&kvm->irqfds.items)) {
> + irqfd = list_first_entry(&kvm->irqfds.items,
> + struct _irqfd, list);
> + irqfd_deactivate(irqfd);
> + }
> +
> + spin_unlock_irq(&kvm->irqfds.lock);
> +
> + return irqfd;
> +}
> +
> +/*
> + * locally releases the irqfd
> + *
> + * This function is called when KVM won the race with eventfd (signalled by
> + * finding the item active on the kvm->irqfds.item list). We are now guaranteed
> + * that we will never schedule a deferred shutdown task against this object,
> + * so we take steps to perform the shutdown ourselves.
> + *
> + * 1) We must remove ourselves from the wait-queue to prevent further events,
> + * which will simultaneously act to sync us with eventfd (via wqh->lock)
> + * 2) Flush any outstanding inject-tasks to ensure its safe to free memory
> + * 3) Delete the object
> + */
> +static void
> +irqfd_shutdown(struct _irqfd *irqfd)
> +{
> + remove_wait_queue(irqfd->wqh, &irqfd->wait);
> + flush_work(&irqfd->inject);
> + irqfd_release(irqfd);
> }
>
> void
> kvm_irqfd_release(struct kvm *kvm)
> {
> - struct _irqfd *irqfd, *tmp;
> -
> - list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
> - if (irqfd->wqh)
> - remove_wait_queue(irqfd->wqh, &irqfd->wait);
> + struct _irqfd *irqfd;
>
> - flush_work(&irqfd->inject);
> + /*
> + * Shutdown all irqfds that still remain
> + */
> + while ((irqfd = irqfd_pop(kvm)))
> + irqfd_shutdown(irqfd);
>
> - mutex_lock(&kvm->lock);
> - list_del(&irqfd->list);
> - mutex_unlock(&kvm->lock);
> + /*
> + * irqfds.outstanding tracks the number of outstanding "shutdown"
> + * jobs pending at any given time. Once we get here, we know that
> + * no more jobs will get scheduled, so go ahead and block until all
> + * of them complete
> + */
> + wait_event(kvm->irqfds.wqh, (!atomic_read(&kvm->irqfds.outstanding)));
>
> - kfree(irqfd);
> - }
> + slow_work_unregister_user();
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Attachment:
signature.asc
Description: OpenPGP digital signature
