Re: [RFC PATCH v2 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 3 potential issue(s) to consider:
- [High] mm: order-0 check in rcu_free_wait() starves high-order compaction
- [High] mm: rcu_free_drain() misses rcu_free_workfn() and PCP-freed pages
- [Medium] mm: rcu_free_wait() counts unreclaimable pages after retry limit
--

commit 4b538833b03e67798c223181a1b410cee9126502
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

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

Before should_reclaim_retry() gives up on reclaim, this commit drains
RCU-deferred pages directly to the buddy lists and retries the allocation
if those pages along with reclaimable memory would bring an eligible zone
above its minimum watermark.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a9058d414f92d..9caaa57bf51d7 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c

[ ... ]

> @@ -1398,7 +1405,7 @@ static void rcu_free_workfn(struct work_struct *work)
>  	rcu_free_start(r);
>  	spin_unlock_irq(&r->lock);
>  
> -	rcu_free_pages(&done);
> +	rcu_free_pages(&done, false);
>  }
>  
>  static void rcu_free_expedite_workfn(struct work_struct *work)
> @@ -1411,7 +1418,7 @@ static void rcu_free_expedite_workfn(struct work_struct *work)
>  	rcu_free_take(r, &list);
>  	spin_unlock_irq(&r->lock);
>  
> -	if (rcu_free_flush(&list))
> +	if (rcu_free_flush(&list, false))
>  		count_vm_event(PGFREE_RCU_EXPEDITE);
>  }
>  
> @@ -1445,6 +1452,33 @@ static void rcu_free_defer(struct folio *folio)
>  		queue_work(rcu_free_wq, &r->expedite);
>  }
>  
> +/*
> + * Free what every CPU's lists hold, straight to the buddy lists: the
> + * callers want to allocate the pages next.  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, true))
> +		count_vm_event(PGFREE_RCU_DRAIN);
> +	/* An expedite in flight holds pages it took off the lists */
> +	for_each_possible_cpu(cpu)
> +		flush_work(&per_cpu_ptr(&rcu_free_pcpu, cpu)->expedite);
> +	mutex_unlock(&lock);
> +}

[Severity: High]
As reported by Sashiko on RFC [1], what happens if the pages counted in
NR_FREE_RCU_PAGES were already spliced off r->batch or r->pending by an
in-flight rcu_free_workfn() or rcu_free_expedite_workfn() when
rcu_free_drain() runs?

First, rcu_free_workfn() splices r->batch onto its stack-local done list
under r->lock and then drops the lock before calling
rcu_free_pages(&done, false), which decrements NR_FREE_RCU_PAGES one folio
at a time and can sleep at cond_resched_tasks_rcu_qs(). Because
rcu_free_drain() only calls flush_work(&r->expedite) and does not flush
r->rwork.work, rcu_free_take() can find the per-CPU lists empty and
rcu_free_drain() can return before rcu_free_workfn() finishes freeing its
done list.

Second, both rcu_free_workfn() and rcu_free_expedite_workfn() call
rcu_free_pages() with direct set to false, which passes pcp_allowed_order()
folios to free_unref_folios() and places them onto the worker CPU's per-CPU
pageset lists rather than the buddy lists while decrementing
NR_FREE_RCU_PAGES to 0.

On the subsequent retry in __alloc_pages_slowpath(), pages sitting on PCP
lists are not counted in NR_FREE_PAGES, and __alloc_pages_direct_reclaim()
skips drain_all_pages(NULL) when __perform_reclaim() makes no progress. On
the next should_reclaim_retry() call, NR_FREE_RCU_PAGES is 0 and
NR_FREE_PAGES is still below min_wmark_pages(zone). Could this cause
__alloc_pages_slowpath() to proceed to __alloc_pages_may_oom() while those
freed pages remain stranded on PCP lists?

[ ... ]

