KVM: x86: move vapic page handling out of fast path

KVM: x86: move vapic page handling out of fast path

[Marcelo Tosatti]

I fail to see the point of handling the vapic page grab and ref
counting in __vcpu_run's heavyweight enter/exit path.

So move it to kvm_lapic_set_vapic_addr / kvm_free_lapic time.

Other than the obvious improvement for non-Flexpriority case, this      
kills a down_read/up_read pair in heavy exits and reduces code size.    

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


Index: kvm/arch/x86/kvm/lapic.c
===================================================================
--- kvm.orig/arch/x86/kvm/lapic.c
+++ kvm/arch/x86/kvm/lapic.c
@@ -800,6 +800,34 @@ static int apic_mmio_range(struct kvm_io
 	return ret;
 }
 
+static void vapic_get_page(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	struct page *page;
+
+	if (!apic || !apic->vapic_addr)
+		return;
+
+	down_read(&vcpu->kvm->slots_lock);
+	down_read(&current->mm->mmap_sem);
+	page = gfn_to_page(vcpu->kvm, apic->vapic_addr >> PAGE_SHIFT);
+	up_read(&current->mm->mmap_sem);
+	up_read(&vcpu->kvm->slots_lock);
+
+	vcpu->arch.apic->vapic_page = page;
+}
+
+static void vapic_put_page(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!irqchip_in_kernel(vcpu->kvm) || !apic || !apic->vapic_addr)
+		return;
+
+	kvm_release_page_dirty(apic->vapic_page);
+	mark_page_dirty(vcpu->kvm, apic->vapic_addr >> PAGE_SHIFT);
+}
+
 void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
 	if (!vcpu->arch.apic)
@@ -810,6 +838,9 @@ void kvm_free_lapic(struct kvm_vcpu *vcp
 	if (vcpu->arch.apic->regs_page)
 		__free_page(vcpu->arch.apic->regs_page);
 
+	if (vcpu->arch.apic->vapic_page)
+		vapic_put_page(vcpu);
+
 	kfree(vcpu->arch.apic);
 }
 
@@ -1172,5 +1203,9 @@ void kvm_lapic_set_vapic_addr(struct kvm
 	if (!irqchip_in_kernel(vcpu->kvm))
 		return;
 
+	if (vcpu->arch.apic->vapic_page)
+		vapic_put_page(vcpu);
+
 	vcpu->arch.apic->vapic_addr = vapic_addr;
+	vapic_get_page(vcpu);
 }
Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -2737,32 +2737,6 @@ static void post_kvm_run_save(struct kvm
 					 vcpu->arch.irq_summary == 0);
 }
 
-static void vapic_enter(struct kvm_vcpu *vcpu)
-{
-	struct kvm_lapic *apic = vcpu->arch.apic;
-	struct page *page;
-
-	if (!apic || !apic->vapic_addr)
-		return;
-
-	down_read(&current->mm->mmap_sem);
-	page = gfn_to_page(vcpu->kvm, apic->vapic_addr >> PAGE_SHIFT);
-	up_read(&current->mm->mmap_sem);
-
-	vcpu->arch.apic->vapic_page = page;
-}
-
-static void vapic_exit(struct kvm_vcpu *vcpu)
-{
-	struct kvm_lapic *apic = vcpu->arch.apic;
-
-	if (!apic || !apic->vapic_addr)
-		return;
-
-	kvm_release_page_dirty(apic->vapic_page);
-	mark_page_dirty(vcpu->kvm, apic->vapic_addr >> PAGE_SHIFT);
-}
-
 static int __vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	int r;
@@ -2778,7 +2752,6 @@ static int __vcpu_run(struct kvm_vcpu *v
 	}
 
 	down_read(&vcpu->kvm->slots_lock);
-	vapic_enter(vcpu);
 
 preempted:
 	if (vcpu->guest_debug.enabled)
@@ -2916,10 +2889,6 @@ out:
 
 	post_kvm_run_save(vcpu, kvm_run);
 
-	down_read(&vcpu->kvm->slots_lock);
-	vapic_exit(vcpu);
-	up_read(&vcpu->kvm->slots_lock);
-
 	return r;
 }
 

Re: KVM: x86: move vapic page handling out of fast path

[Avi Kivity]

