Re: [RFC PATCH v2 07/18] mm: free the pages waiting for a grace period before isolation fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mm: premature NR_FREE_RCU_PAGES decrement in rcu_free_pages() skips sync
- [Medium] mm: stale outer_start in alloc_contig_frozen_range_noprof() after RCU free merge
- [Medium] mm: page_is_unmovable() treats freed compound PG_rcu_free folios as unmovable
--

commit 01209427ff9390747e1e7bf6dbe80b5fa2e38b2b
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

mm: free the pages waiting for a grace period before isolation fails

This commit adds rcu_free_sync() and zone_rcu_free_waiting() so that
alloc_contig_frozen_range_noprof() and offline_pages() can drain and flush
folios waiting for an RCU grace period when test_pages_isolated() fails.

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 226ab9cb078ad..c23fab6529571 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1967,6 +1967,7 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  		.nid = NUMA_NO_NODE,
>  	};
>  	unsigned long flags;
> +	bool migrated;
>  	char *reason;
>  	int ret;
>  	unsigned long normal_pages = 0;
> @@ -2050,6 +2051,7 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  
>  	do {
>  		pfn = start_pfn;
> +		migrated = false;
>  		do {
>  			/*
>  			 * Historically we always checked for any signal and
> @@ -2071,6 +2073,7 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  				 * out
>  				 */
>  				do_migrate_range(pfn, end_pfn);
> +				migrated = true;
>  			}
>  		} while (!ret);
>  
> @@ -2092,6 +2095,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  
>  		ret = test_pages_isolated(start_pfn, end_pfn,
>  					  PB_ISOLATE_MODE_MEM_OFFLINE);
> +		/* What this pass migrated can wait for a grace period */
> +		if (ret && migrated && zone_rcu_free_waiting(zone))
> +			rcu_free_sync();
>  
>  	} while (ret);
>  

[Severity: Medium]
What happens if a large compound folio with PG_rcu_free was already freed
before offline_pages() or alloc_contig_frozen_range_noprof() calls
start_isolate_page_range()?

When __free_pages_prepare() intercepts a folio with PG_rcu_free and defers
it via rcu_free_defer(), the folio has a reference count of zero and
PageLRU cleared, while PageCompound remains set until rcu_free_pages()
later completes the free:

mm/page_alloc.c:__free_pages_prepare() {
    ...
	if (unlikely(folio_test_rcu_free(folio)) &&
	    !(PageHWPoison(page) && !order)) {
		VM_WARN_ON_ONCE(fpi_flags & FPI_NOLOCK);
		__folio_clear_rcu_free(folio);
		count_vm_events(PGFREE_RCU, 1 << order);
		rcu_free_defer(folio);
		return false;
	}
    ...
}

Before reaching test_pages_isolated() and rcu_free_sync(), both
offline_pages() and alloc_contig_frozen_range_noprof() (as well as
alloc_contig_frozen_pages() via pfn_range_valid_contig()) check for
unmovable pages through:

  start_isolate_page_range()
    -> set_migratetype_isolate()
      -> has_unmovable_pages()
        -> page_is_unmovable()

In page_is_unmovable() (outside ZONE_MOVABLE), order-0 pages with a zero
reference count fall through to the !page_ref_count(page) check and are
treated as movable, but compound pages are checked first and rejected when
!PageLRU(page) without checking !folio_ref_count(folio):

