Re: [RFC PATCH 06/18] mm: free the pages waiting for a grace period before OOM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mm: rcu_free_drain() fails to wait for in-flight RCU free workers
- [High] mm: rcu_free_wait() passes high order to __zone_watermark_ok()
- [High] mm: should_reclaim_retry() splits reclaimable and RCU-waiting pages
- [High] mm: rcu_free_drain() leaves freed pages on PCP lists instead of buddy
--

commit 7de2bde642756023a3c02e6554282a28f501cd38
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

mm: free the pages waiting for a grace period before OOM

This commit adds rcu_free_wait() and rcu_free_drain() to
should_reclaim_retry() so that pages deferred for an RCU grace period are
drained via an expedited grace period before giving up and invoking the OOM
killer. It also introduces the PGFREE_RCU_DRAIN vmstat event to count drains
that freed pages.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a9058d414f92d..bec9128d80db8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1445,6 +1445,29 @@ static void rcu_free_defer(struct folio *folio)
>  		queue_work(rcu_free_wq, &r->expedite);
>  }
>  
> +/*
> + * Free what every CPU's lists hold.  A concurrent drain holds the lock
> + * until it has freed what it took.
> + */
> +static void rcu_free_drain(void)
> +{
> +	static DEFINE_MUTEX(lock);
> +	LIST_HEAD(list);
> +	int cpu;
> +
> +	mutex_lock(&lock);
> +	for_each_possible_cpu(cpu) {
> +		struct rcu_free_pcpu *r = per_cpu_ptr(&rcu_free_pcpu, cpu);
> +
> +		spin_lock_irq(&r->lock);
> +		rcu_free_take(r, &list);
> +		spin_unlock_irq(&r->lock);
> +	}
> +	if (rcu_free_flush(&list))
> +		count_vm_event(PGFREE_RCU_DRAIN);
> +	mutex_unlock(&lock);
> +}

[Severity: High]
What happens if rcu_free_expedite_workfn() or rcu_free_workfn() has already
spliced the folios off r->batch and r->pending before rcu_free_drain() runs?

In rcu_free_expedite_workfn(), the worker takes the per-CPU lists into a
local list and drops r->lock before blocking in synchronize_rcu_expedited():

mm/page_alloc.c:rcu_free_expedite_workfn() {
	spin_lock_irq(&r->lock);
	rcu_free_take(r, &list);
	spin_unlock_irq(&r->lock);

	if (rcu_free_flush(&list))
		count_vm_event(PGFREE_RCU_EXPEDITE);
}

Similarly, rcu_free_workfn() splices r->batch into a local list under r->lock
before calling rcu_free_pages(), and NR_FREE_RCU_PAGES is only decremented
inside rcu_free_pages() as each folio is freed.

Because neither worker acquires the mutex in rcu_free_drain(), a task
entering rcu_free_wait() -> rcu_free_drain() while a worker is in flight
sees elevated NR_FREE_RCU_PAGES, finds r->batch and r->pending empty, and
returns immediately from rcu_free_flush(&list) without waiting. Once
*no_progress_loops > MAX_RECLAIM_RETRIES, should_reclaim_retry() also jumps
to out: and skips cond_resched().

Can __alloc_pages_slowpath() then spin through all MAX_RECLAIM_RETRIES
iterations of rcu_free_waits while the worker is still in
synchronize_rcu_expedited() and trigger a premature OOM kill?

[Severity: High]
Also, when rcu_free_drain() calls rcu_free_flush(&list) -> rcu_free_pages() ->
free_unref_folios(), folios with pcp_allowed_order(order) are freed via
free_frozen_page_commit() onto the local CPU's PCP list rather than directly
into the zone's buddy free_area (and NR_FREE_PAGES is only updated when pages
move from PCP to buddy).

When __alloc_pages_slowpath() jumps back to retry:, get_page_from_freelist()
checks zone_watermark_fast() using NR_FREE_PAGES before reaching rmqueue(),
and when __alloc_pages_direct_reclaim() sees *did_some_progress == 0, it
jumps to out: without calling drain_all_pages(NULL):

mm/page_alloc.c:__alloc_pages_direct_reclaim() {
	*did_some_progress = __perform_reclaim(gfp_mask, reclaim_order, ac);
	if (unlikely(!(*did_some_progress)))
		goto out;
	...
		drain_all_pages(NULL);
out:
}

