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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm/page_alloc: __alloc_pages_slowpath() restart loses reserve retry
- [Medium] mm/page_alloc: post-rmqueue() zonelist retry leaves ZONE_MOVABLE blocks unmovable
--

commit 3b023f87050323ef9781701d5864437d2b2b4b5e
Author: Gregory Price (Meta) <gourry@xxxxxxxxxx>

mm/page_alloc: retry allocations that raced with a zonelist rebuild

This commit checks zonelist_update_seq after page allocation succeeds in the
fast path, slowpath, and bulk allocator, freeing the allocated page and
retrying if a concurrent zonelist rebuild occurred during the zonelist walk.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1bb308296fcdc..dd8c9046f9a06 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
[ ... ]
> @@ -5114,6 +5127,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;
>  }

[Severity: Medium]
In __alloc_pages_slowpath() in mm/page_alloc.c, can_retry_reserves is
initialized to true only once before the restart: label:

	bool compact_first = false;
	bool can_retry_reserves = true;
	unsigned long alloc_start_time = jiffies;

When a reserve-eligible allocation (such as PF_MEMALLOC, __GFP_MEMALLOC, or
an OOM victim) fails its initial get_page_from_freelist() attempt at retry:,
it sets reserve_flags in alloc_flags, clears ac->nodemask, sets
can_retry_reserves = false, and jumps to retry:

	reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
	if (reserve_flags)
		alloc_flags = alloc_flags_cma(gfp_mask) | reserve_flags |
				ac->alloc_flags | (alloc_flags & ALLOC_KSWAPD);

	if (!(alloc_flags & ALLOC_CPUSET) || reserve_flags) {
		ac->nodemask = NULL;
		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
					ac->highest_zoneidx, ac->nodemask);

		if (can_retry_reserves) {
			can_retry_reserves = false;
			goto retry;
		}
	}

If that reserve pass allocates a page and reaches got_pg: while
check_retry_zonelist(zonelist_iter_cookie) is true, the new check frees the
page and jumps to restart:.

At restart:, alloc_flags is reinitialized without reserve_flags:

	alloc_flags = ac->alloc_flags | alloc_flags_slowpath(gfp_mask, order);

Because can_retry_reserves is still false (and ac->nodemask remains NULL), if
the first get_page_from_freelist() after restart: fails without
reserve_flags, the can_retry_reserves retry is skipped. Callers with
!can_direct_reclaim (for example GFP_NOWAIT | __GFP_MEMALLOC or
GFP_ATOMIC | __GFP_MEMALLOC) or current->flags & PF_MEMALLOC then jump
directly to nopage: and fail without ever retrying get_page_from_freelist()
with reserve_flags on the rebuilt zonelist.

Could jumping to restart: from got_pg: without resetting can_retry_reserves
(and restoring ac->nodemask) cause reserve-eligible allocations to lose their
reserve retry and fail spuriously?

[ ... ]
> @@ -5472,6 +5499,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;
>  

[Severity: Medium]
When a non-movable request (such as GFP_KERNEL with MIGRATE_UNMOVABLE or
MIGRATE_RECLAIMABLE) races with online_pages() -> build_all_zonelists() and
get_page_from_freelist() selects ZONE_MOVABLE, rmqueue() runs on ZONE_MOVABLE
before check_retry_zonelist() is checked in __alloc_frozen_pages_noprof() or
at got_pg: in __alloc_pages_slowpath().

Because ZONE_MOVABLE has no free pages in MIGRATE_UNMOVABLE,
rmqueue() -> rmqueue_pcplist() -> __rmqueue_pcplist() -> rmqueue_bulk() ->
__rmqueue() -> __rmqueue_claim() -> try_to_claim_block() claims a
MIGRATE_MOVABLE block in ZONE_MOVABLE, converts its pageblock migratetype to
start_type, and expands the remaining pages into ZONE_MOVABLE's non-movable
freelists and PCP list:

	/* Take ownership for orders >= pageblock_order */
	if (current_order >= pageblock_order) {
		unsigned int nr_added;

		del_page_from_free_list(page, zone, current_order, block_type);
		change_pageblock_range(page, current_order, start_type);
		nr_added = expand(zone, page, order, current_order, start_type);
		account_freepages(zone, nr_added, start_type);
		return page;
	}

When check_retry_zonelist() then detects the race and calls
__free_frozen_pages(), __free_frozen_pages() reads the mutated pageblock
migratetype:

	zone = page_zone(page);
	migratetype = get_pfnblock_migratetype(page, pfn);

and frees only that single page back to ZONE_MOVABLE's MIGRATE_UNMOVABLE PCP
list or buddy freelist.

Can checking check_retry_zonelist() only after rmqueue() has already mutated
the zone leave converted MIGRATE_UNMOVABLE, MIGRATE_RECLAIMABLE, or
MIGRATE_HIGHATOMIC pageblocks and populated non-movable PCP/free lists inside
ZONE_MOVABLE, where normal movable allocations will not consume the PCP list
and compaction will skip the non-movable pageblocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021023940.1940971-1-gourry@xxxxxxxxxx?part=5