Re: device-assignment deadlock

Re: device-assignment deadlock

[Yang, Sheng]

[I was kicked off from the mailing list by periodic unknown reason last 
Friday... Sorry]

> Hi Sheng,
>
> 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]

> I wonder if we need finer granularity locking to avoid this.
> Suggestions?  Thanks,

This part again...

I think simply move kvm_deassign_irq() out of critical region is OK, and I 
also add the lock which seems missing in deassign_guest_irq(). Would post a 
patch soon.

-- 
regards
Yang, Sheng

[PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Sheng Yang]

kvm_vm_ioctl_deassign_dev_irq() would potentially recursively get kvm->lock,
because it called kvm_deassigned_irq() which implicit hold kvm->lock by calling
deassign_host_irq().

Fix it by move kvm_deassign_irq() out of critial region. And add the missing
lock for deassign_guest_irq().

Reported-by: Alex Williamson <alex.williamson@hp.com>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4d00942..3c69655 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -215,6 +215,8 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 static void deassign_guest_irq(struct kvm *kvm,
 			       struct kvm_assigned_dev_kernel *assigned_dev)
 {
+	mutex_lock(&kvm->lock);
+
 	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);
 	assigned_dev->ack_notifier.gsi = -1;
 
@@ -222,6 +224,8 @@ static void deassign_guest_irq(struct kvm *kvm,
 		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
 	assigned_dev->irq_source_id = -1;
 	assigned_dev->irq_requested_type &= ~(KVM_DEV_IRQ_GUEST_MASK);
+
+	mutex_unlock(&kvm->lock);
 }
 
 /* The function implicit hold kvm->lock mutex due to cancel_work_sync() */
@@ -558,20 +562,16 @@ static int kvm_vm_ioctl_deassign_dev_irq(struct kvm *kvm,
 					 struct kvm_assigned_irq
 					 *assigned_irq)
 {
-	int r = -ENODEV;
 	struct kvm_assigned_dev_kernel *match;
 
 	mutex_lock(&kvm->lock);
-
 	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
 				      assigned_irq->assigned_dev_id);
+	mutex_unlock(&kvm->lock);
 	if (!match)
-		goto out;
+		return -ENODEV;
 
-	r = kvm_deassign_irq(kvm, match, assigned_irq->flags);
-out:
-	mutex_unlock(&kvm->lock);
-	return r;
+	return kvm_deassign_irq(kvm, match, assigned_irq->flags);
 }
 
 static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
-- 
1.5.4.5

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Marcelo Tosatti]

On Tue, May 12, 2009 at 05:32:09PM +0800, Sheng Yang wrote:
> kvm_vm_ioctl_deassign_dev_irq() would potentially recursively get kvm->lock,
> because it called kvm_deassigned_irq() which implicit hold kvm->lock by calling
> deassign_host_irq().
> 
> Fix it by move kvm_deassign_irq() out of critial region. And add the missing
> lock for deassign_guest_irq().
> 
> Reported-by: Alex Williamson <alex.williamson@hp.com>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  virt/kvm/kvm_main.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 4d00942..3c69655 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -215,6 +215,8 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
>  static void deassign_guest_irq(struct kvm *kvm,
>  			       struct kvm_assigned_dev_kernel *assigned_dev)
>  {
> +	mutex_lock(&kvm->lock);
> +
>  	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);
>  	assigned_dev->ack_notifier.gsi = -1;
>  
> @@ -222,6 +224,8 @@ static void deassign_guest_irq(struct kvm *kvm,
>  		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
>  	assigned_dev->irq_source_id = -1;
>  	assigned_dev->irq_requested_type &= ~(KVM_DEV_IRQ_GUEST_MASK);
> +
> +	mutex_unlock(&kvm->lock);
>  }
>  
>  /* The function implicit hold kvm->lock mutex due to cancel_work_sync() */
> @@ -558,20 +562,16 @@ static int kvm_vm_ioctl_deassign_dev_irq(struct kvm *kvm,
>  					 struct kvm_assigned_irq
>  					 *assigned_irq)
>  {
> -	int r = -ENODEV;
>  	struct kvm_assigned_dev_kernel *match;
>  
>  	mutex_lock(&kvm->lock);
> -
>  	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
>  				      assigned_irq->assigned_dev_id);
> +	mutex_unlock(&kvm->lock);

assigned_dev list is protected by kvm->lock. So you could have another
ioctl adding to it at the same time you're searching.

