Re: [RFC PATCH 3/4] mm/vmscan: drop the combined limit gate in __node_reclaim()

On Fri, Aug 21, 2026 at 04:17:40PM +0800, Ridong Chen wrote:
> From: Ridong Chen <[email protected]>
> 
> __node_reclaim() only ran shrink_node() when unmapped page cache was
> over min_unmapped_pages OR reclaimable slab was over min_slab_pages.
> 
> With slab and file reclaim now gated per type by sc->skip_slab_reclaim and
> sc->skip_file_reclaim, this combined gate is either redundant or harmful:
> 
>  - for the NUMA node reclaim caller it is always true, since
>    node_reclaim() only calls in when at least one limit is exceeded;
> 
>  - for the per-node proactive reclaim caller (which does not go through
>    node_reclaim()'s checks) it wrongly suppressed all reclaim -- anon
>    included -- whenever both page cache and slab happened to sit at or
>    below their limits, even with plenty of reclaimable anon present.
> 
> Drop the gate and let the per-type flags decide what to reclaim.  The
> node reclaim path is unchanged; the proactive path can now reclaim anon
> as requested.
> 
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Ridong Chen <[email protected]>

Could we go with this patch and table the rest of the series?

These minimums are specifically for zone_reclaim_mode. I don't know
who is using that at this point, and it seems the behavior has been
like that for a while with no practical complaints. So my take is,
leave it until somebody has a real problem.

Applying those limits to proactive reclaim, on the other hand, wasn't
intentional, isn't documented, and can lead to unexpected behavior -
considering we have non-zero default values on these knobs.

Since the gate was already redundant for some reason, leaving it in
node_reclaim() (zone_reclaim_mode) and killing it in __node_reclaim()
(the path shared with proactive reclaim), like you did here, sounds
like the best way forward for now to me.

With an updated changelog to that end,

Acked-by: Johannes Weiner <[email protected]>

> ---
>  mm/vmscan.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 1e56973ceb73..5a3f67b3ba32 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -7906,16 +7906,16 @@ static unsigned long __node_reclaim(struct pglist_data *pgdat,
>  	noreclaim_flag = memalloc_noreclaim_save();
>  	set_task_reclaim_state(p, &sc->reclaim_state);
>  
> -	if (node_pagecache_reclaimable(pgdat) > pgdat->min_unmapped_pages ||
> -	    node_page_state_pages(pgdat, NR_SLAB_RECLAIMABLE_B) > pgdat->min_slab_pages) {
> -		/*
> -		 * Free memory by calling shrink node with increasing
> -		 * priorities until we have enough memory freed.
> -		 */
> -		do {
> -			shrink_node(pgdat, sc);
> -		} while (sc->nr_reclaimed < nr_pages && --sc->priority >= 0);
> -	}
> +	/*
> +	 * Free memory by calling shrink node with increasing
> +	 * priorities until we have enough memory freed.

Might as well drop this comment. It just says what the code does.

> +	 *
> +	 * What to reclaim is gated per type by sc->skip_slab_reclaim and
> +	 * sc->skip_file_reclaim.
> +	 */
> +	do {
> +		shrink_node(pgdat, sc);
> +	} while (sc->nr_reclaimed < nr_pages && --sc->priority >= 0);
>  
>  	set_task_reclaim_state(p, NULL);
>  	memalloc_noreclaim_restore(noreclaim_flag);