Re: [PATCH] mm: migrate LRU_REFS_MASK bits in folio_migrate_flags

[Yu Zhao <yuzhao@google.com>]

On Wed, Sep 25, 2024 at 5:50 AM Zhaoyang Huang <huangzhaoyang@gmail.com> wrote:
>
> On Wed, Sep 25, 2024 at 5:42 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 25 Sep 2024 11:02:25 +0800 "zhaoyang.huang" <zhaoyang.huang@unisoc.com> wrote:
> >
> > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > >
> > > Bits of LRU_REFS_MASK are not inherited during migration which lead to
> > > new_folio start from tier0. Fix this by migrate the bits domain.
> >
> > I'm having trouble understanding this, sorry.  Please more fully
> > describe the runtime effects of this flaw.
> Sorry for bringing confusion. According to my understanding, MGLRU
> records how many times that vfs access this page in a range of bits
> domain(LRU_REFS_MASK) in folio->flags which are not being migrated to
> new_folios so far. This commit would like to do so to have the
> new_folio inherit these bits from the old folio. Is it right and
> worthy to do?

Correct. And this was not done because
1. While LRU_REFS_MASK is useful, but it's not important, and
2. More important information, e.g., generations, is lost during migration.

So I'm not sure how much this patch can help. But you think it does, why not.

> > > --- a/include/linux/mm_inline.h
> > > +++ b/include/linux/mm_inline.h
> > > @@ -291,6 +291,12 @@ static inline bool lru_gen_del_folio(struct lruvec *lruvec, struct folio *folio,
> > >       return true;
> > >  }
> > >
> > > +static inline void folio_migrate_refs(struct folio *new_folio, struct folio *folio)

Nitpick: folio_migrate_refs(struct folio *new, struct folio *old)

> > > +{
> > > +     unsigned long refs = READ_ONCE(folio->flags) & LRU_REFS_MASK;
> > > +
> > > +     set_mask_bits(&new_folio->flags, LRU_REFS_MASK, refs);
> > > +}
> > >  #else /* !CONFIG_LRU_GEN */
> > >
> > >  static inline bool lru_gen_enabled(void)
> > > @@ -313,6 +319,8 @@ static inline bool lru_gen_del_folio(struct lruvec *lruvec, struct folio *folio,
> > >       return false;
> > >  }
> > >
> > > +static inline void folio_migrate_refs(struct folio *new_folio, struct folio *folio)

Ditto.

> > > +{}

A line break between "{" and "}".

> > >  #endif /* CONFIG_LRU_GEN */
> > >
> > >  static __always_inline
> > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > index 923ea80ba744..60c97e235ae7 100644
> > > --- a/mm/migrate.c
> > > +++ b/mm/migrate.c
> > > @@ -618,6 +618,7 @@ void folio_migrate_flags(struct folio *newfolio, struct folio *folio)
> > >       if (folio_test_idle(folio))
> > >               folio_set_idle(newfolio);
> > >
> > > +     folio_migrate_refs(newfolio, folio);
> > >       /*
> > >        * Copy NUMA information to the new page, to prevent over-eager
> > >        * future migrations of this same page.
> > > --
> > > 2.25.1