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@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx/