Re: [PATCH v3 2/7] mm/mglru: batch update lrugen->nr_pages in inc_min_seq()

[Barry Song]

On Thu, Sep 3, 2026 at 6:06 PM Baolin Wang
<baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> On 9/2/26 7:24 AM, Barry Song (Xiaomi) wrote:
> > Currently, folio_inc_gen() updates lrugen->nr_pages for every folio
> > as it advances generations. Instead, accumulate the size changes
> > and update lrugen->nr_pages in a batch after scanning the entire
> > oldest generation, or when the scan stops because remaining reaches
> > zero.
> >
> > Since we only move folios from the oldest generation to the second
> > oldest generation, the active/inactive state cannot change. We can
> > therefore skip __lru_update_size().
>
> Make sense to me.
>
> >
> > Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > Tested-by: Xueyuan Chen <xueyuan.chen21@xxxxxxxxx>
> > Reviewed-by: Lian Wang <lianux.mm@xxxxxxxxx>
> > Reviewed-by: Kunwu Chan <kunwu.chan@xxxxxxxxx>
> > ---
>
> LGTM. One nit follows.
> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>

Thanks very much!

>
> >   mm/vmscan.c | 23 ++++++++++++++++++-----
> >   1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 54bce2f608ef..27494505cecc 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -3918,6 +3918,7 @@ static bool inc_min_seq(struct lruvec *lruvec, int type, int swappiness)
> >       struct lru_gen_folio *lrugen = &lruvec->lrugen;
> >       int hist = lru_hist_from_seq(lrugen->min_seq[type]);
> >       int new_gen, old_gen = lru_gen_from_seq(lrugen->min_seq[type]);
> > +     int target_gen = (old_gen + 1) % MAX_NR_GENS;
> >
> >       /* For file type, skip the check if swappiness is anon only */
> >       if (type && (swappiness == SWAPPINESS_ANON_ONLY))
> > @@ -3927,35 +3928,47 @@ static bool inc_min_seq(struct lruvec *lruvec, int type, int swappiness)
> >       if (!type && !swappiness)
> >               goto done;
> >
> > +     VM_WARN_ON_ONCE(get_nr_gens(lruvec, type) != MAX_NR_GENS);
> > +     VM_WARN_ON_ONCE(lru_gen_is_active(lruvec, old_gen) !=
> > +                     lru_gen_is_active(lruvec, target_gen));
> >       /* prevent cold/hot inversion if the type is evictable */
> >       for (zone = 0; zone < MAX_NR_ZONES; zone++) {
> >               struct list_head *head = &lrugen->folios[old_gen][type][zone];
> > +             long delta = 0;
> >
> >               while (!list_empty(head)) {
> >                       struct folio *folio = lru_to_folio(head);
> > +                     long nr_pages = folio_nr_pages(folio);
> >                       int refs = folio_lru_refs(folio);
> >                       bool workingset = folio_test_workingset(folio);
> > +                     bool gen_increased;
> >
> >                       VM_WARN_ON_ONCE_FOLIO(folio_test_unevictable(folio), folio);
> >                       VM_WARN_ON_ONCE_FOLIO(folio_test_active(folio), folio);
> >                       VM_WARN_ON_ONCE_FOLIO(folio_is_file_lru(folio) != type, folio);
> >                       VM_WARN_ON_ONCE_FOLIO(folio_zonenum(folio) != zone, folio);
> >
> > -                     new_gen = folio_inc_gen(lruvec, folio);
> > +                     new_gen = __folio_inc_gen(folio, old_gen, &gen_increased);
>
> IMO, it's better to add some comments to describe why we don't need to
> call __lru_update_size(), in case someone thinks this needs to be fixed
> in the future. :)

We have already said this twice—once in the changelog and once with
the `VM_WARN_ON()` in C :-)

+     VM_WARN_ON_ONCE(lru_gen_is_active(lruvec, old_gen) !=
+                     lru_gen_is_active(lruvec, target_gen));

I guess we don't need to say it a third time, do we? It should be
self-documenting.

For a possible future change, I know Kairui may change the definition
of active/inactive—for example, by no longer using generations to
define them, but instead using refs. Kairui knows the MGLRU code
inside out, though, so I'm not too concerned. :-)

Best Regards
Barry