From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock Date: Tue, 12 May 2009 19:09:08 -0300 Message-ID: <20090512220908.GA22626@amt.cnet> References: <200905121705.53176.sheng.yang@intel.com> <1242120729-2280-1-git-send-email-sheng@linux.intel.com> <20090512115524.GB10901@amt.cnet> <200905122213.36833.sheng.yang@intel.com> <20090512143021.GB12888@amt.cnet> <20090512194432.GA19969@amt.cnet> <1242164187.4788.4.camel@2710p.home> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="IrhDeMKUP4DT/M7F" Cc: "Yang, Sheng" , Avi Kivity , kvm@vger.kernel.org To: Alex Williamson Return-path: Received: from mx2.redhat.com ([66.187.237.31]:45596 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754829AbZELWNt (ORCPT ); Tue, 12 May 2009 18:13:49 -0400 Content-Disposition: inline In-Reply-To: <1242164187.4788.4.camel@2710p.home> Sender: kvm-owner@vger.kernel.org List-ID: --IrhDeMKUP4DT/M7F Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, May 12, 2009 at 03:36:27PM -0600, Alex Williamson wrote: > On Tue, 2009-05-12 at 16:44 -0300, Marcelo Tosatti wrote: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 4d00942..ba067db 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm, > > disable_irq_nosync(assigned_dev-> > > host_msix_entries[i].vector); > > > > + /* > > + * FIXME: kvm_assigned_dev_interrupt_work_handler can deadlock > > + * with cancel_work_sync, since it requires kvm->lock for irq > > + * injection. This is a hack, the irq code must use > > + * a separate lock. > > + */ > > + mutex_unlock(&kvm->lock); > > cancel_work_sync(&assigned_dev->interrupt_work); > > + mutex_lock(&kvm->lock); > > Seems to work, I assume you've got a similar unlock/lock for the > MSI/INTx block. Thanks, KVM: workaround workqueue / deassign_host_irq deadlock I think I'm running into the following deadlock in the kvm kernel module when trying to use device assignment: CPU A CPU B kvm_vm_ioctl_deassign_dev_irq() mutex_lock(&kvm->lock); worker_thread() -> kvm_deassign_irq() -> kvm_assigned_dev_interrupt_work_handler() -> deassign_host_irq() mutex_lock(&kvm->lock); -> cancel_work_sync() [blocked] Workaround the issue by dropping kvm->lock for cancel_work_sync(). Reported-by: Alex Williamson From: Sheng Yang Signed-off-by: Marcelo Tosatti diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4d00942..d4af719 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm, disable_irq_nosync(assigned_dev-> host_msix_entries[i].vector); + /* + * FIXME: kvm_assigned_dev_interrupt_work_handler can deadlock + * with cancel_work_sync, since it requires kvm->lock for irq + * injection. This is a hack, the irq code must use + * a separate lock. Same below for MSI. + */ + mutex_unlock(&kvm->lock); cancel_work_sync(&assigned_dev->interrupt_work); + mutex_lock(&kvm->lock); for (i = 0; i < assigned_dev->entries_nr; i++) free_irq(assigned_dev->host_msix_entries[i].vector, @@ -263,7 +271,9 @@ static void deassign_host_irq(struct kvm *kvm, } else { /* Deal with MSI and INTx */ disable_irq_nosync(assigned_dev->host_irq); + mutex_unlock(&kvm->lock); cancel_work_sync(&assigned_dev->interrupt_work); + mutex_lock(&kvm->lock); free_irq(assigned_dev->host_irq, (void *)assigned_dev); --IrhDeMKUP4DT/M7F Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="assigned-dev-cancel-work-deadlock.patch" diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4d00942..d4af719 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm, disable_irq_nosync(assigned_dev-> host_msix_entries[i].vector); + /* + * FIXME: kvm_assigned_dev_interrupt_work_handler can deadlock + * with cancel_work_sync, since it requires kvm->lock for irq + * injection. This is a hack, the irq code must use + * a separate lock. Same below for MSI. + */ + mutex_unlock(&kvm->lock); cancel_work_sync(&assigned_dev->interrupt_work); + mutex_lock(&kvm->lock); for (i = 0; i < assigned_dev->entries_nr; i++) free_irq(assigned_dev->host_msix_entries[i].vector, @@ -263,7 +271,9 @@ static void deassign_host_irq(struct kvm *kvm, } else { /* Deal with MSI and INTx */ disable_irq_nosync(assigned_dev->host_irq); + mutex_unlock(&kvm->lock); cancel_work_sync(&assigned_dev->interrupt_work); + mutex_lock(&kvm->lock); free_irq(assigned_dev->host_irq, (void *)assigned_dev); --IrhDeMKUP4DT/M7F--