KVM: MMU: optimize set_spte for page sync

The cost of hash table and memslot lookups are quite significant if the workload is pagetable write intensive resulting in increased mmu_lock contention.

arch/x86/kvm/mmu.c +0 -0

@@ -1593,7 +1593,16 @@ static int set_spte(struct kvm_vcpu *vcp
 
 		spte |= PT_WRITABLE_MASK;
 

Quoted 4 lines · Marcelo Tosatti

The cost of hash table and memslot lookups are quite significant if the workload is pagetable write intensive resulting in increased mmu_lock contention.

arch/x86/kvm/mmu.c static int set_spte(struct kvm_vcpu *vcp · line 1603

Quoted 12 lines

arch/x86/kvm/mmu.c +8 -0

@@ -1593,7 +1593,16 @@ static int set_spte(struct kvm_vcpu *vcp
  		spte |= PT_WRITABLE_MASK;
 -		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		/*
+ 	 	 * Optimization: for pte sync, if spte was writable the hash
+ 	 	 * lookup is unnecessary (and expensive). Write protection
+ 	 	 * is responsibility of mmu_get_page / kvm_sync_page.
+ 	 	 * Same reasoning can be applied to dirty page accounting.
+ 	 	 */
+		if (sync_page && is_writeble_pte(*shadow_pte))
+			goto set_pte;
 

What if *shadow_pte points at a different page? Is that possible?

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Marcelo Tosatti

The cost of hash table and memslot lookups are quite significant if the workload is pagetable write intensive resulting in increased mmu_lock contention.

Quoted 14 lines

arch/x86/kvm/mmu.c static int set_spte(struct kvm_vcpu *vcp · line 1603

Quoted 12 lines

arch/x86/kvm/mmu.c +8 -0

@@ -1593,7 +1593,16 @@ static int set_spte(struct kvm_vcpu *vcp
  		spte |= PT_WRITABLE_MASK;
 -		if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
+		/*
+ 	 	 * Optimization: for pte sync, if spte was writable the hash
+ 	 	 * lookup is unnecessary (and expensive). Write protection
+ 	 	 * is responsibility of mmu_get_page / kvm_sync_page.
+ 	 	 * Same reasoning can be applied to dirty page accounting.
+ 	 	 */
+		if (sync_page && is_writeble_pte(*shadow_pte))
+			goto set_pte;
 

What if *shadow_pte points at a different page? Is that possible?

To a different gfn? Then sync_page will have nuked the spte:

                if (gpte_to_gfn(gpte) != gfn || !is_present_pte(gpte) ||
                    !(gpte & PT_ACCESSED_MASK)) {
                        u64 nonpresent;
                        ..
                        set_shadow_pte(&sp->spt[i], nonpresent);
                }

Otherwise:

/*
* Using the cached information from sp->gfns is safe because:
* - The spte has a reference to the struct page, so the pfn for a given
* gfn can't change unless all sptes pointing to it are nuked first.

*shadow_pte can point to a different page if the guest updates pagetable, there is a fault before resync, the fault updates the spte with new gfn (and pfn) via mmu_set_spte. In which case the gfn cache is updated since:

                    } else if (pfn != spte_to_pfn(*shadow_pte)) {
                        printk("hfn old %lx new %lx\n",
                                 spte_to_pfn(*shadow_pte), pfn);
                        rmap_remove(vcpu->kvm, shadow_pte);

Quoted 10 lines · Marcelo Tosatti

*shadow_pte can point to a different page if the guest updates pagetable, there is a fault before resync, the fault updates the spte with new gfn (and pfn) via mmu_set_spte. In which case the gfn cache is updated since:

                    } else if (pfn != spte_to_pfn(*shadow_pte)) {
                        printk("hfn old %lx new %lx\n",
                                 spte_to_pfn(*shadow_pte), pfn);
                        rmap_remove(vcpu->kvm, shadow_pte);

Okay. Please resend but without the reversal of can_unsync, it will make a more readable patch. If you like, send a follow on that only does the reversal.

Here it goes.

KVM: MMU: optimize set_spte for page sync

The write protect verification in set_spte is unnecessary for page sync.

Its guaranteed that, if the unsync spte was writable, the target page does not have a write protected shadow (if it had, the spte would have been write protected under mmu_lock by rmap_write_protect before).

Same reasoning applies to mark_page_dirty: the gfn has been marked as dirty via the pagefault path.

The cost of hash table and memslot lookups are quite significant if the workload is pagetable write intensive resulting in increased mmu_lock contention.

Quoted 14 lines · Marcelo Tosatti

Here it goes.

KVM: MMU: optimize set_spte for page sync

The write protect verification in set_spte is unnecessary for page sync.

Its guaranteed that, if the unsync spte was writable, the target page does not have a write protected shadow (if it had, the spte would have been write protected under mmu_lock by rmap_write_protect before).

Same reasoning applies to mark_page_dirty: the gfn has been marked as dirty via the pagefault path.

Quoted 5 lines

The cost of hash table and memslot lookups are quite significant if the workload is pagetable write intensive resulting in increased mmu_lock contention.

Applied, thanks.

Do you have objections for submitting this patch for 2.6.28 ? The hash lookup kills performance of pagetable write + context switch intensive workloads.

Quoted 4 lines · Marcelo Tosatti

Do you have objections for submitting this patch for 2.6.28 ? The hash lookup kills performance of pagetable write + context switch intensive workloads.

Can you quantify?

2.6.28 is out of the question, 2.6.28.stable is possible, depending on the actual performance difference. I'd hate to introduce a correctness problem just to improve performance.