Could either have a separate kvm->assigned_devs_lock, to protect
kvm->arch.assigned_dev_head (users are ioctls that manipulate it), or
change the IRQ injection to use a separate spinlock, kill the workqueue
and call kvm_set_irq from the assigned device interrupt handler.

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Yang, Sheng]

On Tuesday 12 May 2009 19:55:24 Marcelo Tosatti wrote:
> On Tue, May 12, 2009 at 05:32:09PM +0800, Sheng Yang wrote:
> > kvm_vm_ioctl_deassign_dev_irq() would potentially recursively get
> > kvm->lock, because it called kvm_deassigned_irq() which implicit hold
> > kvm->lock by calling deassign_host_irq().
> >
> > Fix it by move kvm_deassign_irq() out of critial region. And add the
> > missing lock for deassign_guest_irq().
> >
> > Reported-by: Alex Williamson <alex.williamson@hp.com>
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  virt/kvm/kvm_main.c |   14 +++++++-------
> >  1 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 4d00942..3c69655 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -215,6 +215,8 @@ static void kvm_assigned_dev_ack_irq(struct
> > kvm_irq_ack_notifier *kian) static void deassign_guest_irq(struct kvm
> > *kvm,
> >  			       struct kvm_assigned_dev_kernel *assigned_dev)
> >  {
> > +	mutex_lock(&kvm->lock);
> > +
> >  	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);
> >  	assigned_dev->ack_notifier.gsi = -1;
> >
> > @@ -222,6 +224,8 @@ static void deassign_guest_irq(struct kvm *kvm,
> >  		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
> >  	assigned_dev->irq_source_id = -1;
> >  	assigned_dev->irq_requested_type &= ~(KVM_DEV_IRQ_GUEST_MASK);
> > +
> > +	mutex_unlock(&kvm->lock);
> >  }
> >
> >  /* The function implicit hold kvm->lock mutex due to cancel_work_sync()
> > */ @@ -558,20 +562,16 @@ static int kvm_vm_ioctl_deassign_dev_irq(struct
> > kvm *kvm, struct kvm_assigned_irq
> >  					 *assigned_irq)
> >  {
> > -	int r = -ENODEV;
> >  	struct kvm_assigned_dev_kernel *match;
> >
> >  	mutex_lock(&kvm->lock);
> > -
> >  	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
> >  				      assigned_irq->assigned_dev_id);
> > +	mutex_unlock(&kvm->lock);
>
> assigned_dev list is protected by kvm->lock. So you could have another
> ioctl adding to it at the same time you're searching.

Oh, yes... My fault... 

> Could either have a separate kvm->assigned_devs_lock, to protect
> kvm->arch.assigned_dev_head (users are ioctls that manipulate it), or
> change the IRQ injection to use a separate spinlock, kill the workqueue
> and call kvm_set_irq from the assigned device interrupt handler.

Peferred the latter, though needs more work. But the only reason for put a 
workqueue here is because kvm->lock is a mutex? I can't believe... If so, I 
think we had made a big mistake - we have to fix all kinds of racy problem 
caused by this, but finally find it's unnecessary... 

Maybe another reason is kvm_kick_vcpu(), but have already fixed by you.

Continue to check the code...

-- 
regards
Yang, Sheng

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Marcelo Tosatti]

On Tue, May 12, 2009 at 10:13:36PM +0800, Yang, Sheng wrote:
> > > +	mutex_unlock(&kvm->lock);
> >
> > assigned_dev list is protected by kvm->lock. So you could have another
> > ioctl adding to it at the same time you're searching.
> 
> Oh, yes... My fault... 
> 
> > Could either have a separate kvm->assigned_devs_lock, to protect
> > kvm->arch.assigned_dev_head (users are ioctls that manipulate it), or
> > change the IRQ injection to use a separate spinlock, kill the workqueue
> > and call kvm_set_irq from the assigned device interrupt handler.
> 
> Peferred the latter, though needs more work. But the only reason for put a 
> workqueue here is because kvm->lock is a mutex? I can't believe... If so, I 
> think we had made a big mistake - we have to fix all kinds of racy problem 
> caused by this, but finally find it's unnecessary... 

One issue is that kvm_set_irq can take too long while interrupts are
blocked, and you'd have to disable interrupts in other contexes that
inject interrupts (say qemu->ioctl(SET_INTERRUPT)->...->), so all i can
see is a tradeoff.

<guess mode on>

But the interrupt injection path seems to be pretty short and efficient
to happen in host interrupt context.

<guess mode off>

Avi, Gleb?

> Maybe another reason is kvm_kick_vcpu(), but have already fixed by you.

