Re: [PATCH v3 05/16] mm/page_alloc: unify __alloc_frozen_pages[_nolock]_noprof()

[Brendan Jackman]

On Wed Jul 1, 2026 at 2:21 AM UTC, Harry Yoo wrote:
>
>
> On 7/1/26 2:04 AM, Brendan Jackman wrote:
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index a3ba63c7f9199..8d409d075e3e9 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -5271,24 +5271,98 @@ void free_pages_bulk(struct page **page_array, unsigned long nr_pages)
>>>>  	}
>>>>  }
>>>>  
>>>> +static inline bool alloc_trylock_allowed(void)
>>>> +{
>>>> +	/*
>>>> +	 * In PREEMPT_RT spin_trylock() will call raw_spin_lock() which is
>>>> +	 * unsafe in NMI. If spin_trylock() is called from hard IRQ the current
>>>> +	 * task may be waiting for one rt_spin_lock, but rt_spin_trylock() will
>>>> +	 * mark the task as the owner of another rt_spin_lock which will
>>>> +	 * confuse PI logic, so return immediately if called from hard IRQ or
>>>> +	 * NMI.
>>>> +	 *
>>>> +	 * Note, irqs_disabled() case is ok. This function can be called
>>>> +	 * from raw_spin_lock_irqsave region.
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_PREEMPT_RT) && (in_nmi() || in_hardirq()))
>>>> +		return false;
>>>> +
>>>> +	/* On UP, spin_trylock() always succeeds even when it is locked */
>>>> +	if (!IS_ENABLED(CONFIG_SMP) && in_nmi())
>>>> +		return false;
>>>
>>> Except for deferred_pages_enabled(), it's not specific to the page
>>> allocator. SLUB has
>>>
>>> 	/*
>>> 	 * See the comment for the same check in
>>> 	 * alloc_frozen_pages_nolock_noprof()
>>> 	 */
>>>
>>> ... and repeats the same thing as above.
>>>
>>> Perhaps let's factor it out into a helper
>>> rather than trying not to forget to update the other place?
>> 
>> Hm, not sure about this. I think I would say it's a "coincidence" that
>> these two bits of code look the same? Like, page_alloc.c uses
>> spin_trylock() so you can't do alloc_pages_nolock() from IRQ on
>> PREEMPT_RT. slub.c ALSO uses spin_trylock(), so you ALSO can't use
>> kmalloc_nolock() in those scenarios. But those are two different facts
>> that just happen to be isomorphic? Putting them into a shared helper
>> would kinda imply that these are part of a single system with inherently
>> coupled constraints.
>
> But as long as they use spinlocks and can be called in unknown contexts,
> they are supposed to have the same constraint?
>
> And actually, the only reason free_pages_nolock() or kfree_nolock()
> don't have these checks is just because we don't allow kmalloc() ->
> kfree_nolock() or alloc_pages() -> free_pages_nolock(). Once we allow
> them, we'll need to repeat this again.
>
> I think it doesn't even belong page/slab allocators,
> probably should be spin_trylock_allowed()?

You are right. I was imagining a function called alloc_nolock_allowed()
but actually now I see the name spin_trylock_allowed() I realise it's
not about the allocators (which both just happen to use spin_trylock())
at all, it makes perfect sense.

>> But I'd lean towards leaving this out of> the patchset since the
> potential deduplication isn't really related to
>> the other cleanups anyway.
>
> Ack.

Patchset is getting pretty big already but most stuff is already acked
and also most of the patches are trivial so maybe I'll tack it on after
all...