If the pages freed by rcu_free_drain() stay on the PCP list and do not
increase NR_FREE_PAGES above min_wmark_pages(zone), won't the subsequent
retry fail the watermark check in get_page_from_freelist() and proceed to
__alloc_pages_may_oom() with those pages still stranded on PCP?

> +
>  static void __init rcu_free_init(void)
>  {
>  	int cpu;
> @@ -4792,6 +4815,52 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  	return !!__gfp_pfmemalloc_flags(gfp_mask);
>  }
>  
> +/*
> + * Reclaim is about to give up: if the pages waiting for a grace period
> + * would bring an eligible zone above its min watermark, free them and
> + * retry.  Not for a dying task, which frees memory by exiting.
> + */
> +static bool rcu_free_wait(gfp_t gfp_mask, unsigned int order,
> +			  struct alloc_context *ac, int alloc_flags,
> +			  int *rcu_free_waits)
> +{
> +	struct zone *zone;
> +	struct zoneref *z;
> +
> +	if (!IS_ENABLED(CONFIG_PAGE_RCU_FREE))
> +		return false;
> +	if (tsk_is_oom_victim(current) || fatal_signal_pending(current))
> +		return false;
> +	if (*rcu_free_waits >= MAX_RECLAIM_RETRIES ||
> +	    order > PAGE_ALLOC_COSTLY_ORDER)
> +		return false;
> +
> +	for_each_zone_zonelist_nodemask(zone, z, ac->zonelist,
> +				ac->highest_zoneidx, ac->nodemask) {
> +		unsigned long waiting, available;
> +
> +		if (cpusets_enabled() && (alloc_flags & ALLOC_CPUSET) &&
> +		    !__cpuset_zone_allowed(zone, gfp_mask))
> +			continue;
> +
> +		waiting = zone_page_state_snapshot(zone, NR_FREE_RCU_PAGES);
> +		if (!waiting)
> +			continue;
> +
> +		available = waiting +
> +			    zone_page_state_snapshot(zone, NR_FREE_PAGES);
> +		if (__zone_watermark_ok(zone, order, min_wmark_pages(zone),
> +					ac->highest_zoneidx, alloc_flags,
> +					available)) {

[Severity: High]
For non-costly high-order allocations (1 <= order <= PAGE_ALLOC_COSTLY_ORDER),
does passing order to __zone_watermark_ok() here prevent rcu_free_drain()
from being called when z->free_area[o >= order] is currently empty?

In __zone_watermark_ok(), the free_pages argument (available) is only checked
against min + z->lowmem_reserve[highest_zoneidx], after which order > 0
requires an existing free block in z->free_area[o >= order]:

mm/page_alloc.c:__zone_watermark_ok() {
	if (free_pages <= min + z->lowmem_reserve[highest_zoneidx])
		return false;

	/* If this is an order-0 request then the watermark is fine */
	if (!order)
		return true;

	/* For a high-order request, check at least one suitable page is free */
	for (o = order; o < NR_PAGE_ORDERS; o++) {
		struct free_area *area = &z->free_area[o];
		...
}

Because the pages in NR_FREE_RCU_PAGES have not been freed into z->free_area
yet, won't __zone_watermark_ok() return false before rcu_free_drain() has a
chance to free them?

[Severity: High]
Also, should available in should_reclaim_retry() and rcu_free_wait() account
for zone_reclaimable_pages(zone) and NR_FREE_RCU_PAGES together?

When direct reclaim frees folios marked with PG_rcu_free,
__free_pages_prepare() diverts them to rcu_free_defer(), moving those pages
out of zone_reclaimable_pages(zone) and into NR_FREE_RCU_PAGES rather than
NR_FREE_PAGES.

In should_reclaim_retry(), the loop checks:

	available = reclaimable = zone_reclaimable_pages(zone);
	available += zone_page_state_snapshot(zone, NR_FREE_PAGES);

and if that fails, rcu_free_wait() checks:

	available = waiting +
		    zone_page_state_snapshot(zone, NR_FREE_PAGES);

If the pages needed to reach min_wmark_pages(zone) are split between
remaining reclaimable pages on the LRU and already-reclaimed pages waiting in
NR_FREE_RCU_PAGES, won't both checks fail individually and cause
should_reclaim_retry() to return false while reclaim is still making progress?

> +			(*rcu_free_waits)++;
> +			rcu_free_drain();
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}

[ ... ]

>dari

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