Re: [PATCH] mm/page_alloc.c: Avoid infinite retries caused by cpuset race

[Vlastimil Babka <vbabka@suse.cz>]

On 5/13/25 21:16, Andrew Morton wrote:
> On Tue, 13 May 2025 09:26:53 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> 
>> > > > This has been in mm-hotfixes-unstable for six days.  Hopefully we'll
>> > > > see some review activity soon (please).
>> > >
>> > > I reviewed and provided my feedback but saw neither a reply nor a
>> > > respin with proposed changes.
>> >
>> > OK, thanks.  Do you have time to put together a modified version of this?
>> 
>> I think the code is fine as is. Would be good to add Fixes: tag but it
>> will require some investigation to find the appropriate patch to
>> reference here.
> 
> Below is what is in mm-hotfixes.  It doesn't actually have any
> acked-by's or reviewed-by's.
> 
> So... final call for review, please.
> 
> 
> From: Tianyang Zhang <zhangtianyang@loongson.cn>
> Subject: mm/page_alloc.c: avoid infinite retries caused by cpuset race
> Date: Wed, 16 Apr 2025 16:24:05 +0800
> 
> __alloc_pages_slowpath has no change detection for ac->nodemask in the
> part of retry path, while cpuset can modify it in parallel.  For some
> processes that set mempolicy as MPOL_BIND, this results ac->nodemask
> changes, and then the should_reclaim_retry will judge based on the latest
> nodemask and jump to retry, while the get_page_from_freelist only
> traverses the zonelist from ac->preferred_zoneref, which selected by a
> expired nodemask and may cause infinite retries in some cases
> 
> cpu 64:
> __alloc_pages_slowpath {
>         /* ..... */
> retry:
>         /* ac->nodemask = 0x1, ac->preferred->zone->nid = 1 */
>         if (alloc_flags & ALLOC_KSWAPD)
>                 wake_all_kswapds(order, gfp_mask, ac);
>         /* cpu 1:
>         cpuset_write_resmask
>             update_nodemask
>                 update_nodemasks_hier
>                     update_tasks_nodemask
>                         mpol_rebind_task
>                          mpol_rebind_policy
>                           mpol_rebind_nodemask
> 		// mempolicy->nodes has been modified,
> 		// which ac->nodemask point to
> 
>         */
>         /* ac->nodemask = 0x3, ac->preferred->zone->nid = 1 */
>         if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
>                                  did_some_progress > 0, &no_progress_loops))
>                 goto retry;
> }
> 
> Simultaneously starting multiple cpuset01 from LTP can quickly reproduce
> this issue on a multi node server when the maximum memory pressure is
> reached and the swap is enabled
> 
> Link: https://lkml.kernel.org/r/20250416082405.20988-1-zhangtianyang@loongson.cn
> Fixes: 902b62810a57 ("mm, page_alloc: fix more premature OOM due to race with cpuset update").

After the discussion in this thread, Suren retracted this Fixes: suggestion.
I think it actually goes back to this one which introduced the
preferred_zoneref caching.

Fixes: c33d6c06f60f ("mm, page_alloc: avoid looking up the first zone in a
zonelist twice")

> Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Brendan Jackman <jackmanb@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

I would have placed the check bit further down, just above the
should_reclaim_retry() call, but it's not that important to hold up a fix
and can be done later.

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> ---
> 
>  mm/page_alloc.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> --- a/mm/page_alloc.c~mm-page_allocc-avoid-infinite-retries-caused-by-cpuset-race
> +++ a/mm/page_alloc.c
> @@ -4562,6 +4562,14 @@ restart:
>  	}
>  
>  retry:
> +	/*
> +	 * Deal with possible cpuset update races or zonelist updates to avoid
> +	 * infinite retries.
> +	 */
> +	if (check_retry_cpuset(cpuset_mems_cookie, ac) ||
> +	    check_retry_zonelist(zonelist_iter_cookie))
> +		goto restart;
> +
>  	/* Ensure kswapd doesn't accidentally go to sleep as long as we loop */
>  	if (alloc_flags & ALLOC_KSWAPD)
>  		wake_all_kswapds(order, gfp_mask, ac);
> _
>