Re: [PATCH v3 01/14] mm/mglru: consolidate common code for retrieving evictable size

[Kairui Song]

Forwarding sashiko review in case anyone has concern about it:

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

> commit e0119c3e147f68096f59fb96e1e69e83ecdfce40
> Author: Kairui Song <kasong@xxxxxxxxxxx>
> mm/mglru: consolidate common code for retrieving evictable size
> This commit merges commonly used code for counting evictable folios in a
> lruvec into a new helper function lruvec_evictable_size.
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 5a8c8fcccbfc9..adc07501a1375 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> [ ... ]
> > @@ -4909,9 +4915,6 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> >  static bool should_run_aging(struct lruvec *lruvec, unsigned long max_seq,
> >  			     int swappiness, unsigned long *nr_to_scan)
> >  {
> > -	int gen, type, zone;
> > -	unsigned long size = 0;
> > -	struct lru_gen_folio *lrugen = &lruvec->lrugen;
> >  	DEFINE_MIN_SEQ(lruvec);
> >  
> >  	*nr_to_scan = 0;
> > @@ -4919,18 +4922,7 @@ static bool should_run_aging(struct lruvec *lruvec, unsigned long max_seq,
> >  	if (evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS > max_seq)
> >  		return true;
> >  
> > -	for_each_evictable_type(type, swappiness) {
> > -		unsigned long seq;
> > -
> > -		for (seq = min_seq[type]; seq <= max_seq; seq++) {
> > -			gen = lru_gen_from_seq(seq);
> > -
> > -			for (zone = 0; zone < MAX_NR_ZONES; zone++)
> > -				size += max(READ_ONCE(lrugen->nr_pages[gen][type][zone]), 0L);
> > -		}
> > -	}
> > -
> > -	*nr_to_scan = size;
> > +	*nr_to_scan = lruvec_evictable_size(lruvec, swappiness);
> The commit message notes there is no behavior change, but does delegating
> to lruvec_evictable_size() introduce a subtle state inconsistency?
> Previously, should_run_aging() evaluated the aging conditions and iterated
> the generations to calculate the evictable size using a single snapshot of
> max_seq (passed as an argument) and min_seq (read locally via
> DEFINE_MIN_SEQ).
> The new lruvec_evictable_size() helper re-reads both sequence numbers
> internally via its own DEFINE_MAX_SEQ and DEFINE_MIN_SEQ macros. If the
> sequence numbers advance concurrently, the calculated size might be based
> on a different snapshot than the one used just lines above to determine if
> aging should run.
> Could this lead to unintended behavioral changes during concurrent
> execution?

This is totally fine.

Right lruvec_evictable_size re-reads max_seq - and that's really trivial,
I don't think anyone will observe any different in any way. The time
windows is so tiny and even if a different value is read, the updated
value is more accurate for getting the LRU size.