Re: [RFC PATCH 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 1 potential issue(s) to consider:
- [Medium] mm: premature NR_FREE_RCU_PAGES decrement in rcu_free_pages() skips sync
--

commit 173a808c17014ff8171f1ae96f92027767d39401
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

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

This commit introduces rcu_free_sync() to drain and flush pages waiting for an
RCU grace period when test_pages_isolated() fails in alloc_contig_range() and
offline_pages().

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 226ab9cb078ad..a8f0c95627ee1 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
[ ... ]
> @@ -2092,6 +2095,10 @@ 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_page_state_snapshot(zone, NR_FREE_RCU_PAGES))
> +			rcu_free_sync();
>  
>  	} while (ret);
>  
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index bec9128d80db8..e63628e64cbba 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1468,6 +1468,23 @@ 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) {
> +		struct rcu_free_pcpu *r = per_cpu_ptr(&rcu_free_pcpu, cpu);
> +
> +		flush_work(&r->rwork.work);
> +		flush_work(&r->expedite);
> +	}
> +}
> +
>  static void __init rcu_free_init(void)
>  {
>  	int cpu;
> @@ -7632,7 +7649,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_page_state_snapshot(cc.zone, NR_FREE_RCU_PAGES)) {
> +		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);
> +	}

[Severity: Medium]
Can gating rcu_free_sync() on zone_page_state_snapshot(..., NR_FREE_RCU_PAGES)
miss folios that a worker or concurrent drain is actively freeing in
rcu_free_pages()?

rcu_free_sync() flushes r->rwork.work and r->expedite (in addition to
acquiring the drain mutex in rcu_free_drain()) so that it waits for folios
already taken off the per-CPU lists by a worker.

However, in rcu_free_pages(), NR_FREE_RCU_PAGES is decremented as each folio
is added to fbatch, before free_unref_folios(&fbatch) prepares the pages and
returns them to the buddy allocator:

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 (!folio_batch_add(&fbatch, folio)) {
			free_unref_folios(&fbatch);
			cond_resched_tasks_rcu_qs();
		}
	}
	if (folio_batch_count(&fbatch))
		free_unref_folios(&fbatch);
	...
}

If rcu_free_pages() has decremented NR_FREE_RCU_PAGES to 0 for the last
waiting folios in the zone and is still executing free_unref_folios(&fbatch),
those pages are not yet PageBuddy in the buddy allocator when
test_pages_isolated() checks them, while zone_page_state_snapshot() already
reads 0.

When that happens, wouldn't alloc_contig_frozen_range_noprof() skip
rcu_free_sync() and return -EBUSY, and wouldn't offline_pages() skip
rcu_free_sync() on the pass where migrated is true (before resetting migrated
to false on the next pass)?

[ ... ]

>
>  	if (ret) {
>  		ret = -EBUSY;
>  		goto done;
>  	}

[ ... ]

—
🤖 Generated by janitor_bot (expert review)
Single-turn review. Let the author decide how to proceed.
Reply with feedback if anything looks off.

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