Re: [PATCH 1/6] mm/mglru: batch update lrugen->nr_pages in inc_min_seq()

[Kairui Song]

On Thu, Aug 27, 2026 at 7:21 PM Barry Song <baohua@xxxxxxxxxx> wrote:
>
> On Thu, Aug 27, 2026 at 11:21 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> >
> > On Fri, Aug 21, 2026 at 7:09 PM 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>
> > > ---
> > >  mm/vmscan.c | 46 +++++++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > Hello Barry
> >
> > Thanks for the patch!
> >
> > >
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index c1404a59523d..0d74fc00abd3 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -3296,20 +3296,21 @@ static int folio_update_gen(struct folio *folio, int gen, const vma_flags_t *vma
> > >  }
> > >
> > >  /* protect pages accessed multiple times through file descriptors */
> > > -static int folio_inc_gen(struct lruvec *lruvec, struct folio *folio)
> > > +static int __folio_inc_gen(struct folio *folio, int old_gen, bool *increased)
> >
> > I feel the naming is a bit confusing, the __ prefix doesn't tell how
> > it differs from folio_inc_gen very well, maybe just name one
> > folio_inc_gen (the old gen could be any gen), another one is
> > folio_inc_min_gen (the old gen can only be min_seq), and with sanity
> > check in folio_inc_min_gen that expects get_nr_gens == 4, and
> > lru_gen_is_active(min_seq) == lru_gen_is_active(min_seq + 1)? This
> > could be a build-time sanity check instead of a runtime debug check.
>
> Thanks very much for your suggestion, Kairui.
>
> I tried splitting this into two functions, folio_inc_min_gen() and
> folio_inc_gen(). I tried a couple of approaches, but I think I’ll give up
> on this direction.
> folio_inc_min_gen() and folio_inc_gen() share some common code while
> also having some differences. It’s hard to avoid ugly code duplication
> because folio_inc_gen() cannot call folio_inc_min_gen(). However,
> folio_inc_gen() can call __folio_inc_gen().
>
> The __ prefix currently indicates that we don’t update the LRU sizes in
> this case. The sizes can either be updated in a batch later or updated once
> by folio_inc_gen().
> Maybe we could add a comment to make this easier to read, or use a better
> name for __folio_inc_gen()?

OK, thanks for the explanation. Let's just keep the name then.