Re: paravirt clock stil causing hangs in kvm-65

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Apr 07, 2008 at 06:34:57PM -0300, Marcelo Tosatti wrote:
> On Mon, Apr 07, 2008 at 01:53:36PM +0200, Nikola Ciprich wrote:
> > Hi,
> > 
> > I also tried paravirt clock again in latest git with kvm-65 patch 
> > applied, and problem with cpu-lockups persists:
> > 
> > [10813.654806] BUG: soft lockup - CPU#0 stuck for 61s! [swapper:0]
> > [10813.655789] CPU 0:
> > [10813.656624] Modules linked in: virtio_pci virtio_ring virtio_blk virtio 
> > piix dm_snapshot dm_zero dm_mirror dm_mod ide_disk
> >   ide_core sd_mod scsi_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd
> > [10813.658805] Pid: 0, comm: swapper Not tainted 2.6.25-rc7 #5
> > [10813.658805] RIP: 0010:[<ffffffff80222ab2>]  [<ffffffff80222ab2>] 
> > native_safe_halt+0x2/0x10
> > [10813.658805] RSP: 0018:ffffffff805adf50  EFLAGS: 00000296
> > [10813.658805] RAX: 000000019b08eeb0 RBX: ffffffff805f5000 RCX: 
> > 000000019b08eeb0
> > [10813.658805] RDX: 0000000000000006 RSI: 00000000356832b0 RDI: 
> > ffffffff805adf38
> > [10813.658805] RBP: 0000000000000da8 R08: 0000000000000000 R09: 
> > 0000000000000000
> > [10813.658805] R10: 0000000000000001 R11: 0000000000000002 R12: 
> > ffffffff802228ed
> > [10813.658805] R13: 00000000000132a0 R14: ffffffff80200bba R15: 
> > ffff81000100a280
> > [10813.658805] FS:  0000000000000000(0000) GS:ffffffff80576000(0000) 
> > knlGS:0000000000000000
> > [10813.658805] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> > [10813.658805] CR2: 00007fac0f852000 CR3: 0000000000201000 CR4: 
> > 00000000000006e0
> > [10813.658805] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
> > 0000000000000000
> > [10813.658805] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 
> > 0000000000000400
> > [10813.658805]
> > [10813.658805] Call Trace:
> > [10813.658805]  [<ffffffff8020a55b>] ? default_idle+0x3b/0x70
> > [10813.658805]  [<ffffffff8020a520>] ? default_idle+0x0/0x70
> > [10813.658805]  [<ffffffff8020a60e>] ? cpu_idle+0x7e/0xe0
> > [10813.658805]  [<ffffffff80211630>] ? pda_init+0x30/0xb0
> > 
> > Can I somehow help to track this one down??
> 
> Hi Nikola,
> 
> I just reproduced this on a UP guest. Were you seeing the exact same
> stack trace in the guest with kvm-64 ?

I think the logic to wakeup tasks in HLT is racy. Nothing prevents
a timer event from being lost if it fires in between guest exit and
vcpu_block().

Please try the patch below.

> > 2) I'm getting lot's of following messages on host, what do they mean?
> > [16836.605669] Ignoring de-assert INIT to vcpu SOMENUMBER
> > [16836.605687] SIPI to vcpu SOMENUMBER vector 0xSOMENUMBER

This is the APIC SMP initialization protocol.


diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 06a241a..fdd8342 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -199,10 +199,8 @@ int __pit_timer_fn(struct kvm_kpit_state *ps)
 	struct kvm_kpit_timer *pt = &ps->pit_timer;
 
 	atomic_inc(&pt->pending);
-	if (vcpu0 && waitqueue_active(&vcpu0->wq)) {
-		vcpu0->arch.mp_state = VCPU_MP_STATE_RUNNABLE;
-		wake_up_interruptible(&vcpu0->wq);
-	}
+	if (vcpu0)
+		kvm_wakeup_vcpu(vcpu0, VCPU_MP_STATE_RUNNABLE);
 
 	pt->timer.expires = ktime_add_ns(pt->timer.expires, pt->period);
 	pt->scheduled = ktime_to_ns(pt->timer.expires);
@@ -210,6 +208,16 @@ int __pit_timer_fn(struct kvm_kpit_state *ps)
 	return (pt->period == 0 ? 0 : 1);
 }
 
+int pit_has_pending_event(struct kvm_vcpu *vcpu)
+{
+	struct kvm_pit *pit = vcpu->kvm->arch.vpit;
+
+	if (pit && vcpu->vcpu_id == 0)
+		return atomic_read(&pit->pit_state.pit_timer.pending);
+
+	return 0;
+}
+
 static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
 {
 	struct kvm_kpit_state *ps;
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index dbfe21c..18b8a0e 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -26,6 +26,21 @@
 #include "i8254.h"
 
 /*
+ * check if there are pending timer events
+ * to be processed.
+ */
+int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
+{
+	int ret;
+
+	ret = pit_has_pending_event(vcpu);
+	ret |= apic_has_pending_event(vcpu);
+
+	return ret;
+}
+EXPORT_SYMBOL(kvm_cpu_has_pending_timer);
+
+/*
  * check if there is pending interrupt without
  * intack.
  */
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index fa5ed5d..ff0ddb5 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -85,4 +85,7 @@ void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
 void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu);
 
