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) <[email protected]> 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/[email protected]?part=5
