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

[Mike Rapoport]

On Wed, Jul 29, 2026 at 04:48:38PM +0200, Peter Zijlstra wrote:
> 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

I meant static inline cpa_lock(), not removing it entirely.
With your comment why DEBUG_PAGEALLOC is special.

> 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();
 
Yes, but there might appear new callsites (I really hope not).
It's anyway really slow path so an extra if won't make a difference but
with it it's clearer that __kernel_map_pages() is for debug only.
 
> > >  	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);

-- 
Sincerely yours,
Mike.