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

From: Mingwei Zhang <mizhang@google.com>
To: Sean Christopherson <seanjc@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: Thu, 18 Aug 2022 22:13:07 +0000	[thread overview]
Message-ID: <Yv65c/t23GqpLPg3@google.com> (raw)
In-Reply-To: <Yv0Tk0WAdxymSyUt@google.com>

On Wed, Aug 17, 2022, Sean Christopherson wrote:
> On Tue, Aug 16, 2022, Mingwei Zhang wrote:
> > 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@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.
> > 
> > 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?
> 
> Yes, they are shadow pages that the NX recovery thread should zap, but the reason
> they should be zapped is because (a) the shadow page has at least one execute child
> SPTE, (b) zapping the shadow page will also zap its child SPTEs, and (c) eliminating
> all executable child SPTEs means KVM _might_ be able to instantiate an NX huge page.
> 

oh, I scratched my head and finaly got your point. hmm. So the shadow
pages are the 'blockers' to (re)create a NX huge page because of at
least one present child executable spte. So, really, whether these
shadow pages themselves are NX huge or not does not really matter. All
we need to know is that they will be zapped in the future to help making
recovery of an NX huge page possible.

> > `nx_huge_page_disallowed` is easy to understand because it literally say
> > 'nx_huge_page is not allowed', which is correct.
> 
> No, it's not correct.  The list isn't simply the set of shadow pages that disallow
> NX huge pages, it's the set of shadow pages that disallow NX huge pages _and_ that
> can possibly be replaced by an NX huge page if the shadow page and all its
> (executable) children go away.
> 

hmm, I think this naming is correct. The flag is used to talk to the
'fault handler' to say 'hey, don't create nx huge page, stupid'. Of
course, it is also used to by the 'nx huge recovery thread', but the
recovery thread will only check it for sanity purpose, which really does
not matter, i.e., the thread will zap the pages anyway.

> > But this one, it says 'possible nx_huge_pages', but they are not
> > nx huge pages at all.
> 
> Yes, but they _can be_ NX huge pages, hence the "possible".  A super verbose name
> would be something like mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages.
> 

I can make a dramtic example as why 'possible' may not help:

/* Flag that decides something important. */
bool possible_one;

The information we (readers) gain from reading the above is _0_.

With that, since you already mentioned the name:
'mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages',
why can't we shorten it by using 'mmu_pages_to_recover_nx_huge' or
'pages_to_recover_nx_huge'? 'recover' is the word that immediately
connects with the 'recovery thread', which I think makes more sense on
readability.

> > 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,
> 
> "replaced" is past tense in this usage, and these are definitely not shadow pages
> that have already been replaced by NX huge pages.
> 
> > mmu_pages_replaced_by_possible_nx_huge or something starting with
> 
> Same issue with "replaced".
> 
> > possible_pages_, pages_ instead of possible_nx_huge_pages?
> 
> Reprhasing, I think you're asking that the variable have "mmu_pages" somewhere in
> the name to clarify that it's a list of kvm_mmu_page structs.  I agree that ideally
> it would have "mmu_pages" somewhere in there, but I also think that having both
> "nx_huge_pages" and "mmu_pages" in the name makes it unreadable, i.e. we have to
> choose one use of "pages".
> 
> The only alternative I can come up with that isn't completely gross is
> possible_nx_huge_mmu_pages.  I can't tell if that's less confusing or more
> confusing.

Aside from that.

Reviewed-by: Mingwei Zhang <mizhang@google.com>