Note you tested the spinlock_irq patch with GigE and there was no
significant performance regression right?

> 
> Continue to check the code...
> 
> -- 
> regards
> Yang, Sheng

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Marcelo Tosatti]

On Tue, May 12, 2009 at 11:30:21AM -0300, Marcelo Tosatti wrote:
> On Tue, May 12, 2009 at 10:13:36PM +0800, Yang, Sheng wrote:
> > > > +	mutex_unlock(&kvm->lock);
> > >
> > > assigned_dev list is protected by kvm->lock. So you could have another
> > > ioctl adding to it at the same time you're searching.
> > 
> > Oh, yes... My fault... 
> > 
> > > Could either have a separate kvm->assigned_devs_lock, to protect
> > > kvm->arch.assigned_dev_head (users are ioctls that manipulate it), or
> > > change the IRQ injection to use a separate spinlock, kill the workqueue
> > > and call kvm_set_irq from the assigned device interrupt handler.
> > 
> > Peferred the latter, though needs more work. But the only reason for put a 
> > workqueue here is because kvm->lock is a mutex? I can't believe... If so, I 
> > think we had made a big mistake - we have to fix all kinds of racy problem 
> > caused by this, but finally find it's unnecessary... 
> 
> One issue is that kvm_set_irq can take too long while interrupts are
> blocked, and you'd have to disable interrupts in other contexes that
> inject interrupts (say qemu->ioctl(SET_INTERRUPT)->...->), so all i can
> see is a tradeoff.

Or multiple kvm_set_irq calls for MSI.

> 
> <guess mode on>
> 
> But the interrupt injection path seems to be pretty short and efficient
> to happen in host interrupt context.
> 
> <guess mode off>
> 
> Avi, Gleb?
> 
> > Maybe another reason is kvm_kick_vcpu(), but have already fixed by you.
> 
> Note you tested the spinlock_irq patch with GigE and there was no
> significant performance regression right?
> 
> > 
> > Continue to check the code...
> > 
> > -- 
> > regards
> > Yang, Sheng
> --
> 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

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Marcelo Tosatti]

On Tue, May 12, 2009 at 11:30:21AM -0300, Marcelo Tosatti wrote:
> On Tue, May 12, 2009 at 10:13:36PM +0800, Yang, Sheng wrote:
> > > > +	mutex_unlock(&kvm->lock);
> > >
> > > assigned_dev list is protected by kvm->lock. So you could have another
> > > ioctl adding to it at the same time you're searching.
> > 
> > Oh, yes... My fault... 
> > 
> > > Could either have a separate kvm->assigned_devs_lock, to protect
> > > kvm->arch.assigned_dev_head (users are ioctls that manipulate it), or
> > > change the IRQ injection to use a separate spinlock, kill the workqueue
> > > and call kvm_set_irq from the assigned device interrupt handler.
> > 
> > Peferred the latter, though needs more work. But the only reason for put a 
> > workqueue here is because kvm->lock is a mutex? I can't believe... If so, I 
> > think we had made a big mistake - we have to fix all kinds of racy problem 
> > caused by this, but finally find it's unnecessary... 
> 
> One issue is that kvm_set_irq can take too long while interrupts are
> blocked, and you'd have to disable interrupts in other contexes that
> inject interrupts (say qemu->ioctl(SET_INTERRUPT)->...->), so all i can
> see is a tradeoff.
> 
> <guess mode on>
> 
> But the interrupt injection path seems to be pretty short and efficient
> to happen in host interrupt context.
> 
> <guess mode off>
> 
> Avi, Gleb?
> 
> > Maybe another reason is kvm_kick_vcpu(), but have already fixed by you.
> 
> Note you tested the spinlock_irq patch with GigE and there was no
> significant performance regression right?
> 
> > 
> > Continue to check the code...

OK, it might take some time for bigger changes to happen. I've changed
your patch to drop the lock only around cancel_work_sync. Can deadlock
if someone else tries to mess with the assigned device at the same time,
but the VM won't go away under it because of the vmfd reference.

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);
 
 		for (i = 0; i < assigned_dev->entries_nr; i++)
 			free_irq(assigned_dev->host_msix_entries[i].vector,

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Alex Williamson]

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,

Alex

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]

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@hp.com>
From: Sheng Yang <sheng.yang@intel.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


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);
 

[-- Attachment #2: assigned-dev-cancel-work-deadlock.patch --]
[-- Type: text/plain, Size: 1070 bytes --]

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);
 

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Alex Williamson]

