device-assignment deadlock

PATCH In review Yang, Sheng 6 participants 16 messages May 12, 2009 – May 22, 2009 KVM

lore

Share & tools

lore

Yang, Sheng

author

2009-05-12 09:05 UTC #0 lore raw

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

Quoted 10 lines

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

Quoted 5 lines

   -> 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.

Signature · 2 lines

regards
Yang, Sheng

Sheng Yang

2009-05-12 09:31 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/kvm_main.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

virt/kvm/kvm_main.c +7 -7

index 4d00942..3c69655 100644
@@ -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,

Signature · 1 line

1.5.4.5

Marcelo Tosatti

2009-05-12 11:55 UTC #2 lore raw

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

Quoted 37 lines · 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 <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/kvm_main.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

virt/kvm/kvm_main.c +4 -0

index 4d00942..3c69655 100644
@@ -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

virt/kvm/kvm_main.c static int kvm_vm_ioctl_deassign_dev_irq(struct kvm *kvm, · line 570

Quoted 10 lines

virt/kvm/kvm_main.c +1 -2

 					 *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.

Yang, Sheng

author

2009-05-12 14:12 UTC #3 lore raw

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

Quoted 44 lines · On Tuesday 12 May 2009 19:55:24 Marcelo Tosatti wrote:

Quoted 43 lines · 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 <[email protected]>
Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
virt/kvm/kvm_main.c +0 -0
index 4d00942..3c69655 100644
@@ -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,
virt/kvm/kvm_main.c +4 -0
 			       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
virt/kvm/kvm_main.c +0 -1
 					 *assigned_irq)
 {
-	int r = -ENODEV;
 	struct kvm_assigned_dev_kernel *match;

Quoted 8 lines

virt/kvm/kvm_main.c static int kvm_vm_ioctl_deassign_dev_irq(struct kvm *kvm, · line 570
Quoted 5 lines
virt/kvm/kvm_main.c +1 -1
 	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...

Quoted 4 lines

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...

Signature · 2 lines

regards
Yang, Sheng

Marcelo Tosatti

2009-05-12 14:30 UTC #4 lore raw

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

Quoted 12 lines · Sheng

Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	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...
Quoted 4 lines
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.

Quoted 4 lines

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.

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

Avi, Gleb?

Quoted 1 line

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?

Quoted 6 lines

Continue to check the code...
Signature · 2 lines
regards
Yang, Sheng

Marcelo Tosatti

2009-05-12 19:05 UTC #5 lore raw

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

Quoted 18 lines · Marcelo Tosatti

Quoted 12 lines · Sheng
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	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...
Quoted 4 lines
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.
Quoted 4 lines
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...

Quoted 4 lines

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.

Quoted 25 lines

<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?
Quoted 1 line
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?
Quoted 6 lines
Continue to check the code...
Signature · 2 lines
regards
Yang, Sheng
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Marcelo Tosatti

2009-05-12 19:45 UTC #6 lore raw

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

In reply to Marcelo Tosatti

Quoted 28 lines · Marcelo Tosatti

Quoted 12 lines · Sheng
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	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...
Quoted 4 lines
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.
Quoted 4 lines
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.

Quoted 11 lines

<guess mode off>
Avi, Gleb?
Quoted 1 line
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?
Quoted 2 lines
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.

virt/kvm/kvm_main.c +8 -0

index 4d00942..ba067db 100644
@@ -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,

Alex Williamson

2009-05-12 21:36 UTC #7 lore raw

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

Quoted 7 lines · Marcelo Tosatti

virt/kvm/kvm_main.c +0 -0

index 4d00942..ba067db 100644
@@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm,
 			disable_irq_nosync(assigned_dev->
 					   host_msix_entries[i].vector);

virt/kvm/kvm_main.c static void deassign_host_irq(struct kvm *kvm, · line 261

Quoted 10 lines

virt/kvm/kvm_main.c +8 -0

 
+		/*
+		 * 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

Marcelo Tosatti

2009-05-12 22:13 UTC #8 lore raw

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

Quoted 9 lines · Alex Williamson

Quoted 8 lines · Marcelo Tosatti

virt/kvm/kvm_main.c +0 -0

index 4d00942..ba067db 100644
@@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm,
 			disable_irq_nosync(assigned_dev->
 					   host_msix_entries[i].vector);

Quoted 12 lines

virt/kvm/kvm_main.c static void deassign_host_irq(struct kvm *kvm, · line 261
Quoted 9 lines
virt/kvm/kvm_main.c +8 -0
+		/*
+		 * 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 <[email protected]>

From: Sheng Yang <[email protected]>

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/kvm_main.c +10 -0

index 4d00942..d4af719 100644
@@ -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);

virt/kvm/kvm_main.c +10 -0

index 4d00942..d4af719 100644
@@ -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);

Alex Williamson

Acked-by

2009-05-12 22:17 UTC #9 lore raw

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

Quoted 10 lines · Marcelo Tosatti

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

Quoted 8 lines

    -> 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 <[email protected]>
From: Sheng Yang <[email protected]>
Signed-off-by Marcelo Tosatti <[email protected]>

Perfect, thanks.

Acked-by Alex Williamson <[email protected]>

Quoted 30 lines

virt/kvm/kvm_main.c +10 -0

index 4d00942..d4af719 100644
@@ -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);

Yang, Sheng

author

2009-05-13 02:07 UTC #13 lore raw

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

In reply to Marcelo Tosatti

Quoted 33 lines · On Wednesday 13 May 2009 06:09:08 Marcelo Tosatti wrote:

Quoted 9 lines · Alex Williamson
Quoted 8 lines · Marcelo Tosatti
virt/kvm/kvm_main.c +0 -0
index 4d00942..ba067db 100644
@@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm,
 			disable_irq_nosync(assigned_dev->
 					   host_msix_entries[i].vector);
 
Quoted 12 lines
virt/kvm/kvm_main.c static void deassign_host_irq(struct kvm *kvm, · line 261
Quoted 9 lines
virt/kvm/kvm_main.c +8 -0
+		/*
+		 * 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()

Quoted 8 lines

    -> 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 <[email protected]>
From: Sheng Yang <[email protected]>
Signed-off-by Marcelo Tosatti <[email protected]>

Another calling path(kvm_free_all_assigned_devices()) don't hold kvm->lock... Seems it need the lock for travel assigned dev list?

Signature · 2 lines

regards
Yang, Sheng

Quoted 29 lines

virt/kvm/kvm_main.c +10 -0

index 4d00942..d4af719 100644
@@ -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);

Gleb Natapov

2009-05-13 11:43 UTC #15 lore raw

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

In reply to Marcelo Tosatti

Quoted 23 lines · Marcelo Tosatti

Quoted 12 lines · Sheng
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	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...
Quoted 4 lines
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.
Quoted 4 lines
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.

Quoted 9 lines

<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.

Quoted 8 lines

Quoted 1 line
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?
Quoted 3 lines
Continue to check the code...

Quoted 7 lines

Quoted 3 lines
Signature · 2 lines
regards
Yang, Sheng
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

Marcelo Tosatti

2009-05-13 13:14 UTC #14 lore raw

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

In reply to Yang, Sheng

Quoted 13 lines · Sheng

Quoted 13 lines
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]

Quoted 8 lines

Quoted 5 lines
Workaround the issue by dropping kvm->lock for cancel_work_sync().
Reported-by Alex Williamson <[email protected]>
From: Sheng Yang <[email protected]>
Signed-off-by Marcelo Tosatti <[email protected]>
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.

Chris Wright

2009-05-22 15:06 UTC #10 lore raw

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

In reply to Alex Williamson

Quoted 16 lines · * Alex Williamson ([email protected]) wrote:

Quoted 15 lines · Marcelo Tosatti
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().

Quoted 7 lines

Quoted 3 lines
Reported-by Alex Williamson <[email protected]>
From: Sheng Yang <[email protected]>
Signed-off-by Marcelo Tosatti <[email protected]>
Perfect, thanks.
Acked-by Alex Williamson <[email protected]>

Is this still pending?

thanks,
-chris

Alex Williamson

2009-05-22 15:34 UTC #11 lore raw

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

Quoted 15 lines · Chris Wright

Quoted 14 lines · * Alex Williamson ([email protected]) wrote:
Quoted 13 lines · Marcelo Tosatti
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]

Quoted 3 lines

Quoted 1 line
Quoted 1 line
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

Chris Wright

2009-05-22 15:36 UTC #12 lore raw

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

Quoted 16 lines · * Alex Williamson ([email protected]) wrote:

Quoted 15 lines · Chris Wright
Quoted 14 lines · * Alex Williamson ([email protected]) wrote:
Quoted 13 lines · Marcelo Tosatti
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]

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Quoted 1 line
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