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

PATCH Marcelo Tosatti 2 participants 5 messages Jun 19, 2008 – Oct 18, 2008

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2008-06-19 17:44 UTC #0 lore raw

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

arch/x86/kvm/lapic.c +35 -0

Index: 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);
 }

arch/x86/kvm/x86.c +0 ~~-31~~

Index: 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;
 }

Avi Kivity

2008-06-22 04:59 UTC #1 lore raw

Quoted 4 lines · 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.

It's to avoid pinning the page indefinitely.

Quoted 4 lines

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.

Signature · 1 line

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

Marcelo Tosatti

author

2008-06-22 17:06 UTC #2 lore raw

Quoted 23 lines · Avi Kivity

Quoted 4 lines · 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.
It's to avoid pinning the page indefinitely.
Quoted 5 lines
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();

Quoted 7 lines

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 ?

Avi Kivity

2008-06-23 02:29 UTC #3 lore raw

Quoted 35 lines · Marcelo Tosatti

Quoted 24 lines · Avi Kivity
Quoted 5 lines · 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.
It's to avoid pinning the page indefinitely.
Quoted 5 lines
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();
Quoted 8 lines
mmu notifier:
if (gpa == vapic_addr)
exit_vapic()
So let's wait with this until mmu notifiers are merged.

Quoted 8 lines

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.

Quoted 2 lines

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.

Signature · 1 line

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

Marcelo Tosatti

author

2008-10-18 10:14 UTC #4 lore raw

Quoted 2 lines · Avi Kivity

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.

Quoted 17 lines

Quoted 2 lines
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;
}

Quoted 3 lines

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

arch/x86/kvm/x86.c +2 -2

index 26b051b..29e8983 100644
@@ -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;
 }