Re: [PATCH v2 18/22] mm/page_alloc: introduce ALLOC_NOBLOCK

[Brendan Jackman <jackmanb@google.com>]

On Wed May 13, 2026 at 9:43 AM UTC, Vlastimil Babka (SUSE) wrote:
> On 3/20/26 19:23, Brendan Jackman wrote:
>> This flag is set unless we can be sure the caller isn't in an atomic
>> context.
>> 
>> The allocator will soon start needing to call set_direct_map_* APIs
>> which cannot be called with IRQs off. It will need to do this even
>> before direct reclaim is possible.
>> 
>> Despite the fact that, in principle, ALLOC_NOBLOCK is distinct from
>> __GFP_DIRECT_RECLAIM, in order to avoid introducing a GFP flag, just
>> infer the former based on whether the caller set the latter. This means
>> that, in practice, ALLOC_NOBLOCK is just !__GFP_DIRECT_RECLAIM, except
>> that it is not influenced by gfp_allowed_mask. This could change later,
>> though.
>
> I don't think it should change later? We wouldn't want false positives
> during boot, or what do you have in mind?

I don't think I had anything specific in mind or any reason to _want_ to
change it. But I think (??) there are reasons to clear
__GFP_DIRECT_RECLAIM even if you are not atomic? Like some sort of
generalisation of __GFP_NOIO/NOFS. So all I'm getting at here is: I'm
using __GFP_DIRECT_RECLAIM to set ALLOC_NOBLOCK, but I think of that as
a total implementation detail and these two flags should conceptually be
decoupled.

> I wonder if the implementation of the "not influenced" is correct though...

This has been broken in several local iterations of this patchset so I
would not be surprised...

>> Call it ALLOC_NOBLOCK in order to try and mitigate confusion vs the
>> recently-removed ALLOC_NON_BLOCK, which meant something different.
>> 
>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>> ---
>>  mm/internal.h   |  1 +
>>  mm/page_alloc.c | 29 ++++++++++++++++++++++-------
>>  2 files changed, 23 insertions(+), 7 deletions(-)
>> 
>> diff --git a/mm/internal.h b/mm/internal.h
>> index cc19a90a7933f..865991aca06ea 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -1431,6 +1431,7 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
>>  #define ALLOC_HIGHATOMIC	0x200 /* Allows access to MIGRATE_HIGHATOMIC */
>>  #define ALLOC_TRYLOCK		0x400 /* Only use spin_trylock in allocation path */
>>  #define ALLOC_KSWAPD		0x800 /* allow waking of kswapd, __GFP_KSWAPD_RECLAIM set */
>> +#define ALLOC_NOBLOCK	       0x1000 /* Caller may be atomic */
>>  
>>  /* Flags that allow allocations below the min watermark. */
>>  #define ALLOC_RESERVES (ALLOC_HARDER|ALLOC_MIN_RESERVE|ALLOC_HIGHATOMIC|ALLOC_OOM)
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 9a07c552a1f8a..83d06a6db6433 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4608,6 +4608,8 @@ gfp_to_alloc_flags(gfp_t gfp_mask, unsigned int order)
>>  		(gfp_mask & (__GFP_HIGH | __GFP_KSWAPD_RECLAIM));
>>  
>>  	if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
>> +		alloc_flags |= ALLOC_NOBLOCK;
>
> When this is called from __alloc_pages_slowpath(), gfp_allowed_mask is
> already applied, so it will be influenced.

... yep. 

I have tried to generally refactor the flag setup in here to make
these kinda mistakes harder but I didn't have any good ideas (this was
when I spotted [0]). Maybe I was being too timid, I will try again.

[0] https://lore.kernel.org/linux-mm/20260331-b4-prepare_alloc_pages-flags-v1-1-ea2416def698@google.com/

>> +
>>  		/*
>>		 * Not worth trying to allocate harder for __GFP_NOMEMALLOC even
>>  		 * if it can't schedule.
>> @@ -4801,14 +4803,13 @@ check_retry_cpuset(int cpuset_mems_cookie, struct alloc_context *ac)
>>  
>>  static inline struct page *
>>  __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>> -						struct alloc_context *ac)
>> +		       struct alloc_context *ac, unsigned int alloc_flags)
>>  {
>>  	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
>>  	bool can_compact = can_direct_reclaim && gfp_compaction_allowed(gfp_mask);
>>  	bool nofail = gfp_mask & __GFP_NOFAIL;
>>  	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
>>  	struct page *page = NULL;
>> -	unsigned int alloc_flags;
>>  	unsigned long did_some_progress;
>>  	enum compact_priority compact_priority;
>>  	enum compact_result compact_result;
>> @@ -4860,7 +4861,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>  	 * kswapd needs to be woken up, and to avoid the cost of setting up
>>  	 * alloc_flags precisely. So we do that now.
>>  	 */
>> -	alloc_flags = gfp_to_alloc_flags(gfp_mask, order);
>> +	alloc_flags |= gfp_to_alloc_flags(gfp_mask, order);
>
> Is it safe to just combine them? You come with ALLOC_WMARK_LOW and combine
> with ALLOC_WMARK_MIN from gfp_to_alloc_flags() but these are not bit flags,
> I think you end up with ALLOC_WMARK_LOW effectively.

Ah, thanks, I do remember thinking about this and deciding that it was
safe but I probably just misunderstood the watermark code.

This makes me a bit more attracted to the idea of a struct like Gregory
suggested in [1]. Then this could be captured in the type system.

> Probably you need to pass the old alloc_flags to gfp_to_alloc_flags, mask
> only ALLOC_NOBLOCK from it and combine with newly calculated alloc_flags. By
> not recomputing ALLOC_NOBLOCK you also avoid the problem pointed out above?

Nice, thanks for the pointer.

> (or we decide to not use gfp flag but a new function and then it's more like
> what alloc_frozen_pages_nolock_noprof() does).