Re: [PATCH v4 7/7] KVM: x86/mmu: Lazily allocate memslot rmaps

From: Sean Christopherson <seanjc@google.com>
To: Ben Gardon <bgardon@google.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>,
	Peter Shier <pshier@google.com>,
	Yulei Zhang <yulei.kernel@gmail.com>,
	Wanpeng Li <kernellwp@gmail.com>,
	Xiao Guangrong <xiaoguangrong.eric@gmail.com>,
	Kai Huang <kai.huang@intel.com>,
	Keqian Zhu <zhukeqian1@huawei.com>,
	David Hildenbrand <david@redhat.com>
Subject: Re: [PATCH v4 7/7] KVM: x86/mmu: Lazily allocate memslot rmaps
Date: Tue, 11 May 2021 20:04:27 +0000	[thread overview]
Message-ID: <YJrjS/83f7H10HO7@google.com> (raw)
In-Reply-To: <YJrjAt5eyCZQNSkM@google.com>

On Tue, May 11, 2021, Sean Christopherson wrote:
> On Tue, May 11, 2021, Ben Gardon wrote:
> > If the TDP MMU is in use, wait to allocate the rmaps until the shadow
> > MMU is actually used. (i.e. a nested VM is launched.) This saves memory
> > equal to 0.2% of guest memory in cases where the TDP MMU is used and
> > there are no nested guests involved.
> > 
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  2 ++
> >  arch/x86/kvm/mmu/mmu.c          | 53 +++++++++++++++++++++++----------
> >  arch/x86/kvm/mmu/tdp_mmu.c      |  6 ++--
> >  arch/x86/kvm/mmu/tdp_mmu.h      |  4 +--
> >  arch/x86/kvm/x86.c              | 45 +++++++++++++++++++++++++++-
> >  5 files changed, 89 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index fc75ed49bfee..7b65f82ade1c 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1868,4 +1868,6 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
> >  
> >  int kvm_cpu_dirty_log_size(void);
> >  
> > +int alloc_all_memslots_rmaps(struct kvm *kvm);
> > +
> >  #endif /* _ASM_X86_KVM_HOST_H */
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index b0bdb924d519..183afccd2944 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1190,7 +1190,8 @@ static void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
> >  		kvm_tdp_mmu_clear_dirty_pt_masked(kvm, slot,
> >  				slot->base_gfn + gfn_offset, mask, true);
> >  
> > -	if (!kvm->arch.memslots_have_rmaps)
> > +	/* Read memslots_have_rmaps before the rmaps themselves */
> 
> IIRC, you open coded reading memslots_have_rmaps because of a circular
> dependency, but you can solve that simply by defining the helper in mmu.h
> instead of kvm_host.h.
> 
> And I think you could even make it static in mmu.c and omit the smp_load_acuquire
> from the three users in x86.c, though that's probably not worth it.
> 
> Either way, reading the same comment over and over and over, just to make
> checkpatch happy, gets more than a bit tedious.
> 
> That would also allow you to elaborate on why the smp_load_acquire() is
> necessary, and preferably what it pairs with.

Belated thought: you could also introduce the helper in patch 06 in order to
miminize thrash in this patch.