KVM: x86: use smp_send_reschedule in kvm_vcpu_kick

Commit Message

Marcelo Tosatti March 9, 2009, 11:23 p.m. UTC

On Mon, Mar 09, 2009 at 12:09:06PM +0100, Peter Zijlstra wrote:
> On Mon, 2009-03-09 at 11:58 +0100, Ingo Molnar wrote:
> > * Avi Kivity <[e-mail]> wrote:
> > 
> > > Looks good.  The only worry I have is that we depend on 
> > > smp_reschedule_interrupt() being a no-op.  I guess that's a 
> > > reasonable assumption though.
> > 
> > It's a reasonable current assumption - but it might change in 
> > the future - so please also put it into the changelog that KVM 
> > will revert it or fix it differently if the scheduler grows some 
> > functionality there.
> 
> Alternatively, do something like the below, then anybody poking at that
> code will know to prod the KVM folks when they change anything.
> 
> [ I suspect other kvm arches will need to a similar comment ]

OK, reworked patch:
- change ia64 in addition to x86
- add comment on smp send reschedule handlers about KVM's usage

Untested on IA64.

KVM: use smp_send_reschedule in kvm_vcpu_kick

KVM uses a function call IPI to cause the exit of a guest running on a
physical cpu. For virtual interrupt notification there is no need to
wait on IPI receival, or to execute any function.

This is exactly what the reschedule IPI does, without the overhead
of function IPI. So use it instead of smp_call_function_single in
kvm_vcpu_kick.

Also change the "guest_mode" variable to a bit in vcpu->requests, and
use that to collapse multiple IPI's that would be issued between the
first one and zeroing of guest mode.

This allows kvm_vcpu_kick to called with interrupts disabled.

Signed-off-by: Marcelo Tosatti <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Avi Kivity March 10, 2009, 8:01 a.m. UTC | #1

Marcelo Tosatti wrote:
> OK, reworked patch:
> - change ia64 in addition to x86
> - add comment on smp send reschedule handlers about KVM's usage
>
> Untested on IA64.
>
> KVM: use smp_send_reschedule in kvm_vcpu_kick
>
> KVM uses a function call IPI to cause the exit of a guest running on a
> physical cpu. For virtual interrupt notification there is no need to
> wait on IPI receival, or to execute any function.
>
> This is exactly what the reschedule IPI does, without the overhead
> of function IPI. So use it instead of smp_call_function_single in
> kvm_vcpu_kick.
>
> Also change the "guest_mode" variable to a bit in vcpu->requests, and
> use that to collapse multiple IPI's that would be issued between the
> first one and zeroing of guest mode.
>
> This allows kvm_vcpu_kick to called with interrupts disabled.
>   

Looks good. Will wait for Xiantao's test-n-ack before applying.

Zhang, Xiantao March 12, 2009, 2:31 a.m. UTC | #2

Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> OK, reworked patch:
>> - change ia64 in addition to x86
>> - add comment on smp send reschedule handlers about KVM's usage
>> 
>> Untested on IA64.
>> 
>> KVM: use smp_send_reschedule in kvm_vcpu_kick
>> 
>> KVM uses a function call IPI to cause the exit of a guest running on
>> a physical cpu. For virtual interrupt notification there is no need
>> to wait on IPI receival, or to execute any function.
>> 
>> This is exactly what the reschedule IPI does, without the overhead
>> of function IPI. So use it instead of smp_call_function_single in
>> kvm_vcpu_kick. 
>> 
>> Also change the "guest_mode" variable to a bit in vcpu->requests, and
>> use that to collapse multiple IPI's that would be issued between the
>> first one and zeroing of guest mode.
>> 
>> This allows kvm_vcpu_kick to called with interrupts disabled.
>> 
> 
> Looks good. Will wait for Xiantao's test-n-ack before applying.

kvm-ia64 is broken due to recent check-ins about irq-bits, and we are trying to fix it. For this patch, ia64 has to export the symbol smp_send_reschedule before applying the patch.
Xiantao--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Avi Kivity March 12, 2009, 7:06 a.m. UTC | #3

Zhang, Xiantao wrote:
>>
>> Looks good. Will wait for Xiantao's test-n-ack before applying.
>>     
>
> kvm-ia64 is broken due to recent check-ins about irq-bits, and we are trying to fix it. 

Sorry about that, see Gleb's patch as well.

> For this patch, ia64 has to export the symbol smp_send_reschedule before applying the patch.

Please send a patch for that once you've tested it.

Gleb Natapov March 12, 2009, 11:04 a.m. UTC | #4

