[PATCH RFC] KVM: busy-spin detector

[PATCH RFC] KVM: busy-spin detector

[Marcelo Tosatti]

The following patch implements a simple busy-spin detector. It considers
a vcpu as busy-spinning if there are two consecutive exits due to
external interrupt on the same RIP, and sleeps for 100us in that case.

It is very likely that if the vcpu is making progress it will either
exit for other reasons or change RIP.

The percentage numbers below represent improvement in kernel build
time in comparison with mainline (RHEL 5.4 guest).

make -j16, 8 cpu host:
smp 16: 3%
smp 18: 10%
smp 32: 14%

smp 4, make -j4, pinned to 2 cpus:
4%

smp 8, make -j8, pinned to 2 cpus:
5%


Index: kvm/arch/x86/include/asm/kvm_host.h
===================================================================
--- kvm.orig/arch/x86/include/asm/kvm_host.h
+++ kvm/arch/x86/include/asm/kvm_host.h
@@ -301,6 +301,8 @@ struct kvm_vcpu_arch {
 		unsigned long mmu_seq;
 	} update_pte;
 
+	unsigned long last_rip;
+
 	struct fpu guest_fpu;
 
 	gva_t mmio_fault_cr2;
@@ -653,6 +655,8 @@ void kvm_disable_tdp(void);
 int complete_pio(struct kvm_vcpu *vcpu);
 bool kvm_check_iopl(struct kvm_vcpu *vcpu);
 
+void kvm_detect_spin(struct kvm_vcpu *vcpu);
+
 struct kvm_memory_slot *gfn_to_memslot_unaliased(struct kvm *kvm, gfn_t gfn);
 
 static inline struct kvm_mmu_page *page_header(hpa_t shadow_page)
Index: kvm/arch/x86/kvm/svm.c
===================================================================
--- kvm.orig/arch/x86/kvm/svm.c
+++ kvm/arch/x86/kvm/svm.c
@@ -1558,8 +1558,10 @@ static int nmi_interception(struct vcpu_
 
 static int intr_interception(struct vcpu_svm *svm)
 {
+	if (!svm_has(SVM_FEATURE_PAUSE_FILTER))
+		kvm_detect_spin(&svm->vcpu);
 	++svm->vcpu.stat.irq_exits;
-	return 1;
+	return 2;
 }
 
 static int nop_on_interception(struct vcpu_svm *svm)
Index: kvm/arch/x86/kvm/vmx.c
===================================================================
--- kvm.orig/arch/x86/kvm/vmx.c
+++ kvm/arch/x86/kvm/vmx.c
@@ -3116,7 +3116,9 @@ static int handle_exception(struct kvm_v
 static int handle_external_interrupt(struct kvm_vcpu *vcpu)
 {
 	++vcpu->stat.irq_exits;
-	return 1;
+	if (!cpu_has_vmx_ple())
+		kvm_detect_spin(vcpu);
+	return 2;
 }
 
 static int handle_triple_fault(struct kvm_vcpu *vcpu)
Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -4523,6 +4523,17 @@ static void inject_pending_event(struct 
 	}
 }
 
+void kvm_detect_spin(struct kvm_vcpu *vcpu)
+{
+	unsigned long rip = kvm_rip_read(vcpu);
+
+	if (vcpu->arch.last_rip == rip)
+		kvm_vcpu_on_spin(vcpu);
+
+	vcpu->arch.last_rip = rip;
+}
+EXPORT_SYMBOL_GPL(kvm_detect_spin);
+
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;
@@ -4654,6 +4665,8 @@ static int vcpu_enter_guest(struct kvm_v
 	kvm_lapic_sync_from_vapic(vcpu);
 
 	r = kvm_x86_ops->handle_exit(vcpu);
+	if (r == 1)
+		vcpu->arch.last_rip = ~(0UL);
 out:
 	return r;
 }

Re: [PATCH RFC] KVM: busy-spin detector

[Balbir Singh]

* Marcelo Tosatti <mtosatti@redhat.com> [2010-06-10 23:25:51]:

> 
> The following patch implements a simple busy-spin detector. It considers
> a vcpu as busy-spinning if there are two consecutive exits due to
> external interrupt on the same RIP, and sleeps for 100us in that case.
> 
> It is very likely that if the vcpu is making progress it will either
> exit for other reasons or change RIP.
> 
> The percentage numbers below represent improvement in kernel build
> time in comparison with mainline (RHEL 5.4 guest).
>

Interesting approach, is there a reason to tie it in with pause loop
exits? Can't we do something more generic or even para-virtish.

-- 
	Three Cheers,
	Balbir

RE: [PATCH RFC] KVM: busy-spin detector

[Huang, Zhiteng]

PLE-like design may be more generic than para-virtish when it comes to Windows guest.

Is this busy-spin actually a Lock Holder Preemption problem?

Regards,

HUANG, Zhiteng



-----Original Message-----
From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Balbir Singh
Sent: Friday, June 11, 2010 10:07 PM
To: Marcelo Tosatti
Cc: kvm
Subject: Re: [PATCH RFC] KVM: busy-spin detector

* Marcelo Tosatti <mtosatti@redhat.com> [2010-06-10 23:25:51]:

> 
> The following patch implements a simple busy-spin detector. It 
> considers a vcpu as busy-spinning if there are two consecutive exits 
> due to external interrupt on the same RIP, and sleeps for 100us in that case.
> 
> It is very likely that if the vcpu is making progress it will either 
> exit for other reasons or change RIP.
> 
> The percentage numbers below represent improvement in kernel build 
> time in comparison with mainline (RHEL 5.4 guest).
>

Interesting approach, is there a reason to tie it in with pause loop exits? Can't we do something more generic or even para-virtish.

-- 
	Three Cheers,
	Balbir
--
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 RFC] KVM: busy-spin detector

