Re: [PATCH v4 6/6] mm/mglru: fix potential generation folio number leak

[Barry Song]

On Tue, Sep 1, 2026 at 7:14 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
[...]
> > >  static inline int folio_is_file_lru(const struct folio *folio)
> > >  {
> > > -       return !folio_test_swapbacked(folio);
> > > +       return folio_flags_is_file_lru(const_folio_flags(folio, 0));
> > >  }
> >
> > I guess we don't need to change this function? It seems more natural to
> > me to keep using `!folio_test_swapbacked(folio)` here.
> >
> > In `folio_update_gen()`, you already use
> > `*is_file = folio_flags_is_file_lru(&old_flags)` to get the type from
> > `old_flags`. I guess that's all we need?
>
> Yeah, that's all we need, I was thinking that using
> folio_flags_is_file_lru here can avoid any further change to
> PG_swapbacked causing inconsistency of folio_is_file_lru and
> folio_flags_is_file_lru, seems a bit easier to maintain.

Not quite sure. folio_is_file_lru() is based on the folio, so I guess we could
just check against the folio with folio_test_swapbacked(folio)?

Anyway, I’m not particularly strong on this. Either way is fine with me.

>
> >
> > >
> > >  static __always_inline void __update_lru_size(struct lruvec *lruvec,
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 724b6e034e69..87e667c410ed 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -3269,7 +3269,8 @@ static bool positive_ctrl_err(struct ctrl_pos *sp, struct ctrl_pos *pv)
> > >   ******************************************************************************/
> > >
> > >  /* promote pages accessed through page tables */
> > > -static int folio_update_gen(struct folio *folio, int new_gen, const vma_flags_t *vma_flags)
> > > +static int folio_update_gen(struct folio *folio, int new_gen, int *is_file,
> >
> > Rather than is_file, I feel was_file would be more accurate here,
> > since it doesn't represent what the folio is now.
>
> I'm fine either way, is_file seems more natual to me. Or maybe simply
> rename it as "type"? Just the below:
>
> > > @@ -3532,9 +3533,9 @@ static void walk_update_folio(struct lru_gen_mm_walk *walk, struct vm_area_struc
> > >                 folio_mark_dirty(folio);
> > >
> > >         if (walk) {
> > > -               old_gen = folio_update_gen(folio, new_gen, &vma->flags);
> > > +               old_gen = folio_update_gen(folio, new_gen, &file, &vma->flags);
> > >                 if (old_gen >= 0 && old_gen != new_gen)
> > > -                       update_batch_size(walk, folio, old_gen, new_gen);
> > > +                       update_batch_size(walk, folio, old_gen, new_gen, file);
> >
> > This is one more place where we rely on `LRU_GEN_ANON = 0` and
> > `LRU_GEN_FILE = 1`.
> >
> > We do this kind of thing quite often in MGLRU, such as `type = !type`.
> > It relies on the numeric values of the type constants, which is sort of
> > a semantic abuse, but it's hard to find a shorter way to express it.
>
> We also ahve WORKINGSET_ANON, WORKINGSET_FILE, ANON_AND_FILE. I can
> rename the newly added parameter to "type" here if that looks better.

Yes. Using the type seems clearer and easier to understand, and
it’s also consistent across multiple functions.

Best Regards
Barry