On Thu, Mar 12, 2009 at 10:31:47AM +0800, Zhang, Xiantao wrote:
> Avi Kivity wrote:
> > Marcelo Tosatti wrote:
> >> OK, reworked patch:
> >> - change ia64 in addition to x86
> >> - add comment on smp send reschedule handlers about KVM's usage
> >> 
> >> Untested on IA64.
> >> 
> >> KVM: use smp_send_reschedule in kvm_vcpu_kick
> >> 
> >> KVM uses a function call IPI to cause the exit of a guest running on
> >> a physical cpu. For virtual interrupt notification there is no need
> >> to wait on IPI receival, or to execute any function.
> >> 
> >> This is exactly what the reschedule IPI does, without the overhead
> >> of function IPI. So use it instead of smp_call_function_single in
> >> kvm_vcpu_kick. 
> >> 
> >> Also change the "guest_mode" variable to a bit in vcpu->requests, and
> >> use that to collapse multiple IPI's that would be issued between the
> >> first one and zeroing of guest mode.
> >> 
> >> This allows kvm_vcpu_kick to called with interrupts disabled.
> >> 
> > 
> > Looks good. Will wait for Xiantao's test-n-ack before applying.
> 
> kvm-ia64 is broken due to recent check-ins about irq-bits, and we are trying to fix it. For this patch, ia64 has to export the symbol smp_send_reschedule before applying the patch.
Can you try this patch please: http://patchwork.kernel.org/patch/11103/

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhang, Xiantao March 13, 2009, 2:15 a.m. UTC | #5

We also hacked the source like the patch. But the issue is not caused by it. We are still trying to figure the reason out. Thanks!  
Xiantao

-----Original Message-----
From: Gleb Natapov [mailto:[e-mail]] 
Sent: Thursday, March 12, 2009 7:04 PM
To: Zhang, Xiantao
Cc: Avi Kivity; Marcelo Tosatti; Ingo Molnar; [e-mail]; Peter Zijlstra
Subject: Re: x86: use smp_send_reschedule in kvm_vcpu_kick

On Thu, Mar 12, 2009 at 10:31:47AM +0800, Zhang, Xiantao wrote:
> Avi Kivity wrote:
> > Marcelo Tosatti wrote:
> >> OK, reworked patch:
> >> - change ia64 in addition to x86
> >> - add comment on smp send reschedule handlers about KVM's usage
> >> 
> >> Untested on IA64.
> >> 
> >> KVM: use smp_send_reschedule in kvm_vcpu_kick
> >> 
> >> KVM uses a function call IPI to cause the exit of a guest running on
> >> a physical cpu. For virtual interrupt notification there is no need
> >> to wait on IPI receival, or to execute any function.
> >> 
> >> This is exactly what the reschedule IPI does, without the overhead
> >> of function IPI. So use it instead of smp_call_function_single in
> >> kvm_vcpu_kick. 
> >> 
> >> Also change the "guest_mode" variable to a bit in vcpu->requests, and
> >> use that to collapse multiple IPI's that would be issued between the
> >> first one and zeroing of guest mode.
> >> 
> >> This allows kvm_vcpu_kick to called with interrupts disabled.
> >> 
> > 
> > Looks good. Will wait for Xiantao's test-n-ack before applying.
> 
> kvm-ia64 is broken due to recent check-ins about irq-bits, and we are trying to fix it. For this patch, ia64 has to export the symbol smp_send_reschedule before applying the patch.
Can you try this patch please: http://patchwork.kernel.org/patch/11103/

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/ia64/kernel/irq_ia64.c b/arch/ia64/kernel/irq_ia64.c
index 28d3d48..0e8242d 100644
--- a/arch/ia64/kernel/irq_ia64.c
+++ b/arch/ia64/kernel/irq_ia64.c
@@ -607,6 +607,9 @@  static struct irqaction ipi_irqaction = {
 	.name =		"IPI"
 };
 
+/*
+ * KVM uses this interrupt to force a cpu out of guest mode
+ */
 static struct irqaction resched_irqaction = {
 	.handler =	dummy_handler,
 	.flags =	IRQF_DISABLED,
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index e1984cb..080dcdb 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -633,7 +633,7 @@  again:
 		goto out;
 	}
 
-	vcpu->guest_mode = 1;
+	clear_bit(KVM_REQ_KICK, &vcpu->requests);
 	kvm_guest_enter();
 	down_read(&vcpu->kvm->slots_lock);
 	r = vti_vcpu_run(vcpu, kvm_run);
@@ -645,7 +645,7 @@  again:
 	}
 
 	vcpu->arch.launched = 1;
