Re: [PATCH 3/3] x86/mm: Fix and document DEBUG_PAGEALLOC

[Peter Zijlstra]

On Wed, Jul 29, 2026 at 05:13:55PM +0300, Mike Rapoport wrote:
> On Wed, Jul 29, 2026 at 01:08:10PM +0200, Peter Zijlstra wrote:
> > It turns out that commit 5fce67641a3e ("x86/mm/pat: Don't gate
> > cpa_lock on debug_pagealloc_enabled()") was a little too quick to
> > remove the debug_pagealloc exception for cpa_lock.
> > 
> > Notably __kernel_map_pages() is used by the page-allocator from any
> > context the page-allocator itself is used, which violates the cpa_lock
> > rules.
> > 
> > Re-instate the exception, except make it specific to the
> > __kernel_map_pages() such that any other cpa() usage is still fully
> > serialized by cpa_lock. Also note that since cpa() should not be used
> > on memory that isn't allocated, the page-allocator locking and cpa are
> > infact mutually exclusive and all cpa usage in fully serialized.
> > 
> > Add a comment explaining this and other 'funnies' surrounding
> > DEBUG_PAGEALLOC, including how pgd_lock is not affected and the TLB
> > trickery.
> > 
> > Fixes: 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()")
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> >
> > +	/*
> > +	 * DEBUG_PAGEALLOC is special; it is called from any context the
> > +	 * page-allocator is, which violates the normal cpa_lock locking
> > +	 * rules.
> > +	 *
> > +	 * However, since it is part of the page-allocator, things are still
> > +	 * properly serialized by the page-allocator locking and the fact that
> > +	 * when a page is owned by the page-allocator, it isn't owned by
> > +	 * anybody else. That is, you *SHOULD* not be calling cpa() on memory
> 
>                                      *SHOULD NOT* ?

Well yeah, d'0h.

> > +	 * that isn't allocated.
> > +	 *
> > +	 * Additionally, DEBUG_PAGEALLOC ensures (per probe_page_size_mask())
> > +	 * that the kernel mapping is 4k pages, therefore there are no large
> > +	 * pages to split/collapse.
> > +	 *
> > +	 * Furthermore, the page-allocator strictly manages pages that
> > +	 * *exist*, avoiding pgd_lock.
> > +	 *
> > +	 * Therefore, it is safe to not take cpa_lock.
> > +	 */
> > +	if (debug_pagealloc_enabled() && (cpa->flags & CPA_DEBUG_PAGEALLOC))
> > +		lock = false;
> > +
> >  	while (rempages) {
> >  		/*
> >  		 * Store the remaining nr of pages for the large page
> > @@ -2008,9 +2033,12 @@ static int __change_page_attr_set_clr(st
> >  		if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY))
> >  			cpa->numpages = 1;
> >  
> > -		spin_lock(&cpa_lock);
> > -		ret = __change_page_attr(cpa, primary);
> > -		spin_unlock(&cpa_lock);
> > +		if (lock) {
> > +			guard(spinlock)(&cpa_lock);
> > +			ret = __change_page_attr(cpa, primary);
> > +		} else {
> > +			ret = __change_page_attr(cpa, primary);
> > +		}
> 
> This does make DEBUG_PAGEALLOC exception more explicit *here*, but OTOH the
> spin_(un)lock(&cpa_lock) in split_large_page() becomes confusing.

So as the comment states, with DEBUG_PAGEALLOC there are no large pages,
so you should never hit split_large_page().

It is the same as pgd_lock; that isn't guarded anywhere either, and
works by the same reasons; DEBUG_PAGEALLOC isn't ever supposed to hit
those paths.

> I like my version with your comments added there more as it localizes the
> DEBUG_PAGEALLOC exception in the lock wrappers.

So I don't like removing cpa_lock entirely; it is still serializing cpa
usage, even though it isn't as critical on 4k only. Having cpa behave
significantly different for DEBUG_PAGEALLOC just seems like a very dodgy
situation.

And again, pdg_lock is in the same spot. It all works because the code
'magically' never hits the pgd_lock taking paths.

> >  		if (ret)
> >  			goto out;
> >  
> > @@ -2661,15 +2689,23 @@ void __kernel_map_pages(struct page *pag
> >  	 * and hence no memory allocations during large page split.
> >  	 */
> 
> I'd also return early and maybe even WARN if !debug_pagealloc_enabled(). 

The callsites be like:

	if (debug_pagealloc_enabled_static())
		__kernel_map_pages();


> >  	if (enable)
> > -		__set_pages_p(page, numpages);
> > +		__set_pages_p(page, numpages, CPA_DEBUG_PAGEALLOC);
> >  	else
> > -		__set_pages_np(page, numpages);
> > +		__set_pages_np(page, numpages, CPA_DEBUG_PAGEALLOC);