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

[KunWu Chan]

On Sun, Aug 30, 2026 at 12:26 PM Barry Song <baohua@xxxxxxxxxx> wrote:
>
> 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:
>

Hi Barry,

Thanks for confirming the invariant and the control flow reasoning.
The logic makes complete sense to me now.

Reviewed-by: Kunwu Chan <kunwu.chan@xxxxxxxxx>

Thanks,
KunWu

>              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