Re: [PATCH v3 3/8] KVM: x86/mmu: Rename NX huge pages fields/functions for consistency

From: Sean Christopherson <seanjc@google.com>
To: Mingwei Zhang <mizhang@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	David Matlack <dmatlack@google.com>,
	Yan Zhao <yan.y.zhao@intel.com>, Ben Gardon <bgardon@google.com>
Subject: Re: [PATCH v3 3/8] KVM: x86/mmu: Rename NX huge pages fields/functions for consistency
Date: Mon, 15 Aug 2022 21:54:43 +0000	[thread overview]
Message-ID: <YvrAoyhgNzTcvzkU@google.com> (raw)
In-Reply-To: <YvhL6jKfKCj0+74w@google.com>

On Sun, Aug 14, 2022, Mingwei Zhang wrote:
> On Fri, Aug 05, 2022, Sean Christopherson wrote:
> > Rename most of the variables/functions involved in the NX huge page
> > mitigation to provide consistency, e.g. lpage vs huge page, and NX huge
> > vs huge NX, and also to provide clarity, e.g. to make it obvious the flag
> > applies only to the NX huge page mitigation, not to any condition that
> > prevents creating a huge page.
> > 
> > Leave the nx_lpage_splits stat alone as the name is ABI and thus set in
> > stone.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  8 ++--
> >  arch/x86/kvm/mmu/mmu.c          | 70 +++++++++++++++++----------------
> >  arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++----
> >  arch/x86/kvm/mmu/paging_tmpl.h  |  2 +-
> >  arch/x86/kvm/mmu/tdp_mmu.c      |  8 ++--
> >  5 files changed, 59 insertions(+), 51 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index e8281d64a431..5634347e5d05 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1143,7 +1143,7 @@ struct kvm_arch {
> >  	struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> >  	struct list_head active_mmu_pages;
> >  	struct list_head zapped_obsolete_pages;
> > -	struct list_head lpage_disallowed_mmu_pages;
> > +	struct list_head possible_nx_huge_pages;
> 
> Honestly, I am struggling to understand this one. 'possible_*' indicates
> that there are other possibilities. But what are those possibilities?

No, possible is being used as an adjective in this case.  possible_nx_huge_pages
is the list of shadow pages for which it's possible to replace the shadow page
with an NX huge page.

The noun version would yield a name like nx_huge_page_possiblities.

> I feel this name is more confusing than the original one. Maybe just keep

Ignoring lpage => huge_page, the current name is terribly inaccurate.  The list
doesn't contain all disallowed huge pages, nor does it even contain all disallowed
NX huge pages, it specifically tracks shadow pages that might be able to be
replaced with an NX huge page.

I'm open to other names, but whatever we choose should be paired with
account_nx_huge_page()'s param that is currently named "nx_huge_page_possible".