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

[Kairui Song]

Forwarding a sashiko review, which I think part of it makes sense:

> commit d1c559ae5b78a93fe3aef7c2115b93fc96970a6f
> Author: Kairui Song <kasong@xxxxxxxxxxx>
> mm/mglru: restructure the reclaim loop
> This commit decouples aging and rotation in the MGLRU reclaim loop. It
> calculates the scan number once per reclaim pass and consistently applies
> priority shifting. The behavior for offline memcgs and default priority
> aging is adjusted to make rotation more explicit.
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 963362523782a..d4aaaa62056d9 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> [ ... ]
> > +static long get_nr_to_scan(struct lruvec *lruvec, struct scan_control *sc,
> > +    struct mem_cgroup *memcg, int swappiness)
> >  {
> [ ... ]
> >  nr_to_scan = apply_proportional_protection(memcg, sc, nr_to_scan);
> > + nr_to_scan >>= sc->priority;
> >
> > + if (!nr_to_scan && sc->priority < DEF_PRIORITY)
> > + nr_to_scan = min(evictable, SWAP_CLUSTER_MAX);
> >
> > + return nr_to_scan;
> >  }
> Does the new minimum batch fallback invert proportional memory protection?
> When an unprotected memcg has 1024 evictable pages at priority 10, the
> scan target evaluates to 1 (1024 >> 10) and bypasses the fallback. However,
> a protected memcg of the exact same size has its target floored to
> SWAP_CLUSTER_MAX (32) before shifting. This evaluates to 0 (32 >> 10),
> triggering the fallback and forcing a scan of 32 pages. This results in
> the protected memcg being scanned much more aggressively than the
> unprotected one.

No? This part of the review is not correct.
Proportional memory protection have a much more aggressive
batch fallback. We are already very cautious here.

> >  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);
> > +
> > + nr_to_scan = get_nr_to_scan(lruvec, sc, memcg, swappiness);
> > + if (!nr_to_scan)
> > + need_rotate = true;
> >
> > - while (true) {
> > + while (nr_to_scan > 0) {
> Could setting need_rotate for zero scan targets shield small memcgs from
> reclaim?
> If a memcg has fewer than 4096 evictable pages, its scan target at
> DEF_PRIORITY evaluates to 0. Setting need_rotate to true causes shrink_one()
> to return MEMCG_LRU_YOUNG, moving the memcg to the young generation.
> Since subsequent global reclaim passes with higher priorities iterate over
> the old generation, this might completely isolate these small memcgs from
> memory reclaim.

This concern makes sense.

I think the need_rotate = true above can be just dropped then,
it was added due to previous concern that tiny cgroups don't have a
chance to be rotated, but now they might get overly rotated
indeed. Meanwhile now we already have a min batch limit above
when priority < DEF_PRIORITY, and we never do aging (before or
after this patch) at DEF_PRIORITY anyway. So if priority has be
escalated, we will
always enter the reclaim loop and have the chance to do aging / rotation.

Removing this need_rotate for !nr_to_scan looks perfectly fine to me now,
and cleaner than before.