[Balbir Singh]

* Huang, Zhiteng <zhiteng.huang@intel.com> [2010-06-11 23:03:25]:

> PLE-like design may be more generic than para-virtish when it comes to Windows guest.
>

Hmm.. sounds reasonable
 
> Is this busy-spin actually a Lock Holder Preemption problem?
>

Yep, I was hinting towards solving that problem. 

-- 
	Three Cheers,
	Balbir

Re: [PATCH RFC] KVM: busy-spin detector

[Marcelo Tosatti]

On Fri, Jun 11, 2010 at 07:36:34PM +0530, Balbir Singh wrote:
> * Marcelo Tosatti <mtosatti@redhat.com> [2010-06-10 23:25:51]:
> 
> > 
> > The following patch implements a simple busy-spin detector. It considers
> > a vcpu as busy-spinning if there are two consecutive exits due to
> > external interrupt on the same RIP, and sleeps for 100us in that case.
> > 
> > It is very likely that if the vcpu is making progress it will either
> > exit for other reasons or change RIP.
> > 
> > The percentage numbers below represent improvement in kernel build
> > time in comparison with mainline (RHEL 5.4 guest).
> >
> 
> Interesting approach, is there a reason to tie it in with pause loop
> exits? 

Hum, i don't see any. PLE exits provide the same detection, but more
accurately.

> Can't we do something more generic or even para-virtish.

This is pretty generic already? Or what do you mean?

The advantage is it does not require paravirt modifications in the      
guest (at the expense of guessing what the guest is doing).

Re: [PATCH RFC] KVM: busy-spin detector

[Balbir Singh]

* Marcelo Tosatti <mtosatti@redhat.com> [2010-06-11 14:46:27]:

> > Interesting approach, is there a reason to tie it in with pause loop
> > exits? 
> 
> Hum, i don't see any. PLE exits provide the same detection, but more
> accurately.
>
> > Can't we do something more generic or even para-virtish.
> 
> This is pretty generic already? Or what do you mean?
> 
> The advantage is it does not require paravirt modifications in the      
> guest (at the expense of guessing what the guest is doing).
>

Agreed, but one needs to depend on newer hardware to get this feature
to work. 

-- 
	Three Cheers,
	Balbir

Re: [PATCH RFC] KVM: busy-spin detector

[Avi Kivity]

On 06/11/2010 05:25 AM, Marcelo Tosatti wrote:
> The following patch implements a simple busy-spin detector. It considers
> a vcpu as busy-spinning if there are two consecutive exits due to
> external interrupt on the same RIP, and sleeps for 100us in that case.
>
> It is very likely that if the vcpu is making progress it will either
> exit for other reasons or change RIP.
>
> The percentage numbers below represent improvement in kernel build
> time in comparison with mainline (RHEL 5.4 guest).
>
> make -j16, 8 cpu host:
> smp 16: 3%
> smp 18: 10%
> smp 32: 14%
>
> smp 4, make -j4, pinned to 2 cpus:
> 4%
>
> smp 8, make -j8, pinned to 2 cpus:
> 5%
>
>    

The big problem here is not in the patch itself, but in 
kvm_vcpu_on_spin().  An un-overcommitted guest will trigger this on long 
spinlocks and go to sleep, and if the lock is released while we're still 
sleeping, we lose performance.

> @@ -4654,6 +4665,8 @@ static int vcpu_enter_guest(struct kvm_v
>   	kvm_lapic_sync_from_vapic(vcpu);
>
>   	r = kvm_x86_ops->handle_exit(vcpu);
> +	if (r == 1)
> +		vcpu->arch.last_rip = ~(0UL);
>   out:
>   	return r;
>   }
>    

!= 2, no?

It's a little ugly to overload the return code like that.  Perhaps:

+    vcpu->arch.save_last_rip = vcpu->arch.last_rip
+    vcpu->arch.last_rip = ~0UL;
      r = kvm_x86_ops->handle_exit(vcpu);

And kvm_detect_spin() can restore last_rip.

-- 
error compiling committee.c: too many arguments to function