-	vcpu->guest_mode = 0;
+	set_bit(KVM_REQ_KICK, &vcpu->requests);
 	local_irq_enable();
 
 	/*
@@ -1802,24 +1802,17 @@  void kvm_arch_hardware_unsetup(void)
 {
 }
 
-static void vcpu_kick_intr(void *info)
-{
-#ifdef DEBUG
-	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)info;
-	printk(KERN_DEBUG"vcpu_kick_intr %p \n", vcpu);
-#endif
-}
-
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 {
-	int ipi_pcpu = vcpu->cpu;
-	int cpu = get_cpu();
+	int me = get_cpu();
+	int cpu = vcpu->cpu;
 
 	if (waitqueue_active(&vcpu->wq))
 		wake_up_interruptible(&vcpu->wq);
 
-	if (vcpu->guest_mode && cpu != ipi_pcpu)
-		smp_call_function_single(ipi_pcpu, vcpu_kick_intr, vcpu, 0);
+	if (cpu != me && (unsigned) cpu < nr_cpu_ids && cpu_online(cpu))
+		if (!test_and_set_bit(KVM_REQ_KICK, &vcpu->requests))
+			smp_send_reschedule(cpu);
 	put_cpu();
 }
 
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index e6faa33..ca4fa7f 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -173,6 +173,9 @@  void smp_reschedule_interrupt(struct pt_regs *regs)
 {
 	ack_APIC_irq();
 	inc_irq_stat(irq_resched_count);
+	/*
+	 * KVM uses this interrupt to force a cpu out of guest mode
+	 */
 }
 
 void smp_call_function_interrupt(struct pt_regs *regs)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b556b6a..c68168c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3133,6 +3133,9 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 
 	local_irq_disable();
 
+	clear_bit(KVM_REQ_KICK, &vcpu->requests);
+	smp_mb__after_clear_bit();
+
 	if (vcpu->requests || need_resched() || signal_pending(current)) {
 		local_irq_enable();
 		preempt_enable();
@@ -3140,13 +3143,6 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		goto out;
 	}
 
-	vcpu->guest_mode = 1;
-	/*
-	 * Make sure that guest_mode assignment won't happen after
-	 * testing the pending IRQ vector bitmap.
-	 */
-	smp_wmb();
-
 	if (vcpu->arch.exception.pending)
 		__queue_exception(vcpu);
 	else if (irqchip_in_kernel(vcpu->kvm))
@@ -3188,7 +3184,7 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	set_debugreg(vcpu->arch.host_dr6, 6);
 	set_debugreg(vcpu->arch.host_dr7, 7);
 
-	vcpu->guest_mode = 0;
+	set_bit(KVM_REQ_KICK, &vcpu->requests);
 	local_irq_enable();
 
 	++vcpu->stat.exits;
@@ -4429,28 +4425,19 @@  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 	       || vcpu->arch.nmi_pending;
 }
 
-static void vcpu_kick_intr(void *info)
-{
-#ifdef DEBUG
-	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)info;
-	printk(KERN_DEBUG "vcpu_kick_intr %p \n", vcpu);
-#endif
-}
-
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 {
-	int ipi_pcpu = vcpu->cpu;
-	int cpu = get_cpu();
+	int me = get_cpu();
+	int cpu = vcpu->cpu;
 
 	if (waitqueue_active(&vcpu->wq)) {
 		wake_up_interruptible(&vcpu->wq);
 		++vcpu->stat.halt_wakeup;
 	}
-	/*
-	 * We may be called synchronously with irqs disabled in guest mode,
-	 * So need not to call smp_call_function_single() in that case.
-	 */
-	if (vcpu->guest_mode && vcpu->cpu != cpu)
-		smp_call_function_single(ipi_pcpu, vcpu_kick_intr, vcpu, 0);
+
+	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
+		if (!test_and_set_bit(KVM_REQ_KICK, &vcpu->requests))
+			smp_send_reschedule(cpu);
+
 	put_cpu();
 }
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3b91ec9..4d7da7d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -38,6 +38,7 @@ 
 #define KVM_REQ_UNHALT             6
 #define KVM_REQ_MMU_SYNC           7
 #define KVM_REQ_KVMCLOCK_UPDATE    8
+#define KVM_REQ_KICK               9
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID	0
 
@@ -72,7 +73,6 @@  struct kvm_vcpu {
 	struct mutex mutex;
 	int   cpu;
 	struct kvm_run *run;
-	int guest_mode;
 	unsigned long requests;
 	unsigned long guest_debug;
 	int fpu_active;