Re: [PATCH 1/6] mm/compaction: wait for pfn walkers before poisoning offlined memmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/compaction: clamping low_pfn in isolate_migratepages() loops forever
- [High] mm: race in pageblock_pfn_to_page() with move_pfn_range_to_zone() onlining
- [Medium] mm: pfn_walk_srcu is disabled when CONFIG_MEMORY_HOTREMOVE=n
--

commit 6ad7c6454b21a8c91c2521a8135b38b5d2766881
Author: Gregory Price (Meta) <gourry@gourry.net>

mm/compaction: wait for pfn walkers before poisoning offlined memmap

This patch introduces an SRCU-fast read section around compaction PFN walks
and waits for readers in remove_pfn_range_from_zone() before poisoning
offlined memmap pages. It also updates pageblock_pfn_to_page() and
isolate_migratepages() to handle zone span changes across memory offlining.

> diff --git a/mm/compaction.c b/mm/compaction.c
> index f1b2060eb2016..2aeb18b6d54dd 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c

[ ... ]

> @@ -2116,6 +2121,9 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>  	 */
>  	fast_find_block = low_pfn != cc->migrate_pfn && !cc->fast_search_fail;
>  
> +	/* Offline may have shrunk the zone start past an old migrate_pfn */
> +	low_pfn = max(low_pfn, block_start_pfn);
> +
>  	/* Only scan within a pageblock boundary */
>  	block_end_pfn = pageblock_end_pfn(low_pfn);

[Severity: High]
Can clamping low_pfn to block_start_pfn without updating cc->migrate_pfn
cause compact_zone() to enter an unbounded infinite loop?

If memory offlining shrinks cc->zone->zone_start_pfn upward via
offline_pages() -> remove_pfn_range_from_zone() -> shrink_zone_span() while
compaction is running, cc->migrate_pfn can sit below the new
cc->zone->zone_start_pfn while cc->free_pfn is either also in the offlined
lower range (with cc->migrate_pfn < cc->free_pfn) or in the first pageblock
of the new zone start (where block_end_pfn > cc->free_pfn).

