Re: [PATCH] kvm: mmu: lazy collapse small sptes into large sptes

[Wanpeng Li]

On Fri, Apr 03, 2015 at 12:25:14PM +0800, Xiao Guangrong wrote:
>
>
>On 03/30/2015 07:48 AM, Wanpeng Li wrote:
>>There are two scenarios for the requirement of collapsing small sptes
>>into large sptes.
>>- dirty logging tracks sptes in 4k granularity, so large sptes are splitted,
>>   the large sptes will be reallocated in the destination machine and the
>>   guest in the source machine will be destroyed when live migration successfully.
>>   However, the guest in the source machine will continue to run if live migration
>>   fail due to some reasons, the sptes still keep small which lead to bad
>>   performance.
>>- our customers write tools to track the dirty speed of guests by EPT D bit/PML
>>   in order to determine the most appropriate one to be live migrated, however
>>   sptes will still keep small after tracking dirty speed.
>>
>>This patch introduce lazy collapse small sptes into large sptes, the memory region
>>will be scanned on the ioctl context when dirty log is stopped, the ones which can
>>be collapsed into large pages will be dropped during the scan, it depends the on
>>later #PF to reallocate all large sptes.
>>
>>Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxxxxxx>
>>---
>>  arch/x86/include/asm/kvm_host.h |  2 ++
>>  arch/x86/kvm/mmu.c              | 66 +++++++++++++++++++++++++++++++++++++++++
>>  arch/x86/kvm/x86.c              |  5 ++++
>>  3 files changed, 73 insertions(+)
>>
>>diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>index a236e39..73de5d3 100644
>>--- a/arch/x86/include/asm/kvm_host.h
>>+++ b/arch/x86/include/asm/kvm_host.h
>>@@ -859,6 +859,8 @@ void kvm_mmu_set_mask_ptes(u64 user_mask, u64 accessed_mask,
>>  void kvm_mmu_reset_context(struct kvm_vcpu *vcpu);
>>  void kvm_mmu_slot_remove_write_access(struct kvm *kvm,
>>  				      struct kvm_memory_slot *memslot);
>>+void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm,
>>+					struct kvm_memory_slot *memslot);
>>  void kvm_mmu_slot_leaf_clear_dirty(struct kvm *kvm,
>>  				   struct kvm_memory_slot *memslot);
>>  void kvm_mmu_slot_largepage_remove_write_access(struct kvm *kvm,
>>diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>index cee7592..d25ced1 100644
>>--- a/arch/x86/kvm/mmu.c
>>+++ b/arch/x86/kvm/mmu.c
>>@@ -4465,6 +4465,72 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm,
>>  		kvm_flush_remote_tlbs(kvm);
>>  }
>>
>>+static int kvm_mmu_zap_collapsible_spte(struct kvm *kvm,
>>+		unsigned long *rmapp)
>
>Can use 'bool' instead of 'int'. You used 'bool' in
>kvm_mmu_zap_collapsible_sptes anyway.
>
>>+{
>>+	u64 *sptep;
>>+	struct rmap_iterator iter;
>>+	int need_tlb_flush = 0;
>>+	pfn_t pfn;
>>+	struct kvm_mmu_page *sp;
>>+
>>+	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
>>+		BUG_ON(!(*sptep & PT_PRESENT_MASK));
>>+
>>+		sp = page_header(__pa(sptep));
>>+		pfn = spte_to_pfn(*sptep);
>>+		if (sp->role.direct &&
>
>It only works on direct mapping, please drop a comment to explain
>why.
>
>>+			!kvm_is_reserved_pfn(pfn) &&
>>+			PageTransCompound(pfn_to_page(pfn))) {
>>+			drop_spte(kvm, sptep);
>>+			need_tlb_flush = 1;
>>+		}
>>+		sptep = rmap_get_next(&iter);
>
>You can not get the next spte after drop the current spte. Please
>refer to kvm_unmap_rmapp().
>
>>+	}
>>+
>>+	return need_tlb_flush;
>>+}
>>+
>>+void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm,
>>+			struct kvm_memory_slot *memslot)
>>+{
>>+	bool flush = false;
>>+	unsigned long *rmapp;
>>+	unsigned long last_index, index;
>>+	gfn_t gfn_start, gfn_end;
>>+
>>+	spin_lock(&kvm->mmu_lock);
>>+
>>+	gfn_start = memslot->base_gfn;
>>+	gfn_end = memslot->base_gfn + memslot->npages - 1;
>>+
>>+	if (gfn_start >= gfn_end)
>>+		goto out;
>>+
>>+	rmapp = memslot->arch.rmap[0];
>>+	last_index = gfn_to_index(gfn_end, memslot->base_gfn,
>>+					PT_PAGE_TABLE_LEVEL);
>>+
>>+	for (index = 0; index <= last_index; ++index, ++rmapp) {
>>+		if (*rmapp)
>>+			flush |= kvm_mmu_zap_collapsible_spte(kvm, rmapp);
>>+
>>+		if (need_resched() || spin_needbreak(&kvm->mmu_lock)) {
>>+			if (flush) {
>>+				kvm_flush_remote_tlbs(kvm);
>>+				flush = false;
>>+			}
>>+			cond_resched_lock(&kvm->mmu_lock);
>>+		}
>>+	}
>>+
>>+	if (flush)
>>+		kvm_flush_remote_tlbs(kvm);
>>+
>>+out:
>>+	spin_unlock(&kvm->mmu_lock);
>>+}
>>+
>>  void kvm_mmu_slot_leaf_clear_dirty(struct kvm *kvm,
>>  				   struct kvm_memory_slot *memslot)
>>  {
>>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>index c5f7e03..6037389 100644
>>--- a/arch/x86/kvm/x86.c
>>+++ b/arch/x86/kvm/x86.c
>>@@ -7618,6 +7618,11 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>>  	/* It's OK to get 'new' slot here as it has already been installed */
>>  	new = id_to_memslot(kvm->memslots, mem->slot);
>>
>>+	if ((change != KVM_MR_DELETE) &&
>>+		(old->flags & KVM_MEM_LOG_DIRTY_PAGES) &&
>>+		!(new->flags & KVM_MEM_LOG_DIRTY_PAGES))
>>+		kvm_mmu_zap_collapsible_sptes(kvm, new);
>>+
>
>You'd better drop comments here to explain the situation.

Just handle all your comments in v2, thanks for your review. ;)

Regards,
Wanpeng Li 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/