+int pit_has_pending_event(struct kvm_vcpu *vcpu);
+int apic_has_pending_event(struct kvm_vcpu *vcpu);
+
 #endif
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 31280df..9973d8e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -936,13 +936,10 @@ EXPORT_SYMBOL_GPL(kvm_lapic_enabled);
 static int __apic_timer_fn(struct kvm_lapic *apic)
 {
 	int result = 0;
-	wait_queue_head_t *q = &apic->vcpu->wq;
 
 	atomic_inc(&apic->timer.pending);
-	if (waitqueue_active(q)) {
-		apic->vcpu->arch.mp_state = VCPU_MP_STATE_RUNNABLE;
-		wake_up_interruptible(q);
-	}
+	kvm_wakeup_vcpu(apic->vcpu, VCPU_MP_STATE_RUNNABLE);
+
 	if (apic_lvtt_period(apic)) {
 		result = 1;
 		apic->timer.dev.expires = ktime_add_ns(
@@ -952,6 +949,16 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
 	return result;
 }
 
+int apic_has_pending_event(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *lapic = vcpu->arch.apic;
+
+	if (lapic)
+		return atomic_read(&lapic->timer.pending);
+
+	return 0;
+}
+
 static int __inject_apic_timer_irq(struct kvm_lapic *apic)
 {
 	int vector;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cb57b6a..b11ea81 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3925,10 +3944,7 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 {
 	int ipi_pcpu = vcpu->cpu;
 
-	if (waitqueue_active(&vcpu->wq)) {
-		wake_up_interruptible(&vcpu->wq);
-		++vcpu->stat.halt_wakeup;
-	}
+	kvm_wakeup_vcpu(vcpu, vcpu->arch.mp_state);
 	if (vcpu->guest_mode)
 		smp_call_function_single(ipi_pcpu, vcpu_kick_intr, vcpu, 0, 0);
 }
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a2ceb51..a6cb75e 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -69,6 +69,7 @@ struct kvm_vcpu {
 	int fpu_active;
 	int guest_fpu_loaded;
 	wait_queue_head_t wq;
+	spinlock_t wq_lock;
 	int sigset_active;
 	sigset_t sigset;
 	struct kvm_vcpu_stat stat;
@@ -184,6 +185,7 @@ int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn);
 void mark_page_dirty(struct kvm *kvm, gfn_t gfn);
 
 void kvm_vcpu_block(struct kvm_vcpu *vcpu);
+void kvm_wakeup_vcpu(struct kvm_vcpu *vcpu, int mpstate);
 void kvm_resched(struct kvm_vcpu *vcpu);
 void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
@@ -256,6 +262,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm);
 
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *v);
+int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu);
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
 
 static inline void kvm_guest_enter(void)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3396a5f..00d1a6c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -152,6 +152,7 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->kvm = kvm;
 	vcpu->vcpu_id = id;
 	init_waitqueue_head(&vcpu->wq);
+	spin_lock_init(&vcpu->wq_lock);
 
 	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
 	if (!page) {
@@ -698,6 +699,23 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
 	}
 }
 
+void kvm_wakeup_vcpu(struct kvm_vcpu *vcpu, int mpstate)
+{
+	wait_queue_head_t *q = &vcpu->wq;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vcpu->wq_lock, flags);
+	if (waitqueue_active(q)) {
+#ifdef CONFIG_X86
+		vcpu->arch.mp_state = mpstate;
+#endif
+		wake_up_interruptible(q);
+		++vcpu->stat.halt_wakeup;
+	}
+	spin_unlock_irqrestore(&vcpu->wq_lock, flags);
+
+}
+
 /*
  * The vCPU has executed a HLT instruction with in-kernel mode enabled.
  */
@@ -705,19 +723,24 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 {
 	DECLARE_WAITQUEUE(wait, current);
 
+	spin_lock_irq(&vcpu->wq_lock);
 	add_wait_queue(&vcpu->wq, &wait);
 
 	/*
 	 * We will block until either an interrupt or a signal wakes us up
 	 */
 	while (!kvm_cpu_has_interrupt(vcpu)
+	       && !kvm_cpu_has_pending_timer(vcpu)
 	       && !signal_pending(current)
 	       && !kvm_arch_vcpu_runnable(vcpu)) {
 		set_current_state(TASK_INTERRUPTIBLE);
+		spin_unlock_irq(&vcpu->wq_lock);
 		vcpu_put(vcpu);
 		schedule();
 		vcpu_load(vcpu);
+		spin_lock_irq(&vcpu->wq_lock);
 	}
+	spin_unlock_irq(&vcpu->wq_lock);
 
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&vcpu->wq, &wait);


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Register now and save $200. Hurry, offer ends at 11:59 p.m., 
Monday, April 7! Use priority code J8TLD2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone