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

[Barry Song]

On Sun, Aug 30, 2026 at 11:59 AM Kunwu Chan <kunwu.chan@xxxxxxxxx> wrote:
>
> On Fri, 28 Aug 2026 07:46:59 +0800 "Barry Song (Xiaomi)" <baohua@xxxxxxxxxx> 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().
> >
> > Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > Tested-by: Xueyuan Chen <xueyuan.chen21@xxxxxxxxx>
> > ---
> >  mm/vmscan.c | 23 ++++++++++++++++++-----
[...]
> >
> >                       if (!--remaining)
> > -                             return false;
> > +                             break;
> >               }
> > +             WRITE_ONCE(lrugen->nr_pages[old_gen][type][zone],
> > +                        lrugen->nr_pages[old_gen][type][zone] - delta);
> > +             WRITE_ONCE(lrugen->nr_pages[target_gen][type][zone],
> > +                        lrugen->nr_pages[target_gen][type][zone] + delta);
>
> Hi Barry,
>
> One subtle point about the `remaining` handling:
> when `remaining` reaches zero, we now `break` rather than return
> so that the accumulated `delta` is applied before returning.
>
> As I understand it, this is required because `__folio_inc_gen()`
> has already changed the generation of the scanned folios,
> while `lrugen->nr_pages[]` is now updated only in batch.
>
> So the invariant is that every successful generation increment must
> have its corresponding `delta` flushed before `inc_min_seq()` returns.
>
> Is this the intended accounting invariant?

Yes, Kunwu, you're right. We can't return when `remaining == 0`,
otherwise we will miss the chance to flush the delta.

So changing `return` to `break` is intentional. Another option would be
to duplicate the code that flushes the delta and keep the `return`, but
that would be ugly since we would have to duplicate the code below:

             WRITE_ONCE(lrugen->nr_pages[old_gen][type][zone],
                       lrugen->nr_pages[old_gen][type][zone] - delta);
             WRITE_ONCE(lrugen->nr_pages[target_gen][type][zone],
                        lrugen->nr_pages[target_gen][type][zone] + delta);

Thanks
Barry