Re: [PATCH v3 04/14] mm/mglru: restructure the reclaim loop

[Kairui Song]

Forwarding sashiko review:

https://sashiko.dev/#/patchset/20260403-mglru-reclaim-v3-0-a285efd6ff91%40tencent.com?part=4

> commit 89b3058e01113eb027062254e73446d398af330a
> Author: Kairui Song <kasong@xxxxxxxxxxx>
> mm/mglru: restructure the reclaim loop
> This commit restructures the MGLRU reclaim loop by calculating the scan
> number once at the beginning of reclaim and decoupling aging from rotation.
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 963362523782a..93ffb3d98fedf 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> [ ... ]
> > -static long get_nr_to_scan(struct lruvec *lruvec, struct scan_control *sc, int swappiness)
> > +static long get_nr_to_scan(struct lruvec *lruvec, struct scan_control *sc,
> > +			   struct mem_cgroup *memcg, int swappiness)
> >  {
> > -	bool need_aging;
> > -	unsigned long nr_to_scan;
> > -	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
> > -	DEFINE_MAX_SEQ(lruvec);
> > -
> > -	if (mem_cgroup_below_min(sc->target_mem_cgroup, memcg))
> > -		return -1;
> > -
> > -	need_aging = should_run_aging(lruvec, max_seq, swappiness, &nr_to_scan);
> > +	unsigned long evictable, nr_to_scan;
> >  
> > +	evictable = lruvec_evictable_size(lruvec, swappiness);
> > +	nr_to_scan = evictable;
> [ ... ]
> >  	nr_to_scan = apply_proportional_protection(memcg, sc, nr_to_scan);
> >  
> > -	/* try to get away with not aging at the default priority */
> > -	if (!need_aging || sc->priority == DEF_PRIORITY)
> > -		return nr_to_scan >> sc->priority;
> > +	/*
> > +	 * Always respect scan priority, minimally target some folios
> > +	 * to keep reclaim moving forwards.
> > +	 */
> > +	nr_to_scan >>= sc->priority;
> > +	if (!nr_to_scan)
> > +		nr_to_scan = min(evictable, SWAP_CLUSTER_MAX);
> When falling back to a minimum scan size, does this bypass the memory.low
> protection applied earlier by apply_proportional_protection()?
> Since the fallback uses the original unprotected evictable value rather
> than the protected nr_to_scan calculated prior to the priority shift, it
> appears protected memcgs will evaluate to min(evictable, 32) = 32
> and be forcefully scanned, bypassing the protection.
> Could this also cause an inversion based on memcg size?
> For example, an unprotected memcg with 4,000 folios shifts to 0 at
> DEF_PRIORITY (4000 >> 12) and falls back to 32 folios, whereas a larger
> memcg with 4,096 folios shifts to 1 (4096 >> 12) and scans only 1 folio.

That's really trivial I think.

The low protection calculation already has a "max(scan, SWAP_CLUSTER_MAX)"
check. If it's concerning here then the
nr_to_scan = min(evictable, SWAP_CLUSTER_MAX) can be dropped. And actually
in previous version of this series there isn't such check and it was
concerned that always apply the priority shift could lead to inefficient
reclaim of small cgroup. Having the min check here seems safer and
make the reclaim loop a bit more like what it was.

In theory and in practise there isn't much difference since only very
tiny cgroups are slightly affected, and in most cases the reclaimer
already starts with requesting SWAP_CLUSTER_MAX folios to be reclaimed
(nr_to_reclaim).

Maybe some chore cleanup can be done for a perfect accurate reclaim
budget starting from the reclaim start point. Not sure if that
would be a practical thing to do though.

> > -	/* stop scanning this lruvec as it's low on cold folios */
> > -	return try_to_inc_max_seq(lruvec, max_seq, swappiness, false) ? -1 : 0;
> > +	return nr_to_scan;
> >  }
> [ ... ]
> > +static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
> >  {
> > +	bool need_rotate = false;
> >  	long nr_batch, nr_to_scan;
> > -	unsigned long scanned = 0;
> >  	int swappiness = get_swappiness(lruvec, sc);
> > +	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
> >  
> > -	while (true) {
> > +	nr_to_scan = get_nr_to_scan(lruvec, sc, memcg, swappiness);
> > +	while (nr_to_scan > 0) {
> >  		int delta;
> > +		DEFINE_MAX_SEQ(lruvec);
> >  
> > -		nr_to_scan = get_nr_to_scan(lruvec, sc, swappiness);
> > -		if (nr_to_scan <= 0)
> > +		if (mem_cgroup_below_min(sc->target_mem_cgroup, memcg)) {
> > +			need_rotate = true;
> >  			break;
> > +		}
> > +
> > +		if (should_run_aging(lruvec, max_seq, sc, swappiness)) {
> > +			if (try_to_inc_max_seq(lruvec, max_seq, swappiness, false))
> > +				need_rotate = true;
> > +			break;
> > +		}
> If an lruvec has zero evictable folios in its oldest generation,
> get_nr_to_scan() will return 0, and this loop will be completely bypassed.

No, get_nr_to_scan won't return zero if aging helps get this LRU
evictable. And if even aging won't help there is no point to
reclaim it.

> Because the memcg rotation check (mem_cgroup_below_min) and the MGLRU aging
> logic (should_run_aging) are now entirely inside this loop, will this leave
> an lruvec permanently stalled?

There won't be stall, in worst case empty memcgs filled all the
random bucket, it just requires a few more iteration to find a
reclaimable memcg.

But on second though, if get_nr_to_scan returns 0 in the first try,
try_to_shrink_lruvec should just return true to rotate unevictable
LRU to speed up following reclaim indeed. The old behavior is a bit
fuzzy about this since the rotation and not doing the best
choice either, roration is decided by if an aging success but
an unevictable LRU doesn't always trigger aging.

This part can be improved easily while at it, should be a
nice micro optimization, would do that if V3 is sent.