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

["Brendan Jackman" <brendan.jackman@linux.dev>]

On Fri Jun 19, 2026 at 3:56 AM UTC, Matthew Wilcox wrote:
> On Wed, Jun 17, 2026 at 03:29:42PM +0000, Brendan Jackman wrote:
>> +++ b/mm/page_alloc.c
>> @@ -5253,24 +5253,98 @@ void free_pages_bulk(struct page **page_array, unsigned long nr_pages)
>>  	}
>>  }
>>  
>> -/*
>> - * This is the 'heart' of the zoned buddy allocator.
>> - */
>> -struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>> -		int preferred_nid, nodemask_t *nodemask)
>> +static inline bool alloc_order_allowed(gfp_t gfp, unsigned int order,
>> +				       unsigned int alloc_flags)
>>  {
>> -	struct page *page;
>> -	unsigned int alloc_flags = ALLOC_WMARK_LOW;
>> -	gfp_t alloc_gfp; /* The gfp_t that was actually used for allocation */
>> -	struct alloc_context ac = { };
>> +
>
> Spurious blank line?

Yep, thanks.

>> +	if (alloc_flags & ALLOC_TRYLOCK)
>> +		return pcp_allowed_order(order);
> [...]
>> +/*
>> + * GFP flags to set for ALLOC_TRYLOCK i.e. alloc_pages_nolock().
>> + *
>> + * Do not specify __GFP_DIRECT_RECLAIM, since direct claim is not allowed.
>> + * Do not specify __GFP_KSWAPD_RECLAIM either, since wake up of kswapd
>> + * is not safe in arbitrary context.
>> + *
>> + * These two are the conditions for gfpflags_allow_spinning() being true.
>> + *
>> + * Specify __GFP_NOWARN since failing alloc_pages_nolock() is not a reason
>> + * to warn. Also warn would trigger printk() which is unsafe from
>> + * various contexts. We cannot use printk_deferred_enter() to mitigate,
>> + * since the running context is unknown.
>> + *
>> + * Specify __GFP_ZERO to make sure that call to kmsan_alloc_page() below
>> + * is safe in any context. Also zeroing the page is mandatory for
>> + * BPF use cases.
>
> It may be mandatory for BPF, but it seems wasteful for other uses.

True, don't see why we shouldn't push this out to the caller, I can do
it as part of this series.

>> + * Though __GFP_NOMEMALLOC is not checked in the code path below,
>> + * specify it here to highlight that alloc_pages_nolock()
>> + * doesn't want to deplete reserves.
>> + */
>> +static const gfp_t gfp_trylock = __GFP_NOWARN | __GFP_ZERO | __GFP_NOMEMALLOC |
>> +				__GFP_COMP;

> I rather dislike this being turned into a file-scope variable, even a
> non-varying variable.  Can't it remain inside a function?

Um, we could put it into a function like `void add_gfp_trylock(gfp_t *gfp)` 
but that doesn't really reduce the scope in any meaninful way, right? 

We could also squash it into what's currently called
`alloc_trylock_allowed` but then it's a bit of a mush function, would it
be called `do_trlock_stuff`?

Putting it directly into __alloc_frozen_pages_noprof() would make that
function too big IMO, and its real estate would be dominated by trylock
stuff.

It's definitely understandable to find the large variable scope yucky
but IMO the real fix for that would be to break up page_alloc.c, which
I don't really want to do in this series. 

>> +/*
>> + * This is the 'heart' of the zoned buddy allocator.
>> + */
>> +struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>> +		int preferred_nid, nodemask_t *nodemask, unsigned int alloc_flags)
>> +{
>> +	struct page *page;
>> +	gfp_t alloc_gfp; /* The gfp_t that was actually used for allocation */
>> +	struct alloc_context ac = { };
>> +
>> +	/* Other flags could be supported later if needed. */
>> +	if (WARN_ON(alloc_flags & ~ALLOC_TRYLOCK))
>>  		return NULL;
>>  
>> +	if (!alloc_order_allowed(gfp, order, alloc_flags))
>> +		return NULL;
>> +
>> +	if (alloc_flags & ALLOC_TRYLOCK) {
>> +		VM_WARN_ON_ONCE(gfp & ~__GFP_ACCOUNT);
>
> So the only GFP flag the user is allowed to specify is __GFP_ACCOUNT?
> That seems bogus; other flags would be reasonable including all the ones
> in gfp_trylock, as well as GFP_HIGHMEM, GFP_DMA, GFP_MOVABLE, GFP_HARDWALL.

Definitely makes sense for the ones in gfp_trylock.

For the others, I'm not sure - this "nolock" functionality is a bit
weird and sketchy, I suspect the reason for the WARN here is "let's make
sure we have a proper think before we allow it to grow usecases that are
meaningfully different from the other ones". I think I like that
conservatism here, I would lean towards keeping it? Not a passionately
held opinion though.