Re: [PATCH v2] KVM: Avoid zapping unrelated shadows in __kvm_set_memory_region()

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Apr 10, 2012 at 10:05:03PM +0900, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> 
> We do not need to zap all shadow pages of the guest when we create or
> destroy a slot in this function.
> 
> To change this, we make kvm_mmu_zap_all()/kvm_arch_flush_shadow()
> zap only those which have mappings into a given slot.
> 
> The way we iterate through active shadow pages is also changed to avoid
> checking unrelated pages again and again.
> 
> Furthermore, the condition to see if we have any mmio sptes to clear is
> changed so that we will not do flush for newly created slots.
> 
> With all these changes applied, the total amount of time needed to flush
> shadow pages of a usual Linux guest, running Fedora with 4GB memory,
> during a shutdown was reduced from 90ms to 60ms.
> 
> Furthermore, the total number of flushes needed to boot and shutdown
> that guest was also reduced from 52 to 31.

Wow. Also it avoids gazillion faults which slow down the guest.

> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 29ad6f9..a50f7ba 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3930,16 +3930,30 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
>  	kvm_flush_remote_tlbs(kvm);
>  }
>  
> -void kvm_mmu_zap_all(struct kvm *kvm)
> +/**
> + * kvm_mmu_zap_all - zap all shadows which have mappings into a given slot
> + * @kvm: the kvm instance
> + * @slot: id of the target slot
> + *
> + * If @slot is -1, zap all shadow pages.
> + */
> +void kvm_mmu_zap_all(struct kvm *kvm, int slot)
>  {

kvm_mmu_zap_all(struct kvm *kvm)
and
kvm_mmu_zap_slot(struct kvm *kvm, int slot)

are more descriptive names

>  	kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
>  
> -	/*
> -	 * If the new memory slot is created, we need to clear all
> -	 * mmio sptes.
> -	 */
> -	if (npages && old.base_gfn != mem->guest_phys_addr >> PAGE_SHIFT)
> -		kvm_arch_flush_shadow(kvm);
> +	/* Need to clear all mmio sptes used before. */
> +	if (npages && old.npages && base_gfn != old.base_gfn)
> +		kvm_arch_flush_shadow(kvm, mem->slot);

mmio sptes are not related to any particular slot (by definition),
i think you misunderstood the purpose of this flush. It handles

1) [start, end] gpa range unmapped.
2) mmio sptes are created that map this range.
3) a new slot is created. mmio sptes that map [start, end] range,
which is now backed by new memslot should be removed.

Perhaps there is an efficient way to clear all mmio sptes, or all
mmio sptes pointing to [start, end] (instead of all sptes), here.