On Wednesday 13 May 2009 06:09:08 Marcelo Tosatti wrote:
> 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 <alex.williamson@xxxxxx>
> From: Sheng Yang <sheng.yang@xxxxxxxxx>
> Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
Another calling path(kvm_free_all_assigned_devices()) don't hold kvm->lock...
Seems it need the lock for travel assigned dev list?
--
regards
Yang, Sheng
> 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);
--
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
