From: Christian Borntraeger <borntraeger@de.ibm.com>
To: Cornelia Huck <cornelia.huck@de.ibm.com>,
kvm@vger.kernel.org, linux-s390@vger.kernel.org,
qemu-devel@nongnu.org
Cc: agraf@suse.de, pbonzini@redhat.com, gleb@kernel.org
Subject: Re: [PATCH v2 1/5] KVM: eventfd: Fix lock order inversion.
Date: Mon, 17 Mar 2014 22:55:49 +0100 [thread overview]
Message-ID: <53276F65.4070501@de.ibm.com> (raw)
In-Reply-To: <1395079899-29239-2-git-send-email-cornelia.huck@de.ibm.com>
On 17/03/14 19:11, Cornelia Huck wrote:
> When registering a new irqfd, we call its ->poll method to collect any
> event that might have previously been pending so that we can trigger it.
> This is done under the kvm->irqfds.lock, which means the eventfd's ctx
> lock is taken under it.
>
> However, if we get a POLLHUP in irqfd_wakeup, we will be called with the
> ctx lock held before getting the irqfds.lock to deactivate the irqfd,
> causing lockdep to complain.
>
> Calling the ->poll method does not really need the irqfds.lock, so let's
> just move it after we've given up the irqfds.lock in kvm_irqfd_assign().
>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Do you still have the lockdep message somewhere?
Looking at the patch and the description this makes sense. Even without
irqfd for s390:
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Paolo, maybe this patch can go in independently from s390?
Christian
> ---
> virt/kvm/eventfd.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index abe4d60..29c2a04 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -391,19 +391,19 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
> lockdep_is_held(&kvm->irqfds.lock));
> irqfd_update(kvm, irqfd, irq_rt);
>
> - events = f.file->f_op->poll(f.file, &irqfd->pt);
> -
> list_add_tail(&irqfd->list, &kvm->irqfds.items);
>
> + spin_unlock_irq(&kvm->irqfds.lock);
> +
> /*
> * Check if there was an event already pending on the eventfd
> * before we registered, and trigger it as if we didn't miss it.
> */
> + events = f.file->f_op->poll(f.file, &irqfd->pt);
> +
> if (events & POLLIN)
> schedule_work(&irqfd->inject);
>
> - spin_unlock_irq(&kvm->irqfds.lock);
> -
> /*
> * do not drop the file until the irqfd is fully initialized, otherwise
> * we might race against the POLLHUP
>
WARNING: multiple messages have this Message-ID (diff)
From: Christian Borntraeger <borntraeger@de.ibm.com>
To: Cornelia Huck <cornelia.huck@de.ibm.com>,
kvm@vger.kernel.org, linux-s390@vger.kernel.org,
qemu-devel@nongnu.org
Cc: gleb@kernel.org, pbonzini@redhat.com, agraf@suse.de
Subject: Re: [Qemu-devel] [PATCH v2 1/5] KVM: eventfd: Fix lock order inversion.
Date: Mon, 17 Mar 2014 22:55:49 +0100 [thread overview]
Message-ID: <53276F65.4070501@de.ibm.com> (raw)
In-Reply-To: <1395079899-29239-2-git-send-email-cornelia.huck@de.ibm.com>
On 17/03/14 19:11, Cornelia Huck wrote:
> When registering a new irqfd, we call its ->poll method to collect any
> event that might have previously been pending so that we can trigger it.
> This is done under the kvm->irqfds.lock, which means the eventfd's ctx
> lock is taken under it.
>
> However, if we get a POLLHUP in irqfd_wakeup, we will be called with the
> ctx lock held before getting the irqfds.lock to deactivate the irqfd,
> causing lockdep to complain.
>
> Calling the ->poll method does not really need the irqfds.lock, so let's
> just move it after we've given up the irqfds.lock in kvm_irqfd_assign().
>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Do you still have the lockdep message somewhere?
Looking at the patch and the description this makes sense. Even without
irqfd for s390:
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Paolo, maybe this patch can go in independently from s390?
Christian
> ---
> virt/kvm/eventfd.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index abe4d60..29c2a04 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -391,19 +391,19 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
> lockdep_is_held(&kvm->irqfds.lock));
> irqfd_update(kvm, irqfd, irq_rt);
>
> - events = f.file->f_op->poll(f.file, &irqfd->pt);
> -
> list_add_tail(&irqfd->list, &kvm->irqfds.items);
>
> + spin_unlock_irq(&kvm->irqfds.lock);
> +
> /*
> * Check if there was an event already pending on the eventfd
> * before we registered, and trigger it as if we didn't miss it.
> */
> + events = f.file->f_op->poll(f.file, &irqfd->pt);
> +
> if (events & POLLIN)
> schedule_work(&irqfd->inject);
>
> - spin_unlock_irq(&kvm->irqfds.lock);
> -
> /*
> * do not drop the file until the irqfd is fully initialized, otherwise
> * we might race against the POLLHUP
>
next prev parent reply other threads:[~2014-03-17 21:55 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-17 18:11 [PATCH v2 0/5] KVM: irqfds for s390 Cornelia Huck
2014-03-17 18:11 ` [Qemu-devel] " Cornelia Huck
2014-03-17 18:11 ` [PATCH v2 1/5] KVM: eventfd: Fix lock order inversion Cornelia Huck
2014-03-17 18:11 ` [Qemu-devel] " Cornelia Huck
2014-03-17 21:55 ` Christian Borntraeger [this message]
2014-03-17 21:55 ` Christian Borntraeger
2014-03-18 9:18 ` Cornelia Huck
2014-03-18 9:18 ` [Qemu-devel] " Cornelia Huck
2014-03-18 16:05 ` Paolo Bonzini
2014-03-18 16:05 ` [Qemu-devel] " Paolo Bonzini
2014-03-17 18:11 ` [PATCH v2 2/5] KVM: Add per-vm capability enablement Cornelia Huck
2014-03-17 18:11 ` [Qemu-devel] " Cornelia Huck
2014-03-17 22:09 ` Christian Borntraeger
2014-03-17 22:09 ` [Qemu-devel] " Christian Borntraeger
2014-03-21 9:12 ` Christian Borntraeger
2014-03-21 9:12 ` [Qemu-devel] " Christian Borntraeger
2014-03-17 18:11 ` [PATCH v2 3/5] KVM: s390: adapter interrupt sources Cornelia Huck
2014-03-17 18:11 ` [Qemu-devel] " Cornelia Huck
2014-03-18 8:11 ` Heiko Carstens
2014-03-18 8:11 ` [Qemu-devel] " Heiko Carstens
2014-03-18 8:41 ` Cornelia Huck
2014-03-18 8:41 ` [Qemu-devel] " Cornelia Huck
2014-03-21 9:26 ` Christian Borntraeger
2014-03-21 9:26 ` [Qemu-devel] " Christian Borntraeger
2014-03-21 10:07 ` Cornelia Huck
2014-03-21 10:07 ` [Qemu-devel] " Cornelia Huck
2014-03-17 18:11 ` [PATCH v2 4/5] KVM: s390: irq routing for adapter interrupts Cornelia Huck
2014-03-17 18:11 ` [Qemu-devel] " Cornelia Huck
2014-03-21 9:32 ` Christian Borntraeger
2014-03-21 9:32 ` [Qemu-devel] " Christian Borntraeger
2014-03-21 10:08 ` Cornelia Huck
2014-03-21 10:08 ` [Qemu-devel] " Cornelia Huck
2014-03-17 18:11 ` [PATCH v2 5/5] KVM: Bump KVM_MAX_IRQ_ROUTES for s390 Cornelia Huck
2014-03-17 18:11 ` [Qemu-devel] " Cornelia Huck
2014-03-18 16:07 ` Paolo Bonzini
2014-03-18 16:07 ` [Qemu-devel] " Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53276F65.4070501@de.ibm.com \
--to=borntraeger@de.ibm.com \
--cc=agraf@suse.de \
--cc=cornelia.huck@de.ibm.com \
--cc=gleb@kernel.org \
--cc=kvm@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.