Marcelo Tosatti wrote:
> I fail to see the point of handling the vapic page grab and ref
> counting in __vcpu_run's heavyweight enter/exit path.
>
>   

It's to avoid pinning the page indefinitely.

> So move it to kvm_lapic_set_vapic_addr / kvm_free_lapic time.
>
> Other than the obvious improvement for non-Flexpriority case, this      
> kills a down_read/up_read pair in heavy exits and reduces code size.    
>   

With mmu notifiers we can do this and still not ping the page:

fast path:


if (vapic_addr && !vapic_page)
    enter_vapic();


mmu notifier:

if (gpa == vapic_addr)
    exit_vapic()


So let's wait with this until mmu notifiers are merged.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: KVM: x86: move vapic page handling out of fast path

[Marcelo Tosatti]

On Sun, Jun 22, 2008 at 08:00:11AM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> I fail to see the point of handling the vapic page grab and ref
>> counting in __vcpu_run's heavyweight enter/exit path.
>>
>>   
>
> It's to avoid pinning the page indefinitely.
>
>> So move it to kvm_lapic_set_vapic_addr / kvm_free_lapic time.
>>
>> Other than the obvious improvement for non-Flexpriority case, this      
>> kills a down_read/up_read pair in heavy exits and reduces code size.    
>>   
>
> With mmu notifiers we can do this and still not ping the page:
>
> fast path:
>
>
> if (vapic_addr && !vapic_page)
>    enter_vapic();
>
>
> mmu notifier:
>
> if (gpa == vapic_addr)
>    exit_vapic()
>
>
> So let's wait with this until mmu notifiers are merged.

But what is the point, or advantage, of having the _any_ vapic page
handling in __vcpu_run ? 

The reference for it is grabbed at kvm_lapic_set_vapic_addr() (which
does not take any spinlock, so its safe to swapin the page) and released
at guest exit.

So, what do you have against this patch ?

Re: KVM: x86: move vapic page handling out of fast path

[Avi Kivity]

Marcelo Tosatti wrote:
> On Sun, Jun 22, 2008 at 08:00:11AM +0300, Avi Kivity wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> I fail to see the point of handling the vapic page grab and ref
>>> counting in __vcpu_run's heavyweight enter/exit path.
>>>
>>>   
>>>       
>> It's to avoid pinning the page indefinitely.
>>
>>     
>>> So move it to kvm_lapic_set_vapic_addr / kvm_free_lapic time.
>>>
>>> Other than the obvious improvement for non-Flexpriority case, this      
>>> kills a down_read/up_read pair in heavy exits and reduces code size.    
>>>   
>>>       
>> With mmu notifiers we can do this and still not ping the page:
>>
>> fast path:
>>
>>
>> if (vapic_addr && !vapic_page)
>>    enter_vapic();
>>
>>
>> mmu notifier:
>>
>> if (gpa == vapic_addr)
>>    exit_vapic()
>>
>>
>> So let's wait with this until mmu notifiers are merged.
>>     
>
> But what is the point, or advantage, of having the _any_ vapic page
> handling in __vcpu_run ? 
>
> The reference for it is grabbed at kvm_lapic_set_vapic_addr() (which
> does not take any spinlock, so its safe to swapin the page) and released
> at guest exit.
>
>   

The page can't be swapped out since its reference count is elevated 
indefinitely.


> So, what do you have against this patch ?
>   

We need to move away from reference counts, they make kvm brittle.  The 
patch improves the current state of things (since pages are pinned 
indefinitely anyway now) but takes the wrong direction for the future.

Note that kvmclock has the same issue, so we might as well share the 
solution:

struct kvm_fast_guest_page {
     gfn_t gfn;
     struct page *page;
     spinlock_t lock;
     struct list_head link;
}

The mmu notifier callbacks can scan this list and null any pages that 
match the gfn being cleared, but in the normal cast, ->page can be 
accessed directly.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: KVM: x86: move vapic page handling out of fast path

[Marcelo Tosatti]

On Mon, Jun 23, 2008 at 05:29:34AM +0300, Avi Kivity wrote:
> The page can't be swapped out since its reference count is elevated  
> indefinitely.

AFAICT the page can be swapped out when the guest has exited to
userspace and the only reference is the qemu userspace mapping.

