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

On Tue, Jul 28, 2026 at 05:33:41PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 28, 2026 at 04:20:10PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Tue, Jul 28, 2026 at 05:01:26PM +0200, Peter Zijlstra wrote:
> > > On Tue, Jul 28, 2026 at 04:55:28PM +0200, Peter Zijlstra wrote:
> > > > On Tue, Jul 28, 2026 at 05:46:30PM +0300, Mike Rapoport wrote:
> > > > > On Tue, Jul 28, 2026 at 04:31:35PM +0200, Peter Zijlstra wrote:
> > > > > > On Tue, Jul 28, 2026 at 07:30:27AM -0700, Dave Hansen wrote:
> > > > > > > On 7/28/26 07:21, Peter Zijlstra wrote:
> > > > > > > > There was already a patch merged that removed the shole debug_pagealloc
> > > > > > > > exception. Is that not better?
> > > > > > >
> > > > > > > As I'm scanning through email this morning, there's another issue that
> > > > > > > popped up with that patch. It's causing hangs on boot.
> > > > > > >
> > > > > > > It's looking like debug pagealloc not taking the lock is actually
> > > > > > > functional, not an optimization. Although, I hesitate to say
> > > > > > > "functional" and would prefer to use much less nice words to describe it.
> > > > > >
> > > > > > Yeah, lets figure out why that is before we retain this wart ;-)
> > > > >
> > > > > As Lorenzo said:
> > > > >
> > > > >   __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
> > > > >
> > > >
> > > > The TLBI hack in __kernel_map_pages() makes me wonder how any of this is
> > > > correct to begin with. That comment isn't helping.
> > > >
> > >
> > > Also, the 'atomic context' usage hereabout seems confused. In particular
> > > the issue is with IRQ-disabled context, they are not the same thing.
> > 
> > I mean yeah the IRQs being off is the issue with holding the lock over an IPI, a
> > softirq allocating GFP_ATOMIC won't be a problem for that, but I think all the
> > conclusions are still the same.
> 
> preempt_disable() is an 'atomic context', but does not present the
> problem.
>
> Anyway, yes not saying the conclusions are wrong, just that the wording
> is confusing at heck.

How would you like to word this? 

Is this one better?

/*
 * When debug_pagealloc_enabled(), page attributes could be changed in a
 * context with IRQs disabled, so using spin_lock() with debug_pagealloc can
 * cause a deadlock. 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.
 */

-- 
Sincerely yours,
Mike.