Re: [PATCH v4 5/8] mm/page_alloc: retry allocations that raced with a zonelist rebuild

[Vlastimil Babka (SUSE)]

On 10/28/26 22:19, Gregory Price (Meta) wrote:
> Onlining the first block of a zone rebuilds every zonelist in place,
> shifting the slots after the new zone.  The iterator approves a slot,
> then the allocator loads the zone from it, so a GFP_KERNEL request can
> land in ZONE_MOVABLE and a nodemask request on a node outside its mask:
> 
>   CPU 0                             CPU 1
>   prepare_alloc_pages()
>     first_zones_zonelist(nodemask {1})
>       slot 0 (NORMAL1) approved
>                                     online_pages()
>                                       build_all_zonelists()
>                                         [NORMAL1, NORMAL2, ...]
>                                         -> [MOVABLE1, NORMAL1, ...]
>   get_page_from_freelist()
>     zone = zonelist_zone(slot 0)    /* MOVABLE1 */
>     allocates from MOVABLE1
> 
> zonelist_update_seq detects the rebuild, but only the slowpath's failure
> exits check it.
> 
> Check it whenever an allocation succeeds too, and if it changed, free
> the page and retry, as the memcg charge failure path does.  Eligible
> pages are dropped as well: zonelists only change when memory comes or
> goes, and a retry is simpler than rechecking the page.
> 
> Freeing the page does not undo a block the allocation claimed.  A
> GFP_KERNEL request that lands in a fresh ZONE_MOVABLE converts a free
> block to MIGRATE_UNMOVABLE, which stays until the zone's movable free
> list runs out and a movable allocation takes it back.

Yeah that's fine.

> The fast and bulk paths read the seqlock's raw seqcount without waiting:
> an ALLOC_NOLOCK caller can be an NMI that interrupted the writer, and on
> PREEMPT_RT the seqlock accessor takes the spinlock.  A sample taken
> mid-write costs a retry, or NULL under ALLOC_NOLOCK.
> check_retry_zonelist() returns 0 when compiled out, so a backport
> without the hot-add change stays harmless.

Hmm wasn't even aware there's waiting possible otherwise for seqcounts.

> Fixes: 11cd8638c37f ("mm, page_alloc: remove stop_machine from build_all_zonelists")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>

Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>

> ---
>  mm/page_alloc.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 24106c881ec2..33aab9deaaa1 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4419,12 +4419,25 @@ static unsigned int zonelist_iter_begin(void)
>  	return 0;
>  }
>  
> +/*
> + * Never wait for the writer, which an ALLOC_NOLOCK caller in NMI may have
> + * interrupted. Read the raw seqcount: on PREEMPT_RT the seqlock accessor takes
> + * the spinlock while a write is in progress.
> + */
> +static unsigned int zonelist_iter_sample(void)
> +{
> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> +		return raw_seqcount_begin(&zonelist_update_seq.seqcount.seqcount);
> +
> +	return 0;
> +}
> +
>  static unsigned int check_retry_zonelist(unsigned int seq)
>  {
>  	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>  		return read_seqretry(&zonelist_update_seq, seq);
>  
> -	return seq;
> +	return 0;
>  }
>  
>  /* Perform direct synchronous page reclaim */
> @@ -5115,6 +5128,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	warn_alloc(gfp_mask, ac->nodemask,
>  			"page allocation failure: order:%u", order);
>  got_pg:
> +	/* A rebuild during the walk may have handed out an ineligible zone. */
> +	if (page && check_retry_zonelist(zonelist_iter_cookie)) {
> +		__free_frozen_pages(page, order, FPI_NONE);
> +		page = NULL;
> +		goto restart;
> +	}
>  	return page;
>  }
>  
> @@ -5199,6 +5218,7 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  	struct list_head *pcp_list;
>  	struct alloc_context ac;
>  	unsigned int alloc_flags = ALLOC_WMARK_LOW;
> +	unsigned int zonelist_iter_cookie;
>  	int nr_populated = 0, nr_account = 0;
>  
>  	/*
> @@ -5239,6 +5259,7 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  	/* May set ALLOC_NOFRAGMENT, fragmentation will return 1 page. */
>  	gfp &= gfp_allowed_mask;
>  	gfp = current_gfp_context(gfp);
> +	zonelist_iter_cookie = zonelist_iter_sample();
>  	if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, &ac, &gfp, &alloc_flags))
>  		goto out;
>  
> @@ -5288,6 +5309,10 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  	if (unlikely(!zone))
>  		goto failed;
>  
> +	/* The zone was chosen from a zonelist that a rebuild may have changed. */
> +	if (unlikely(check_retry_zonelist(zonelist_iter_cookie)))
> +		goto failed;
> +
>  	/* spin_trylock may fail due to a parallel drain or IRQ reentrancy. */
>  	pcp = pcp_spin_trylock(zone->per_cpu_pageset);
>  	if (!pcp)
> @@ -5428,6 +5453,7 @@ struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>  		.alloc_flags = alloc_flags,
>  	};
>  	unsigned int fastpath_alloc_flags = alloc_flags;
> +	unsigned int zonelist_iter_cookie;
>  
>  	/* Other flags could be supported later if needed. */
>  	if (WARN_ON(alloc_flags & ~(ALLOC_NOLOCK | ALLOC_NO_CODETAG)))
> @@ -5457,6 +5483,7 @@ struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>  	 */
>  	gfp = current_gfp_context(gfp);
>  	alloc_gfp = gfp;
> +	zonelist_iter_cookie = zonelist_iter_sample();
>  	if (!prepare_alloc_pages(gfp, order, preferred_nid, nodemask, &ac,
>  			&alloc_gfp, &fastpath_alloc_flags))
>  		return NULL;
> @@ -5473,6 +5500,11 @@ struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>  
>  	/* First allocation attempt (or, for nolock, only attempt) */
>  	page = get_page_from_freelist(alloc_gfp, order, fastpath_alloc_flags, &ac);
> +	if (likely(page) && unlikely(check_retry_zonelist(zonelist_iter_cookie))) {
> +		__free_frozen_pages(page, order,
> +				    alloc_flags & ALLOC_NOLOCK ? FPI_NOLOCK : 0);
> +		page = NULL;
> +	}
>  	if (likely(page) || (alloc_flags & ALLOC_NOLOCK))
>  		goto out;
>