>> So, what do you have against this patch ?
>>   
>
> We need to move away from reference counts, they make kvm brittle.  The  
> patch improves the current state of things (since pages are pinned  
> indefinitely anyway now) but takes the wrong direction for the future.
>
> Note that kvmclock has the same issue, so we might as well share the  
> solution:
>
> struct kvm_fast_guest_page {
>     gfn_t gfn;
>     struct page *page;
>     spinlock_t lock;
>     struct list_head link;
> }
>
> The mmu notifier callbacks can scan this list and null any pages that  
> match the gfn being cleared, but in the normal cast, ->page can be  
> accessed directly.

OK, can you please apply this at least:

KVM: move slots_lock acquision down to vapic_exit

There is no need to grab slots_lock if the vapic_page will not
be touched.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 26b051b..29e8983 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2759,8 +2759,10 @@ static void vapic_exit(struct kvm_vcpu *vcpu)
 	if (!apic || !apic->vapic_addr)
 		return;
 
+	down_read(&vcpu->kvm->slots_lock);
 	kvm_release_page_dirty(apic->vapic_page);
 	mark_page_dirty(vcpu->kvm, apic->vapic_addr >> PAGE_SHIFT);
+	up_read(&vcpu->kvm->slots_lock);
 }
 
 static int __vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
@@ -2916,9 +2918,7 @@ out:
 
 	post_kvm_run_save(vcpu, kvm_run);
 
-	down_read(&vcpu->kvm->slots_lock);
 	vapic_exit(vcpu);
-	up_read(&vcpu->kvm->slots_lock);
 
 	return r;
 }

Re: KVM: x86: move vapic page handling out of fast path

[Marcelo Tosatti]

On Mon, Jun 23, 2008 at 05:29:34AM +0300, Avi Kivity wrote:
> The page can't be swapped out since its reference count is elevated  
> indefinitely.

AFAICT the page can be swapped out when the guest has exited to
userspace and the only reference is the qemu userspace mapping.

>> So, what do you have against this patch ?
>>   
>
> We need to move away from reference counts, they make kvm brittle.  The  
> patch improves the current state of things (since pages are pinned  
> indefinitely anyway now) but takes the wrong direction for the future.
>
> Note that kvmclock has the same issue, so we might as well share the  
> solution:
>
> struct kvm_fast_guest_page {
>     gfn_t gfn;
>     struct page *page;
>     spinlock_t lock;
>     struct list_head link;
> }
>
> The mmu notifier callbacks can scan this list and null any pages that  
> match the gfn being cleared, but in the normal cast, ->page can be  
> accessed directly.

OK, can you please apply this at least:

KVM: move slots_lock acquision down to vapic_exit

There is no need to grab slots_lock if the vapic_page will not
be touched.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 26b051b..29e8983 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2759,8 +2759,10 @@ static void vapic_exit(struct kvm_vcpu *vcpu)
 	if (!apic || !apic->vapic_addr)
 		return;
 
+	down_read(&vcpu->kvm->slots_lock);
 	kvm_release_page_dirty(apic->vapic_page);
 	mark_page_dirty(vcpu->kvm, apic->vapic_addr >> PAGE_SHIFT);
+	up_read(&vcpu->kvm->slots_lock);
 }
 
 static int __vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
@@ -2916,9 +2918,7 @@ out:
 
 	post_kvm_run_save(vcpu, kvm_run);
 
-	down_read(&vcpu->kvm->slots_lock);
 	vapic_exit(vcpu);
-	up_read(&vcpu->kvm->slots_lock);
 
 	return r;
 }

Re: KVM: x86: move vapic page handling out of fast path

[Avi Kivity]

Marcelo Tosatti wrote:
> On Mon, Jun 23, 2008 at 05:29:34AM +0300, Avi Kivity wrote:
>   
>> The page can't be swapped out since its reference count is elevated  
>> indefinitely.
>>     
>
> AFAICT the page can be swapped out when the guest has exited to
> userspace and the only reference is the qemu userspace mapping.
>
>   

That's true, but immaterial, as guests spend most of their time running 
guest code or sleeping in the kernel.

We could drop the page while the guest is sleeping, but I'd like things 
to work well even if the guest never idles.

> OK, can you please apply this at least:
>
> KVM: move slots_lock acquision down to vapic_exit
>
> There is no need to grab slots_lock if the vapic_page will not
> be touched.
>
>   

Sure, applied.


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