Re: [PATCH v4 1/8] mm/page_alloc: reset can_retry_reserves when the slowpath restarts

[Vlastimil Babka (SUSE)]

On 10/28/26 22:19, Gregory Price (Meta) wrote:
> Commit 2c4c3e29897d ("mm/page_alloc: simplify __alloc_pages_slowpath()
> flow") retries an allocation once with reserve_flags when the caller
> becomes eligible for reserves, guarded by can_retry_reserves.  The flag
> is set at its declaration and never reset.
> 
> A restart after a cpuset update or a zonelist rebuild drops
> reserve_flags

I don't follow, nothing "drops reserve_flags"? after the restart,
there will still be a get_page_from_freelist() without them followed by
get_page_from_freelist() with them, right?

> and comes back with can_retry_reserves already false, so a
> caller that cannot reclaim, such as GFP_NOWAIT | __GFP_MEMALLOC, fails
> without trying the reserves again.

The "goto retry" triggered by can_retry_reserves actually drops nodemask and
recalculates preferred zones. Perhaps "can_retry_reserves" wasn't the best name.

> Reset can_retry_reserves at restart.  Only a cpuset or zonelist change
> restarts the slowpath, so the retry still runs at most once per change.
> 
> With kprobes failing every attempt without reserves and the first with
> them, and a memory block onlined during the walk, the allocation
> returned NULL in 3 of 3 runs, and a page in 3 of 3 with this change.

So IIUC the allocation was racing with a memory online in a way that it
would fail if the online happened fully after the allocation. So this just
turns part of the race window into the allocation's favor?

> Fixes: 2c4c3e29897d ("mm/page_alloc: simplify __alloc_pages_slowpath() flow")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>

I mean it's not wrong and if patch 5 depends on it (unless it also only
fixes a racy window in the same way I think is happening here) then so be
it. But the commit log would need correcting.

> ---
>  mm/page_alloc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 2daf358269ae..78569ac6f2d9 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4783,7 +4783,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	unsigned int zonelist_iter_cookie;
>  	int reserve_flags;
>  	bool compact_first = false;
> -	bool can_retry_reserves = true;
> +	bool can_retry_reserves;
>  	unsigned long alloc_start_time = jiffies;
>  
>  	/*
> @@ -4817,6 +4817,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	no_progress_loops = 0;
>  	compact_result = COMPACT_SKIPPED;
>  	compact_priority = DEF_COMPACT_PRIORITY;
> +	can_retry_reserves = true;
>  	cpuset_mems_cookie = read_mems_allowed_begin();
>  	zonelist_iter_cookie = zonelist_iter_begin();
>