Re: [PATCH 1/5] x86/mm/pat: introcude cpa_lock() and cpa_unlock()

[Lorenzo Stoakes (ARM)]

Typo in subject :) introcude -> introduce

Hopefully Dave can fix up easily.

On Tue, Jul 28, 2026 at 04:07:44PM +0300, Mike Rapoport (Microsoft) wrote:
> The splitting and merging of kernel page table mappings between small and
> large is protected by cpa_lock.
>
> Commit 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on
> debug_pagealloc_enabled()") disabled gatig of cpa_lock on
> debug_pagealloc_enabled() to simplify the code presuming that skipping
> the lock when debug_pagealloc_enabled() was an optimization.
>
> However Lorenzo Stoakes notes that:
>
>   __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]
>
>   So you're spin locking in irq context here, which is probably not a
>   good idea.
>
> It would be possible to unconditionally use spin_lock_irqsave() and
> spin_unlock_irqrestore() but that would complicate locking rules even
> more.
>
> Restore gating of cpa_lock of debug_pagealloc_enabled(), but instead of
> putting the open-coded condition
>
> 	if (debug_pagealloc_enabled())
>
> before every lock and unlock operation, wrap the condition and the
> locking operation into cpa_lock() and cpa_unlock() helpers.

Yeah seems sensible.

>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>

LGTM, so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

> ---
>  arch/x86/mm/pat/set_memory.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index b1e780a465b5..4c8922695fd3 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -65,8 +65,25 @@ static const int cpa_warn_level = CPA_PROTECT;
>   * 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.
> + *
> + * When debug_pagealloc_enabled(), page attributes could be changed in atomic
> + * context that would warrant disabling IRQs. But since debug_pagealloc always
> + * uses 4k pages in the direct map there are no races for splits and collapses
> + * and locking can be just skipped altogether.
>   */
> -static DEFINE_SPINLOCK(cpa_lock);
> +static DEFINE_SPINLOCK(_cpa_lock);
> +
> +static inline void cpa_lock(void)
> +{
> +	if (!debug_pagealloc_enabled())
> +		spin_lock(&_cpa_lock);
> +}
> +
> +static inline void cpa_unlock(void)
> +{
> +	if (!debug_pagealloc_enabled())
> +		spin_unlock(&_cpa_lock);
> +}
>
>  #define CPA_FLUSHTLB 1
>  #define CPA_ARRAY 2
> @@ -417,7 +434,7 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa)
>  	int collapsed = 0;
>  	int i;
>
> -	spin_lock(&cpa_lock);
> +	cpa_lock();
>
>  	if (cpa->flags & (CPA_PAGES_ARRAY | CPA_ARRAY)) {
>  		for (i = 0; i < cpa->numpages; i++)
> @@ -433,7 +450,7 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa)
>  	}
>
>  	if (!collapsed) {
> -		spin_unlock(&cpa_lock);
> +		cpa_unlock();
>  		return;
>  	}
>
> @@ -444,7 +461,7 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa)
>  		pagetable_free(ptdesc);
>  	}
>
> -	spin_unlock(&cpa_lock);
> +	cpa_unlock();
>  }
>
>  static void cpa_flush(struct cpa_data *cpa, int cache)
> @@ -1239,9 +1256,9 @@ static int split_large_page(struct cpa_data *cpa, pte_t *kpte,
>  {
>  	struct ptdesc *ptdesc;
>
> -	spin_unlock(&cpa_lock);
> +	cpa_unlock();
>  	ptdesc = pagetable_alloc(GFP_KERNEL, 0);
> -	spin_lock(&cpa_lock);
> +	cpa_lock();
>  	if (!ptdesc)
>  		return -ENOMEM;
>
> @@ -2025,9 +2042,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;
>
> -		spin_lock(&cpa_lock);
> +		cpa_lock();
>  		ret = __change_page_attr(cpa, primary);
> -		spin_unlock(&cpa_lock);
> +		cpa_unlock();
>  		if (ret)
>  			goto out;
>
>
> --
> 2.53.0
>

Cheers, Lorenzo