lkml.org 
[lkml]   [2009]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [KVM-RFC PATCH 2/2] kvm: use POLLHUP to close an irqfd instead of an explicit ioctl
    Paul E. McKenney wrote:
    > On Tue, Jun 02, 2009 at 02:23:14PM -0400, Gregory Haskins wrote:
    >
    >> Paul E. McKenney wrote:
    >>
    >>> On Tue, Jun 02, 2009 at 11:15:38AM -0400, Gregory Haskins wrote:
    >>>
    >>>
    >>>> Assigning an irqfd object to a kvm object creates a relationship that we
    >>>> currently manage by having the kvm oject acquire/hold a file* reference to
    >>>> the underlying eventfd. The lifetime of these objects is properly maintained
    >>>> by decoupling the two objects whenever the irqfd is closed or kvm is closed,
    >>>> whichever comes first.
    >>>>
    >>>> However, the irqfd "close" method is less than ideal since it requires two
    >>>> system calls to complete (one for ioctl(kvmfd, IRQFD_DEASSIGN), the other for
    >>>> close(eventfd)). This dual-call approach was utilized because there was no
    >>>> notification mechanism on the eventfd side at the time irqfd was implemented.
    >>>>
    >>>> Recently, Davide proposed a patch to send a POLLHUP wakeup whenever an
    >>>> eventfd is about to close. So we eliminate the IRQFD_DEASSIGN ioctl (*)
    >>>> vector in favor of sensing the desassign automatically when the fd is closed.
    >>>> The resulting code is slightly more complex as a result since we need to
    >>>> allow either side to sever the relationship independently. We utilize SRCU
    >>>> to guarantee stable concurrent access to the KVM pointer without adding
    >>>> additional atomic operations in the fast path.
    >>>>
    >>>> At minimum, this design should be acked by both Davide and Paul (cc'd).
    >>>>
    >>>> (*) The irqfd patch does not exist in any released tree, so the understanding
    >>>> is that we can alter the irqfd specific ABI without taking the normal
    >>>> precautions, such as CAP bits.
    >>>>
    >>>>
    >>> A few questions and suggestions interspersed below.
    >>>
    >>> Thanx, Paul
    >>>
    >>>
    >> Thanks for the review, Paul.
    >>
    >
    > Some questions, clarifications, and mea culpas below.
    >
    > Thanx, Paul
    >
    >
    >> (FYI: This isn't quite what I was asking you about on IRC yesterday, but
    >> it's related...and the SRCU portion of the conversation *did* inspire me
    >> here. Just note that the stuff I was asking about is still forthcoming)
    >>
    >
    > ;-)
    >
    >
    >>>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
    >>>> CC: Davide Libenzi <davidel@xmailserver.org>
    >>>> CC: Michael S. Tsirkin <mst@redhat.com>
    >>>> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    >>>> ---
    >>>>
    >>>> include/linux/kvm.h | 2 -
    >>>> virt/kvm/eventfd.c | 177 +++++++++++++++++++++++----------------------------
    >>>> virt/kvm/kvm_main.c | 3 +
    >>>> 3 files changed, 81 insertions(+), 101 deletions(-)
    >>>>
    >>>> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
    >>>> index 632a856..29b62cc 100644
    >>>> --- a/include/linux/kvm.h
    >>>> +++ b/include/linux/kvm.h
    >>>> @@ -482,8 +482,6 @@ struct kvm_x86_mce {
    >>>> };
    >>>> #endif
    >>>>
    >>>> -#define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
    >>>> -
    >>>> struct kvm_irqfd {
    >>>> __u32 fd;
    >>>> __u32 gsi;
    >>>> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
    >>>> index f3f2ea1..6ed62e2 100644
    >>>> --- a/virt/kvm/eventfd.c
    >>>> +++ b/virt/kvm/eventfd.c
    >>>> @@ -37,26 +37,63 @@
    >>>> * --------------------------------------------------------------------
    >>>> */
    >>>> struct _irqfd {
    >>>> + struct mutex lock;
    >>>> + struct srcu_struct srcu;
    >>>> struct kvm *kvm;
    >>>> int gsi;
    >>>> - struct file *file;
    >>>> struct list_head list;
    >>>> poll_table pt;
    >>>> wait_queue_head_t *wqh;
    >>>> wait_queue_t wait;
    >>>> - struct work_struct work;
    >>>> + struct work_struct inject;
    >>>> };
    >>>>
    >>>> static void
    >>>> irqfd_inject(struct work_struct *work)
    >>>> {
    >>>> - struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
    >>>> - struct kvm *kvm = irqfd->kvm;
    >>>> + 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);
    >>>>

    [4]

    >>>> + if (kvm) {
    >>>> + mutex_lock(&kvm->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->lock);
    >>>> + }
    >>>> +
    >>>> + srcu_read_unlock(&irqfd->srcu, idx);
    >>>> +}
    >>>>
    >>>>
    >> [1]
    >>
    >>
    >>>> +
    >>>> +static void
    >>>> +irqfd_disconnect(struct _irqfd *irqfd)
    >>>> +{
    >>>> + struct kvm *kvm;
    >>>> +
    >>>> + mutex_lock(&irqfd->lock);
    >>>> +
    >>>> + kvm = rcu_dereference(irqfd->kvm);
    >>>> + rcu_assign_pointer(irqfd->kvm, NULL);
    >>>> +
    >>>> + mutex_unlock(&irqfd->lock);
    >>>>
    >>>>
    >> [2]
    >>
    >>
    >>>> +
    >>>> + if (!kvm)
    >>>> + return;
    >>>>
    >>>> mutex_lock(&kvm->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);
    >>>> + list_del(&irqfd->list);
    >>>> mutex_unlock(&kvm->lock);
    >>>> +
    >>>> + /*
    >>>> + * It is important to not drop the kvm reference until the next grace
    >>>> + * period because there might be lockless references in flight up
    >>>> + * until then
    >>>> + */
    >>>>
    >>>>
    >>> The lockless references are all harmless even if carried out after the
    >>> kvm structure has been removed?
    >>>
    >> No, but all ([1]) references to my knowledge occur within an srcu
    >> read-side CS, and we only drop the object reference ([3]) outside of
    >> that CS by virtue of the synchronize_srcu() barrier below. The one
    >> notable exception is [2], which I don't declare as a read-side CS since
    >> I hold the mutex during the swap.
    >>
    >> OTOH, this is really my _intention_, not _reality_ per se. ;) E.g. I
    >> may have completely flubbed up the design, so I'm glad you are looking
    >> at it.
    >>
    >
    > Looks good in general -- my question is about the window of time
    > between when the object is removed from the list (and might still have
    > readers referencing it) and when the object is freed (and, courtesy of
    > synchronize_srcu(), can no longer be referenced by readers).
    >
    > In other words, the following sequence of events:
    >
    > o CPU 0 picks up a pointer to the object.
    >
    > o CPU 1 removes that same object from the list, and invokes
    > synchronize_srcu(), which cannot return yet due to CPU 0
    > being in an SRCU read-side critical section.
    >
    > o CPU 0 acquires the lock and invokes the pair of kvm_set_irq()
    > calls, releases the lock and exits the SRCU read-side critical
    > section.
    >
    > o CPU 1's synchronize_srcu() can now return, and does.
    >
    > o CPU 1 frees the object.
    >
    > I honestly don't know enough about KVM to say whether or not this is a
    > problem, but thought I should ask. ;-)
    >

    Right, ok. What you outline is consistent with my expectations. That
    is, I need to make sure that it is not possible to have any concurrent
    code call kvm_put_cpu between [4] and [1] against the same pointer. It
    is, of course, ok if some other code path enters irqfd_inject() _after_
    [2] because I would have already swapped the pointer with NULL and it
    will simply bail out on the conditional right after [4].
    >
    >>> Or does there need to be a ->deleted
    >>> field that allows the lockless references to ignore a kvm structure that
    >>> has already been deleted?
    >>>
    >> I guess you could say that the "rcu_assign_pointer(NULL)" is my
    >> "deleted" field. The reader-side code in question should check for that
    >> condition before proceeding.
    >>
    >
    > Fair enough! But please keep in mind that the pointer could be assigned
    > to NULL just after we dereference it, especially if we are interrupted
    > or preempted or something.

    Right, and that should be ok IIUC as long as I can be guaranteed to
    never call kvm_put_kvm() before the dereferencer calls
    srcu_read_unlock(). I currently believe this guarantee is provided by
    the synchronize_srcu() at [3], but please straighten me out if that is a
    naive assumption.

    > Or is the idea to re-check the pointer under some lock?
    >
    >
    I do not currently believe I need to worry about that case, but as
    always, straighten me out if that is wrong. ;)

    >>> On the other hand, if it really is somehow OK for kvm_set_irq() to be
    >>> called on an already-deleted kvm structure, then this code would be OK
    >>> as is.
    >>>
    >> Definitely not, so if you think that can happen please raise the flag.
    >>
    >
    > Apologies, I was being a bit sloppy. Instead of "already-deleted",
    > I should have said something like "already removed from the list but
    > not yet freed".
    >

    Ah, ok. The answer in that case would be "yes". It's ok to call
    kvm_set_irq() while the irqfd->kvm pointer is NULL, but it is not ok to
    call it after or during kvm_put_kvm() has been invoked. Technically the
    first safe point is right after the last mutex_unlock(&kvm->lock)
    completes (right before [1]), and is officially annotated with the
    subsequent srcu_read_unlock().
    >
    >>>> + synchronize_srcu(&irqfd->srcu);
    >>>> + kvm_put_kvm(kvm);
    >>>>
    >>>>
    >> [3]
    >>
    >>
    >>>> }
    >>>>
    >>>> static int
    >>>> @@ -64,12 +101,28 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
    >>>> {
    >>>> struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
    >>>>
    >>>> - /*
    >>>> - * The 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->work);
    >>>> + switch ((unsigned long)key) {
    >>>> + case 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);
    >>>> + break;
    >>>> + case POLLHUP:
    >>>> + /*
    >>>> + * The POLLHUP is called unlocked, so it theoretically should
    >>>> + * be safe to remove ourselves from the wqh
    >>>> + */
    >>>> + remove_wait_queue(irqfd->wqh, &irqfd->wait);
    >>>> + flush_work(&irqfd->inject);
    >>>> + irqfd_disconnect(irqfd);
    >>>>
    >>>>
    >>> Good. The fact that irqfd_disconnect() does a synchronize_srcu()
    >>> prevents any readers from trying to do an SRCU operation on an already
    >>> cleaned-up srcu_struct, so this does look safe to me.
    >>>
    >> As an additional data point, we can guarantee that we will never be
    >> called again since we synchronously unhook from the wqh and kvm->irqfds
    >> list, and the POLLHUP is called from f_ops->release().
    >>
    >
    > So the window is short, but still exists, correct?
    >

    Can you elaborate?


    >
    >>>> + cleanup_srcu_struct(&irqfd->srcu);
    >>>> + kfree(irqfd);
    >>>> + break;
    >>>> + }
    >>>>
    >>>> return 0;
    >>>> }
    >>>> @@ -84,8 +137,8 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
    >>>> add_wait_queue(wqh, &irqfd->wait);
    >>>> }
    >>>>
    >>>> -static int
    >>>> -kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
    >>>> +int
    >>>> +kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
    >>>> {
    >>>> struct _irqfd *irqfd;
    >>>> struct file *file = NULL;
    >>>> @@ -95,10 +148,12 @@ kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
    >>>> if (!irqfd)
    >>>> return -ENOMEM;
    >>>>
    >>>> + mutex_init(&irqfd->lock);
    >>>> + init_srcu_struct(&irqfd->srcu);
    >>>> irqfd->kvm = kvm;
    >>>> irqfd->gsi = gsi;
    >>>> INIT_LIST_HEAD(&irqfd->list);
    >>>> - INIT_WORK(&irqfd->work, irqfd_inject);
    >>>> + INIT_WORK(&irqfd->inject, irqfd_inject);
    >>>>
    >>>> /*
    >>>> * Embed the file* lifetime in the irqfd.
    >>>> @@ -120,12 +175,18 @@ kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
    >>>> if (ret < 0)
    >>>> goto fail;
    >>>>
    >>>> - irqfd->file = file;
    >>>> + kvm_get_kvm(kvm);
    >>>>
    >>>> mutex_lock(&kvm->lock);
    >>>> list_add_tail(&irqfd->list, &kvm->irqfds);
    >>>>
    >>>>
    >>> Doesn't the above need to be list_add_tail_rcu()? Unless I am confused,
    >>> this is the point at which the new SRCU-protected structure is first
    >>> made public. If so, you really do need list_add_tail_rcu() to make sure
    >>> that concurrent readers don't see pre-initialized values in the structure.
    >>>
    >> I *think* this is ok as a traditional list, because the only paths that
    >> touch this list are guarded by the kvm->lock mutex. Let me know if you
    >> see otherwise or if that is not enough.
    >>
    >
    > My mistake, you are using rcu_assign_pointer() and rcu_dereference()
    > instead of the list primitives. Never mind!!!
    >

    Yeah, and note that we actually have two types of objects and their
    references floating around:

    *) We have "struct irqfd" which can be thought of as an extension of
    eventfd. It holds exactly one (or zero) references to kvm via the
    irqfd->kvm pointer, and as you note above I use rcu_XX() macros and srcu
    to manage it.

    *) Conversely, we have "struct kvm" which may have a 1:N relationship
    with many irqfds, which I manage with a standard list at kvm->irqfds
    protected by kvm->lock.

    So the code that uses the rcu_dereference/rcu_assign_pointer is actually
    different than the code mentioned above that is manipulating the
    kvm->irqfds list with list_add_tail(). The latter isn't directly RCU
    related and is why you see the non-rcu variants of the list functions in
    use.

    That said, if you still see a hole in that approach, do not be shy in
    pointing it out ;)

    Thanks again for taking to time to go over all this, Paul. I know you
    are very busy, and its very much appreciated!

    -Greg

    >
    >>>> mutex_unlock(&kvm->lock);
    >>>>
    >>>> + /*
    >>>> + * do not drop the file until the irqfd is fully initialized, otherwise
    >>>> + * we might race against the POLLHUP
    >>>> + */
    >>>> + fput(file);
    >>>> +
    >>>> return 0;
    >>>>
    >>>> fail:
    >>>> @@ -139,97 +200,17 @@ fail:
    >>>> return ret;
    >>>> }
    >>>>
    >>>> -static void
    >>>> -irqfd_release(struct _irqfd *irqfd)
    >>>> -{
    >>>> - /*
    >>>> - * The ordering is important. We must remove ourselves from the wqh
    >>>> - * first to ensure no more event callbacks are issued, and then flush
    >>>> - * any previously scheduled work prior to freeing the memory
    >>>> - */
    >>>> - remove_wait_queue(irqfd->wqh, &irqfd->wait);
    >>>> -
    >>>> - flush_work(&irqfd->work);
    >>>> -
    >>>> - fput(irqfd->file);
    >>>> - kfree(irqfd);
    >>>> -}
    >>>> -
    >>>> -static struct _irqfd *
    >>>> -irqfd_remove(struct kvm *kvm, struct file *file, int gsi)
    >>>> -{
    >>>> - struct _irqfd *irqfd;
    >>>> -
    >>>> - mutex_lock(&kvm->lock);
    >>>> -
    >>>> - /*
    >>>> - * linear search isn't brilliant, but this should be an infrequent
    >>>> - * slow-path operation, and the list should not grow very large
    >>>> - */
    >>>> - list_for_each_entry(irqfd, &kvm->irqfds, list) {
    >>>> - if (irqfd->file != file || irqfd->gsi != gsi)
    >>>> - continue;
    >>>> -
    >>>> - list_del(&irqfd->list);
    >>>> - mutex_unlock(&kvm->lock);
    >>>> -
    >>>> - return irqfd;
    >>>> - }
    >>>> -
    >>>> - mutex_unlock(&kvm->lock);
    >>>> -
    >>>> - return NULL;
    >>>> -}
    >>>> -
    >>>> -static int
    >>>> -kvm_deassign_irqfd(struct kvm *kvm, int fd, int gsi)
    >>>> -{
    >>>> - struct _irqfd *irqfd;
    >>>> - struct file *file;
    >>>> - int count = 0;
    >>>> -
    >>>> - file = fget(fd);
    >>>> - if (IS_ERR(file))
    >>>> - return PTR_ERR(file);
    >>>> -
    >>>> - while ((irqfd = irqfd_remove(kvm, file, gsi))) {
    >>>> - /*
    >>>> - * We remove the item from the list under the lock, but we
    >>>> - * free it outside the lock to avoid deadlocking with the
    >>>> - * flush_work and the work_item taking the lock
    >>>> - */
    >>>> - irqfd_release(irqfd);
    >>>> - count++;
    >>>> - }
    >>>> -
    >>>> - fput(file);
    >>>> -
    >>>> - return count ? count : -ENOENT;
    >>>> -}
    >>>> -
    >>>> void
    >>>> kvm_irqfd_init(struct kvm *kvm)
    >>>> {
    >>>> INIT_LIST_HEAD(&kvm->irqfds);
    >>>> }
    >>>>
    >>>> -int
    >>>> -kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
    >>>> -{
    >>>> - if (flags & KVM_IRQFD_FLAG_DEASSIGN)
    >>>> - return kvm_deassign_irqfd(kvm, fd, gsi);
    >>>> -
    >>>> - return kvm_assign_irqfd(kvm, fd, gsi);
    >>>> -}
    >>>> -
    >>>> void
    >>>> kvm_irqfd_release(struct kvm *kvm)
    >>>> {
    >>>> struct _irqfd *irqfd, *tmp;
    >>>>
    >>>> - /* don't bother with the lock..we are shutting down */
    >>>> - list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
    >>>> - list_del(&irqfd->list);
    >>>> - irqfd_release(irqfd);
    >>>> - }
    >>>> + list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list)
    >>>> + irqfd_disconnect(irqfd);
    >>>> }
    >>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
    >>>> index 902fed9..a9f62bb 100644
    >>>> --- a/virt/kvm/kvm_main.c
    >>>> +++ b/virt/kvm/kvm_main.c
    >>>> @@ -1029,7 +1029,6 @@ static void kvm_destroy_vm(struct kvm *kvm)
    >>>> spin_lock(&kvm_lock);
    >>>> list_del(&kvm->vm_list);
    >>>> spin_unlock(&kvm_lock);
    >>>> - kvm_irqfd_release(kvm);
    >>>> kvm_free_irq_routing(kvm);
    >>>> kvm_io_bus_destroy(&kvm->pio_bus);
    >>>> kvm_io_bus_destroy(&kvm->mmio_bus);
    >>>> @@ -1064,6 +1063,8 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
    >>>> {
    >>>> struct kvm *kvm = filp->private_data;
    >>>>
    >>>> + kvm_irqfd_release(kvm);
    >>>> +
    >>>> kvm_put_kvm(kvm);
    >>>> return 0;
    >>>> }
    >>>>
    >>>>
    >>>>
    >>> --
    >>> To unsubscribe from this list: send the line "unsubscribe kvm" in
    >>> the body of a message to majordomo@vger.kernel.org
    >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
    >>>
    >>>
    >>
    >
    >
    >



    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2009-06-03 03:57    [W:0.073 / U:1.200 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site