mm/page_isolation.c:page_is_unmovable() {
    ...
	if (PageCompound(page)) {
		struct folio *folio = page_folio(page);
		...
		if (folio_test_hugetlb(folio)) {
			...
		} else if (!PageLRU(page)) {
			return true;
		}
    ...
}

Won't start_isolate_page_range() or pfn_range_valid_contig() treat a freed
compound folio waiting for an RCU grace period as unmovable and fail with
-EBUSY before rcu_free_sync() is ever reached?

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9caaa57bf51d7..eea287ab2a011 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1479,6 +1479,19 @@ static void rcu_free_drain(void)
>  	mutex_unlock(&lock);
>  }
>  
> +/* Free every waiting folio, including those a work took off the lists */
> +void rcu_free_sync(void)
> +{
> +	int cpu;
> +
> +	if (!IS_ENABLED(CONFIG_PAGE_RCU_FREE))
> +		return;
> +
> +	rcu_free_drain();
> +	for_each_possible_cpu(cpu)
> +		flush_work(&per_cpu_ptr(&rcu_free_pcpu, cpu)->rwork.work);
> +}
> +
>  static void __init rcu_free_init(void)
>  {
>  	int cpu;
> @@ -7649,7 +7662,16 @@ int alloc_contig_frozen_range_noprof(unsigned long start, unsigned long end,
>  	outer_start = find_large_buddy(start);
>  
>  	/* Make sure the range is really isolated. */
> -	if (test_pages_isolated(outer_start, end, mode)) {
> +	ret = test_pages_isolated(outer_start, end, mode);
> +	if (ret && zone_rcu_free_waiting(cc.zone)) {
> +		rcu_free_sync();
> +
> +		/* A freed folio can merge with a free buddy below start */
> +		outer_start = find_large_buddy(start);
> +		ret = test_pages_isolated(outer_start, end, mode);
> +	}
> +
> +	if (ret) {
>  		ret = -EBUSY;
>  		goto done;
>  	}

[Severity: Medium]
Can outer_start in alloc_contig_frozen_range_noprof() still become stale if
a background RCU free completes and merges across start or outer_start?

First, suppose a migrated folio at start had PG_rcu_free and a free buddy
exists immediately below start in the same isolated pageblock. When the
initial find_large_buddy(start) runs, the folio at start is not yet freed,
so find_large_buddy(start) returns start:

mm/page_alloc.c:find_large_buddy() {
    ...
	if (pfn + (1 << buddy_order(page)) > start_pfn)
		return pfn;

	/* Nothing found */
	return start_pfn;
}

If rcu_free_workfn() or rcu_free_expedite_workfn() on another CPU finishes
freeing that folio before zone_rcu_free_waiting(cc.zone) is checked,
__free_one_page() merges the folio with the preceding buddy below start,
clearing PageBuddy on start. Then test_pages_isolated(outer_start, end,
mode) fails because outer_start (still equal to start) is now a tail of the
merged buddy, and zone_rcu_free_waiting(cc.zone) returns false because
NR_FREE_RCU_PAGES was already decremented to 0, so outer_start is never
recomputed and alloc_contig_frozen_range_noprof() returns -EBUSY.

Second, suppose all pages in [start, end) are already free in the buddy
allocator when the initial find_large_buddy(start) and
test_pages_isolated(outer_start, end, mode) run, so test_pages_isolated()
returns 0 and rcu_free_sync() is not called. If a folio below outer_start
within the same isolated pageblock was waiting for an RCU grace period and
is freed by rcu_free_workfn() before isolate_freepages_range(&cc,
outer_start, end) runs:

mm/page_alloc.c:alloc_contig_frozen_range_noprof() {
    ...
	/* Grab isolated pages from freelists. */
	outer_end = isolate_freepages_range(&cc, outer_start, end);
	if (!outer_end) {
		ret = -EBUSY;
		goto done;
	}
    ...
}

__free_one_page() can merge that newly freed folio with the buddy at
outer_start, moving the buddy head to a lower PFN and causing
isolate_freepages_range() to fail with -EBUSY.

> diff --git a/mm/page_alloc.h b/mm/page_alloc.h
> index b9259deddb59d..764e3f43b3b98 100644
> --- a/mm/page_alloc.h
> +++ b/mm/page_alloc.h
> @@ -235,6 +235,14 @@ struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order, int nid,
>  	alloc_hooks(__alloc_frozen_pages_noprof(__VA_ARGS__))
>  void free_frozen_pages(struct page *page, unsigned int order);
>  void free_unref_folios(struct folio_batch *fbatch);
> +void rcu_free_sync(void);
> +
> +/* Whether @zone has pages waiting for a grace period */
> +static inline bool zone_rcu_free_waiting(struct zone *zone)
> +{
> +	return IS_ENABLED(CONFIG_PAGE_RCU_FREE) &&
> +	       zone_page_state_snapshot(zone, NR_FREE_RCU_PAGES);
> +}

[Severity: Medium]
As reported by Sashiko on RFC [1], doesn't gating rcu_free_sync() on
zone_rcu_free_waiting() miss folios that are actively being freed inside
rcu_free_pages() after NR_FREE_RCU_PAGES has already been decremented?

In rcu_free_pages(), NR_FREE_RCU_PAGES is decremented for each folio before
__free_pages_ok() or free_unref_folios(&fbatch) prepares the pages and
returns them to the buddy freelists under zone->lock:

mm/page_alloc.c:rcu_free_pages() {
    ...
	list_for_each_entry_safe(folio, next, list, lru) {
		nr += folio_nr_pages(folio);
		mod_zone_page_state(folio_zone(folio), NR_FREE_RCU_PAGES,
				    -folio_nr_pages(folio));
		if (direct) {
			__free_pages_ok(&folio->page, folio_order(folio),
					FPI_NONE);
			cond_resched();
		} else if (!folio_batch_add(&fbatch, folio)) {
			free_unref_folios(&fbatch);
			cond_resched_tasks_rcu_qs();
		}
	}
	if (folio_batch_count(&fbatch))
		free_unref_folios(&fbatch);
    ...
}

When rcu_free_workfn(), rcu_free_expedite_workfn(), or a concurrent
rcu_free_drain() decrements NR_FREE_RCU_PAGES to 0 for the last waiting
folios in the zone, there is a window while __free_pages_prepare() (page
zeroing, KASAN poisoning, memcg uncharging, etc.) and free_one_page() are
still running where the pages are not yet PageBuddy in the buddy allocator,
but zone_rcu_free_waiting(zone) already reads 0 and returns false.

Although rcu_free_sync() flushes rwork.work and expedite works to wait for
folios taken off the per-CPU lists, both alloc_contig_frozen_range_noprof()
and offline_pages() skip calling rcu_free_sync() when
zone_rcu_free_waiting() returns false, causing
alloc_contig_frozen_range_noprof() to return -EBUSY and offline_pages() to
skip synchronizing on the pass that migrated folios.

[1] https://lore.kernel.org/r/20261019154913.1765974-8-kas@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104130118.2507513-1-kas@kernel.org?part=7