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

[Harry Yoo <harry@kernel.org>]

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()?

> 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.

-- 
Cheers,
Harry / Hyeonggon