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

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

[Mike Rapoport]

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);
 	ptdesc = pagetable_alloc(GFP_KERNEL, 0);
-	if (!debug_pagealloc_enabled())
-		spin_lock(&cpa_lock);
+	spin_lock(&cpa_lock);
 	if (!ptdesc)
 		return -ENOMEM;
 
@@ -2023,11 +2020,9 @@ static int __change_page_attr_set_clr(struct cpa_data *cpa, int primary)
 		if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY))
 			cpa->numpages = 1;
 
-		if (!debug_pagealloc_enabled())
-			spin_lock(&cpa_lock);
+		spin_lock(&cpa_lock);
 		ret = __change_page_attr(cpa, primary);
-		if (!debug_pagealloc_enabled())
-			spin_unlock(&cpa_lock);
+		spin_unlock(&cpa_lock);
 		if (ret)
 			goto out;
 

base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
-- 
2.53.0

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

[Dave Hansen]

On 7/15/26 07:45, 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.

Thanks for sending this, Mike! I munged the changelog a bit and applied it.

[tip: x86/mm] x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()

[tip-bot2 for Mike Rapoport (Microsoft)]

The following commit has been merged into the x86/mm branch of tip:

Commit-ID:     5fce67641a3ed9a0782eaa228ddece526461a367
Gitweb:        https://git.kernel.org/tip/5fce67641a3ed9a0782eaa228ddece526461a367
Author:        Mike Rapoport (Microsoft) <rppt@kernel.org>
AuthorDate:    Wed, 15 Jul 2026 17:45:19 +03:00
Committer:     Dave Hansen <dave.hansen@linux.intel.com>
CommitterDate: Wed, 15 Jul 2026 08:00:49 -07:00

x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()

The splitting and merging of kernel page table mappings between small and
large is protected by cpa_lock. The merging is relatively new but the
splitting is ancient.

The splitting has a locking optimization: since DEBUG_PAGEALLOC forces all
mappings to 4k, there are no large pages to split. So the code that *might*
cause a split can just skip the locking (and a few other things).

This is entertaining, but it adds complexity and makes for weird locking
rules. Plus it's all for a debugging feature which makes the kernel super
slow in the first place. Optimizing something which is already super slow
and not used in production is not the best way to spend our complexity
budget.

Stop gating cpa_lock on debug_pagealloc_enabled() to simplify the code
and the locking rules.

[ dhansen: flesh out changelog ]

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Link: https://patch.msgid.link/20260715144519.934289-1-rppt@kernel.org
Link: https://lore.kernel.org/all/aab44f08-89f8-47fe-bee4-0ab6b25968c6@intel.com/
---
 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 45623d4..e9b4083 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);
 
@@ -1234,11 +1233,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);
 	ptdesc = pagetable_alloc(GFP_KERNEL, 0);
-	if (!debug_pagealloc_enabled())
-		spin_lock(&cpa_lock);
+	spin_lock(&cpa_lock);
 	if (!ptdesc)
 		return -ENOMEM;
 
@@ -2022,11 +2019,9 @@ static int __change_page_attr_set_clr(struct cpa_data *cpa, int primary)
 		if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY))
 			cpa->numpages = 1;
 
-		if (!debug_pagealloc_enabled())
-			spin_lock(&cpa_lock);
+		spin_lock(&cpa_lock);
 		ret = __change_page_attr(cpa, primary);
-		if (!debug_pagealloc_enabled())
-			spin_unlock(&cpa_lock);
+		spin_unlock(&cpa_lock);
 		if (ret)
 			goto out;
 

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

[Lorenzo Stoakes (ARM)]

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

>  	ptdesc = pagetable_alloc(GFP_KERNEL, 0);
> -	if (!debug_pagealloc_enabled())
> -		spin_lock(&cpa_lock);
> +	spin_lock(&cpa_lock);

-> _irqrestore() as below

>  	if (!ptdesc)
>  		return -ENOMEM;
>
> @@ -2023,11 +2020,9 @@ static int __change_page_attr_set_clr(struct cpa_data *cpa, int primary)
>  		if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY))
>  			cpa->numpages = 1;
>
> -		if (!debug_pagealloc_enabled())
> -			spin_lock(&cpa_lock);
> +		spin_lock(&cpa_lock);
>  		ret = __change_page_attr(cpa, primary);
> -		if (!debug_pagealloc_enabled())
> -			spin_unlock(&cpa_lock);
> +		spin_unlock(&cpa_lock);

__kernel_map_pages() can be called from irq context:

< GFP_ATOMIC context >
kfree() or whatever
-> ...
-> __free_pages_prepare()
-> debug_pagealloc_unmap_pages()
-> __kernel_map_pages()
-> __change_page_attr_set_clr()
-> cpa_lock spins [irqs off]

Sooo you're spin locking in irq context here, which is probably not a good idea.

All the cpa_lock spin locks have to be updated to reflect this.

So spin_lock_irqsave/restore I think?

But note that this turns Denis's cpa_lock patch ([0]) into a deadlock because
it's held across an IPI on TLB flush. So that has to be changed too, or possibly
dropped. I'll reply about that over there!

>  		if (ret)
>  			goto out;
>
>
> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
> --
> 2.53.0
>
>

Thanks, Lorenzo

[0]:https://lore.kernel.org/all/20260715183453.2381141-1-den@openvz.org/

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

[Mike Rapoport]

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

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

-- 
Sincerely yours,
Mike.

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

[Lorenzo Stoakes (ARM)]

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