In isolate_migratepages() in mm/compaction.c, clamping low_pfn to
block_start_pfn advances block_end_pfn = pageblock_end_pfn(low_pfn) past
cc->free_pfn, so the pageblock loop is skipped completely:

	for (; block_end_pfn <= cc->free_pfn;
			fast_find_block = false,
			cc->migrate_pfn = low_pfn = block_end_pfn,
			block_start_pfn = block_end_pfn,
			block_end_pfn += pageblock_nr_pages) {

Because the loop does not execute, isolate_migratepages() returns
ISOLATE_NONE without advancing cc->migrate_pfn or calling cond_resched().

Back in compact_zone(), compact_finished() -> __compact_finished() checks
compact_scanners_met(cc):

static inline bool compact_scanners_met(struct compact_control *cc)
{
	return (cc->free_pfn >> pageblock_order)
		<= (cc->migrate_pfn >> pageblock_order);
}

Since cc->migrate_pfn was never updated to low_pfn, compact_scanners_met(cc)
remains false and __compact_finished() returns COMPACT_CONTINUE (even
before checking fatal_signal_pending(current) when is_via_compact_memory()
or !pageblock_aligned(cc->migrate_pfn) holds), causing compact_zone() to
spin forever on ISOLATE_NONE without rescheduling.

Should cc->migrate_pfn also be updated when low_pfn is clamped to
block_start_pfn?

> diff --git a/mm/internal.h b/mm/internal.h
> index 3a402e735dbe2..fdc83a7aafb51 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -18,6 +18,7 @@
>  #include <linux/rmap.h>
>  #include <linux/swap.h>
>  #include <linux/leafops.h>
> +#include <linux/srcu.h>
>  #include <linux/tracepoint-defs.h>
>  
>  /* Internal core VMA manipulation functions. */
> @@ -1208,6 +1209,46 @@ struct tlbflush_unmap_batch;
>   */
>  extern struct workqueue_struct *mm_percpu_wq;
>  
> +/*
> + * Compaction checks that a pageblock is online and then uses its pages,
> + * possibly across sleeps.  remove_pfn_range_from_zone() waits for these
> + * readers before it poisons the memmap of a range taken offline.
> + */
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +extern struct srcu_struct pfn_walk_srcu;

[Severity: Medium]
Should pfn_walk_srcu be guarded by CONFIG_MEMORY_HOTPLUG rather than
CONFIG_MEMORY_HOTREMOVE?

When CONFIG_MEMORY_HOTPLUG=y, CONFIG_MHP_MEMMAP_ON_MEMORY=y, and
CONFIG_MEMORY_HOTREMOVE=n, pfn_walk_lock(), pfn_walk_unlock(), and
pfn_walk_synchronize() compile to no-ops.

However, during memory_block_online() in drivers/base/memory.c:

	if (nr_vmemmap_pages) {
		ret = mhp_init_memmap_on_memory(start_pfn, nr_vmemmap_pages, zone);
		if (ret)
			goto out;
	}

	ret = online_pages(start_pfn + nr_vmemmap_pages,
			   nr_pages - nr_vmemmap_pages, zone, mem->group);
	if (ret) {
		if (nr_vmemmap_pages)
			mhp_deinit_memmap_on_memory(start_pfn, nr_vmemmap_pages);
		goto out;
	}

If nr_vmemmap_pages >= PAGES_PER_SECTION, mhp_init_memmap_on_memory() in
mm/memory_hotplug.c marks full vmemmap sections online via
online_mem_sections() before online_pages() runs. If online_pages() then
fails, mhp_deinit_memmap_on_memory() calls offline_mem_sections() and
remove_pfn_range_from_zone(), which poisons the struct pages without waiting
for concurrent compaction pfn_walk readers when CONFIG_MEMORY_HOTREMOVE=n.

[ ... ]

> diff --git a/mm/page_alloc.h b/mm/page_alloc.h
> index 2b1017a91928c..3446a2ebdd0f2 100644
> --- a/mm/page_alloc.h
> +++ b/mm/page_alloc.h
> @@ -216,7 +216,12 @@ extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>  static inline struct page *pageblock_pfn_to_page(unsigned long start_pfn,
>  				unsigned long end_pfn, struct zone *zone)
>  {
> -	if (zone->contiguous)
> +	/*
> +	 * Offline can shrink the zone under a caller's old pfns and then set
> +	 * zone->contiguous again, so only trust it inside the current span.
> +	 */
> +	if (smp_load_acquire(&zone->contiguous) &&
> +	    zone_spans_pfn(zone, start_pfn) && zone_spans_pfn(zone, end_pfn - 1))
>  		return pfn_to_page(start_pfn);
>  
>  	return __pageblock_pfn_to_page(start_pfn, end_pfn, zone);

[Severity: High]
Can pageblock_pfn_to_page() race with move_pfn_range_to_zone() during memory
onlining and return poisoned struct pages through the zone->contiguous fast
path?

When a boundary range is offlined by remove_pfn_range_from_zone(), its
struct pages are poisoned, the zone span shrinks, and set_zone_contiguous()
sets zone->contiguous = true for the remaining span.

When that range (or a newly hot-added range) is later onlined via
online_pages() -> move_pfn_range_to_zone() in mm/memory_hotplug.c:

	clear_zone_contiguous(zone);

	if (zone_is_empty(zone))
		init_currently_empty_zone(zone, start_pfn, nr_pages);
	resize_zone_range(zone, start_pfn, nr_pages);
	resize_pgdat_range(pgdat, start_pfn, nr_pages);
	...
	memmap_init_range(nr_pages, nid, zone_idx(zone), start_pfn, 0,
			 MEMINIT_HOTPLUG, altmap, migratetype,
			 isolate_pageblock);

move_pfn_range_to_zone() clears zone->contiguous first (with a plain store
and no barrier or grace period) and then expands zone->zone_start_pfn and
zone->spanned_pages in resize_zone_range() before memmap_init_range()
reinitializes the poisoned struct pages and before the sections are marked
online.

Because pageblock_pfn_to_page() also loads zone->contiguous first and checks
zone_spans_pfn() second, a concurrent reader in pageblock_pfn_to_page() can
load zone->contiguous == true right before clear_zone_contiguous(zone), and
then observe the newly expanded zone_start_pfn / spanned_pages from
resize_zone_range() while memmap_init_range() is still running (or observe
the stores out of order on weakly ordered architectures), bypassing
__pageblock_pfn_to_page() on offline poisoned pages.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027042204.3741307-1-gourry@gourry.net?part=1