> @@ -4792,6 +4826,58 @@ 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,
> + * with what reclaim can still free, 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_reclaimable_pages(zone) +
> +			    zone_page_state_snapshot(zone, NR_FREE_PAGES);
> +		/*
> +		 * Count pages only: for order > 0 __zone_watermark_ok() also
> +		 * wants a free block of that order, and the waiting pages are
> +		 * not free yet.
> +		 */
> +		if (__zone_watermark_ok(zone, 0, min_wmark_pages(zone),
> +					ac->highest_zoneidx, alloc_flags,
> +					available)) {

[Severity: Medium]
When should_reclaim_retry() reaches out: via the
*no_progress_loops > MAX_RECLAIM_RETRIES check, direct reclaim has failed
to make progress 16 times in a row even though
zone_reclaimable_pages(zone) + NR_FREE_PAGES was above min_wmark_pages(zone)
(for instance under GFP_NOFS or when remaining LRU pages are dirty, under
writeback, or pinned).

In that situation, since rcu_free_wait() still includes
zone_reclaimable_pages(zone) in available, won't
zone_reclaimable_pages(zone) + NR_FREE_PAGES already exceed
min_wmark_pages(zone) on its own, causing even a single waiting page
(waiting == 1) to trigger up to MAX_RECLAIM_RETRIES additional
rcu_free_drain() calls and no-progress reclaim loops while also skipping
the PF_WQ_WORKER sleep in should_reclaim_retry()?

> +			(*rcu_free_waits)++;
> +			rcu_free_drain();
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * Checks whether it makes sense to retry the reclaim to make a forward progress
>   * for the given allocation request.
> @@ -4805,7 +4891,8 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  static inline bool
>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		     struct alloc_context *ac, int alloc_flags,
> -		     bool did_some_progress, int *no_progress_loops)
> +		     bool did_some_progress, int *no_progress_loops,
> +		     int *rcu_free_waits)
>  {
>  	struct zone *zone;
>  	struct zoneref *z;
> @@ -4816,10 +4903,12 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  	 * their order will become available due to high fragmentation so
>  	 * always increment the no progress counter for them
>  	 */
> -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER) {
>  		*no_progress_loops = 0;
> -	else
> +		*rcu_free_waits = 0;
> +	} else {
>  		(*no_progress_loops)++;
> +	}
>  
>  	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
>  		goto out;
> @@ -4872,6 +4961,10 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  	else
>  		cond_resched();
>  out:
> +	if (!ret && rcu_free_wait(gfp_mask, order, ac, alloc_flags,
> +				  rcu_free_waits))
> +		return true;

[Severity: High]
Can this starve compaction for non-costly high-order allocations
(1 <= order <= PAGE_ALLOC_COSTLY_ORDER)?

In should_reclaim_retry(), the zone loop calls:

    wmark = __zone_watermark_ok(zone, order, min_wmark,
            ac->highest_zoneidx, alloc_flags, available);

For order > 0, __zone_watermark_ok() requires a free block of at least that
order in zone->free_area[]. When a high-order allocation fails due to
fragmentation despite having plenty of order-0 memory
(reclaimable + NR_FREE_PAGES > min_wmark), should_reclaim_retry() previously
returned false so __alloc_pages_slowpath() could reach
should_compact_retry() (to escalate compact_priority toward
COMPACT_PRIO_SYNC_FULL) and relax ALLOC_NOFRAGMENT:

    if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
                             did_some_progress > 0, &no_progress_loops,
                             &rcu_free_waits))
            goto retry;

    if (did_some_progress > 0 && can_compact &&
        should_compact_retry(gfp_mask, ac, order, alloc_flags,
                             compact_result, &compact_priority,
                             &compaction_retries))
            goto retry;

With rcu_free_wait() checking __zone_watermark_ok() at order 0, any non-zero
NR_FREE_RCU_PAGES count (including pages just evicted by direct reclaim
itself) satisfies the order-0 watermark check whenever order-0 memory is
already above min_wmark.

Furthermore, because __alloc_pages_direct_reclaim() sets did_some_progress
whenever it reclaims pages or sees sc->compaction_ready,
should_reclaim_retry() resets *rcu_free_waits = 0 on every iteration.
Doesn't this cause __alloc_pages_slowpath() to loop on goto retry via
should_reclaim_retry(), repeatedly firing synchronize_rcu_expedited() in
rcu_free_drain() and reclaiming page cache without ever reaching
should_compact_retry()?

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

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