Re: [PATCH v4 6/8] mm/hugetlb: retry the pool walk when the zonelist is rebuilt

[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.  dequeue_hugetlb_folio_nodemask()
> approves a slot by its node, then loads the zone from it, so a hugetlb
> fault bound to one node can take a pool page from another:
> 
>   CPU 0                             CPU 1
>   dequeue_hugetlb_folio_nodemask(nodemask {2})
>     slot 3 (NORMAL2) approved
>                                     online_pages()
>                                       build_all_zonelists()
>                                         [.., NORMAL1, NORMAL2]
>                                         -> [.., MOVABLE1, NORMAL1, ...]
>     zone = zonelist_zone(slot 3)    /* NORMAL1 */
>     dequeue_hugetlb_folio_node_exact(h, 1)
> 
> Check zonelist_update_seq before using each zone, and on the way out
> with nothing found, and restart the walk if it changed.  Checking before
> the dequeue leaves nothing to put back.  Reset the last-tried node on
> restart, so it is not skipped.
> 
> With kprobes holding the walk across the online, a fault bound to node 2
> took a node 1 page in 3 of 3 runs, and a node 2 page in 3 of 3 with this
> change.
> 
> Fixes: 11cd8638c37f ("mm, page_alloc: remove stop_machine from build_all_zonelists")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>

LGTM, but...

> ---
>  mm/hugetlb.c    | 13 ++++++++++---
>  mm/page_alloc.c |  4 ++--
>  mm/page_alloc.h |  3 +++
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index b37d7d0db7fc..8edf323baf0f 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1293,11 +1293,11 @@ static struct folio *dequeue_hugetlb_folio_node_exact(struct hstate *h,
>  static struct folio *dequeue_hugetlb_folio_nodemask(struct hstate *h, gfp_t gfp_mask,
>  							int nid, nodemask_t *nmask)
>  {
> -	unsigned int cpuset_mems_cookie;
> +	unsigned int cpuset_mems_cookie, zonelist_cookie;
>  	struct zonelist *zonelist;
>  	struct zone *zone;
>  	struct zoneref *z;
> -	int node = NUMA_NO_NODE;
> +	int node;
>  
>  	/* 'nid' should not be NUMA_NO_NODE. Try to catch any misuse of it and rectifiy. */
>  	if (nid == NUMA_NO_NODE)
> @@ -1306,10 +1306,16 @@ static struct folio *dequeue_hugetlb_folio_nodemask(struct hstate *h, gfp_t gfp_
>  	zonelist = node_zonelist(nid, gfp_mask);
>  
>  retry_cpuset:
> +	node = NUMA_NO_NODE;
>  	cpuset_mems_cookie = read_mems_allowed_begin();
> +	zonelist_cookie = zonelist_iter_begin();
>  	for_each_zone_zonelist_nodemask(zone, z, zonelist, gfp_zone(gfp_mask), nmask) {
>  		struct folio *folio;
>  
> +		/* A rebuild may have shifted a node outside @nmask into this slot. */
> +		if (unlikely(check_retry_zonelist(zonelist_cookie)))
> +			goto retry_cpuset;
> +
>  		if (!cpuset_zone_allowed(zone, gfp_mask))
>  			continue;
>  		/*
> @@ -1324,7 +1330,8 @@ static struct folio *dequeue_hugetlb_folio_nodemask(struct hstate *h, gfp_t gfp_
>  		if (folio)
>  			return folio;
>  	}
> -	if (unlikely(read_mems_allowed_retry(cpuset_mems_cookie)))
> +	if (unlikely(read_mems_allowed_retry(cpuset_mems_cookie) ||
> +		     check_retry_zonelist(zonelist_cookie)))
>  		goto retry_cpuset;
>  
>  	return NULL;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 33aab9deaaa1..70830689048a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4411,7 +4411,7 @@ EXPORT_SYMBOL_GPL(fs_reclaim_release);
>   */
>  static DEFINE_SEQLOCK(zonelist_update_seq);
>  
> -static unsigned int zonelist_iter_begin(void)
> +unsigned int zonelist_iter_begin(void)
>  {
>  	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>  		return read_seqbegin(&zonelist_update_seq);
> @@ -4432,7 +4432,7 @@ static unsigned int zonelist_iter_sample(void)
>  	return 0;
>  }
>  
> -static unsigned int check_retry_zonelist(unsigned int seq)
> +unsigned int check_retry_zonelist(unsigned int seq)
>  {
>  	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>  		return read_seqretry(&zonelist_update_seq, seq);
> diff --git a/mm/page_alloc.h b/mm/page_alloc.h
> index 2b1017a91928..53df6b8a0c00 100644
> --- a/mm/page_alloc.h
> +++ b/mm/page_alloc.h
> @@ -265,6 +265,9 @@ extern void zone_pcp_disable(struct zone *zone);
>  extern void zone_pcp_enable(struct zone *zone);
>  extern void zone_pcp_init(struct zone *zone);
>  
> +unsigned int zonelist_iter_begin(void);
> +unsigned int check_retry_zonelist(unsigned int seq);

... is it possible to inline these here instead?

> +
>  enum fallback_result {
>  	/* Found suitable migratetype, *mt_out is valid. */
>  	FALLBACK_FOUND,