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

[Mingwei Zhang]

On Mon, Aug 15, 2022, Sean Christopherson wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >  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.

Right, but these shadow pages are not NX huge pages, right? IIUC, they
are pages to be zapped due to NX huge pages, aren't they?

`nx_huge_page_disallowed` is easy to understand because it literally say
'nx_huge_page is not allowed', which is correct.

But this one, it says 'possible nx_huge_pages', but they are not
nx huge pages at all. Instead, they are 'shadow pages that are replaced
with nx_huge_pages'. So that's why updates to this list is done together
with stats nx_plage_splits.

Please correct me if I am wrong. I am still struggling to understand the
meaning of these variables.

>
> > 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".

How about mmu_pages_replaced_by_nx_huge,
mmu_pages_replaced_by_possible_nx_huge or something starting with
possible_pages_, pages_ instead of possible_nx_huge_pages?