Re: [PATCH] x86/mm/pat: don't gate cpa_lock on debug_pagealloc_enabled()

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Wed, Jul 22, 2026 at 11:51:56AM +0300, Mike Rapoport wrote:
> On Tue, Jul 21, 2026 at 04:49:13PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Wed, Jul 15, 2026 at 05:45:19PM +0300, Mike Rapoport wrote:
> > > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> > >
> > > Dave Hansen says:
> > >   My only question is *why*!?!? Why add extra locking complexity and rules
> > >   to optimize debug_pagealloc, which is already horrendously slow.
> > >
> > > Stop gating cpa_lock on debug_pagealloc_enabled() to simplify the code.
> > >
> > > Suggested-by: Dave Hansen <dave.hansen@intel.com>
> > > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> > > ---
> > >  arch/x86/mm/pat/set_memory.c | 19 +++++++------------
> > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > > index d023a40a1e03..e8316f5ffa8a 100644
> > > --- a/arch/x86/mm/pat/set_memory.c
> > > +++ b/arch/x86/mm/pat/set_memory.c
> > > @@ -62,10 +62,9 @@ enum cpa_warn {
> > >  static const int cpa_warn_level = CPA_PROTECT;
> > >
> > >  /*
> > > - * Serialize cpa() (for !DEBUG_PAGEALLOC which uses large identity mappings)
> > > - * using cpa_lock. So that we don't allow any other cpu, with stale large tlb
> > > - * entries change the page attribute in parallel to some other cpu
> > > - * splitting a large page entry along with changing the attribute.
> > > + * Serialize cpa() using cpa_lock so that we don't allow any other cpu, with
> > > + * stale large tlb entries, to change the page attribute in parallel to some
> > > + * other cpu splitting a large page entry along with changing the attribute.
> > >   */
> > >  static DEFINE_SPINLOCK(cpa_lock);
> > >
> > > @@ -1235,11 +1234,9 @@ static int split_large_page(struct cpa_data *cpa, pte_t *kpte,
> > >  {
> > >  	struct ptdesc *ptdesc;
> > >
> > > -	if (!debug_pagealloc_enabled())
> > > -		spin_unlock(&cpa_lock);
> > > +	spin_unlock(&cpa_lock);
> >
> > -> _irqsave() is needed I think :) see below
>
> I'm rather thinking that it will be simpler overall to restore the if, but
> wrap it in cpa_lock()/cpa_unlock() helpers with a better comment than what
> we have now above DEFINE_SPINLOCK(cpa_lock).

Right yeah, like cpa_[un]lock() that has the conditional in it and explains
why.

But that does leave us with unfortunate races too if you allow collapse I
think :)

So if reinstated and we retain Denis's patch, his should gate the collapse
on debug pagealloc again.

Oh what a web we have spun...

>
> > >  	ptdesc = pagetable_alloc(GFP_KERNEL, 0);
> > > -	if (!debug_pagealloc_enabled())
> > > -		spin_lock(&cpa_lock);
> > > +	spin_lock(&cpa_lock);
> >
> > -> _irqrestore() as below
>
> --
> Sincerely yours,
> Mike.

Cheers, Lorenzo