On Tue, 2009-05-12 at 19:09 -0300, Marcelo Tosatti wrote:
> 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@hp.com>
> From: Sheng Yang <sheng.yang@intel.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Perfect, thanks.

Acked-by: Alex Williamson <alex.williamson@hp.com>

> 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);
>  

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Yang, Sheng]

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@hp.com>
> From: Sheng Yang <sheng.yang@intel.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

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);

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Gleb Natapov]

On Tue, May 12, 2009 at 11:30:21AM -0300, Marcelo Tosatti wrote:
> On Tue, May 12, 2009 at 10:13:36PM +0800, Yang, Sheng wrote:
> > > > +	mutex_unlock(&kvm->lock);
> > >
> > > assigned_dev list is protected by kvm->lock. So you could have another
> > > ioctl adding to it at the same time you're searching.
> > 
> > Oh, yes... My fault... 
> > 
> > > Could either have a separate kvm->assigned_devs_lock, to protect
> > > kvm->arch.assigned_dev_head (users are ioctls that manipulate it), or
> > > change the IRQ injection to use a separate spinlock, kill the workqueue
> > > and call kvm_set_irq from the assigned device interrupt handler.
> > 
> > Peferred the latter, though needs more work. But the only reason for put a 
> > workqueue here is because kvm->lock is a mutex? I can't believe... If so, I 
> > think we had made a big mistake - we have to fix all kinds of racy problem 
> > caused by this, but finally find it's unnecessary... 
> 
> One issue is that kvm_set_irq can take too long while interrupts are
> blocked, and you'd have to disable interrupts in other contexes that
> inject interrupts (say qemu->ioctl(SET_INTERRUPT)->...->), so all i can
> see is a tradeoff.
> 
> <guess mode on>
> 
> But the interrupt injection path seems to be pretty short and efficient
> to happen in host interrupt context.
> 
> <guess mode off>
> 
> Avi, Gleb?
> 
Interrupt injection path also use IRQ routing data structures so access
to them should be protected by the same lock. And of cause in kernel
device (apic/ioapic/pic) mmio is done holding this lock so interrupt
injection cannot happen in parallel with device reconfiguration. May
be we want more parallelism here.

> > Maybe another reason is kvm_kick_vcpu(), but have already fixed by you.
> 
> Note you tested the spinlock_irq patch with GigE and there was no
> significant performance regression right?
> 
> > 
> > Continue to check the code...
> > 
> > -- 
> > regards
> > Yang, Sheng
> --
> 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

--
			Gleb.

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Marcelo Tosatti]

On Wed, May 13, 2009 at 10:07:54AM +0800, Yang, Sheng wrote:
> > 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@hp.com>
> > From: Sheng Yang <sheng.yang@intel.com>
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Another calling path(kvm_free_all_assigned_devices()) don't hold kvm->lock... 
> Seems it need the lock for travel assigned dev list?

Sheng,

The task executing the deassign irq ioctl has a reference to the vm
instance. This solution is just temporary though until the locks can be
split and then dropping kvm->lock around cancel_work_sync will not be
necessary anymore.

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Chris Wright]

* Alex Williamson (alex.williamson@hp.com) wrote:
> On Tue, 2009-05-12 at 19:09 -0300, Marcelo Tosatti wrote:
> > 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@hp.com>
> > From: Sheng Yang <sheng.yang@intel.com>
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Perfect, thanks.
> 
> Acked-by: Alex Williamson <alex.williamson@hp.com>

Is this still pending?

thanks,
-chris

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Alex Williamson]

On Fri, 2009-05-22 at 08:06 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@hp.com) wrote:
> > On Tue, 2009-05-12 at 19:09 -0300, Marcelo Tosatti wrote:
> > > 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().
> 
> Is this still pending?

I haven't seen this particular workaround make it into a tree, however
Marcelo has been working on a set of patches to properly fix this.  Most
recent version was sent on 5/20.

Alex

Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

[Chris Wright]

* Alex Williamson (alex.williamson@hp.com) wrote:
> On Fri, 2009-05-22 at 08:06 -0700, Chris Wright wrote:
> > * Alex Williamson (alex.williamson@hp.com) wrote:
> > > On Tue, 2009-05-12 at 19:09 -0300, Marcelo Tosatti wrote:
> > > > 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().
> > 
> > Is this still pending?
> 
> I haven't seen this particular workaround make it into a tree, however
> Marcelo has been working on a set of patches to properly fix this.  Most
> recent version was sent on 5/20.

Great, thanks.
-chris