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

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.
> 
> Signed-off-by: Mike Rapoport (Microsoft) <[email protected]>
> ---
>  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);
> +}

There was already a patch merged that removed the shole debug_pagealloc
exception. Is that not better?