Re: [PATCH RFC v3 13/17] mm/mglru: make folio_inc_lru_refs lruvec lockless

[KunWu Chan]

On Thu, Oct 15, 2026 at 11:46 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
> On Mon, Oct 05, 2026 at 01:29:00AM +0800, KunWu Chan wrote:
> > On Sat, Oct 3, 2026 at 8:55 PM Kairui Song via B4 Relay
> > <devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
> > >
> > > From: Kairui Song <kasong@xxxxxxxxxxx>
> > >
> > > The lruvec spinlock in folio_inc_lru_refs was only taken to keep
> > > concurrent aging from corrupting the size counters.  This is no
> > > longer needed: the per-generation counters are atomic, and the
> > > active/inactive ABI counters are updated per folio and linearized by
> > > the CAS on the folio flags, so concurrent aging can no longer corrupt
> > > them.  The same argument already allowed folio_reset_lru_refs() to
> > > drop the lock.
> > >
> > > Use folio_lruvec_live_get()/folio_lruvec_live_put() to hold the RCU
> > > read lock around the lruvec lookup, and drop the spinlock entirely.
> > >
> > > Without the lock, aging can advance max_seq while a promotion is in
> > > flight.  Generations are indexed by seq % MAX_NR_GENS, so the folio
> > > flags can return to the value read earlier and the cmpxchg succeeds
> > > with a target gen computed from a stale max_seq.  Recheck max_seq after
> > > a gen-changing cmpxchg: if it moved, account the committed update and
> > > retry with LRU_REF_FORCE, which promotes the folio to the newest gen.
> > > The retry may count the access twice, which is acceptable and avoids
> > > folio_activate(), whose LRU lock causes latency jitter.  smp_rmb()
> > > orders the flags read, including the one returned by a failed
> > > cmpxchg(), before the max_seq read.
> > >
> > > Also simplify the post-CAS accounting guards: gen is only assigned
> > > non-negative values after the old_gen < 0 early exit, so gen != old_gen
> > > implies gen >= 0, and lru_gen_update_size() needs no explicit guard.
> > > The active/inactive ABI update keeps its gen >= 0 check because an
> > > off-LRU folio has no lruvec to update.
> > >
> > > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > > ---
> > >  include/linux/mm_inline.h | 10 +++++-----
> > >  include/linux/mmzone.h    |  3 ++-
> > >  mm/vmscan.c               | 47 ++++++++++++++++++++++++++++-------------------
> > >  3 files changed, 35 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> > > index ee7fcbe2366b..b68d68101248 100644
> > > --- a/include/linux/mm_inline.h
> > > +++ b/include/linux/mm_inline.h
> > > @@ -293,10 +293,9 @@ static inline int folio_lru_gen(const struct folio *folio)
> > >         return lru_get_gen_flags(READ_ONCE(*const_folio_flags(folio, 0)));
> > >  }
> > >
> > > -static inline void lru_gen_update_size(struct lruvec *lruvec, struct folio *folio,
> > > -                                      int old_gen, int new_gen)
> > > +static inline void lru_gen_update_size(struct lruvec *lruvec, int type,
> > > +               struct folio *folio, int old_gen, int new_gen)
> > >  {
> > > -       int type = folio_is_file_lru(folio);
> > >         int zone = folio_zonenum(folio);
> > >         int delta = folio_nr_pages(folio);
> > >         struct lru_gen_folio *lrugen = &lruvec->lrugen;
> > > @@ -371,7 +370,7 @@ static inline bool lru_gen_add_folio(struct lruvec *lruvec, struct folio *folio,
> > >         /* use the refs from the atomic snapshot to avoid raced update */
> > >         refs = lru_get_refs_flags(flags);
> > >
> > > -       lru_gen_update_size(lruvec, folio, -1, gen);
> > > +       lru_gen_update_size(lruvec, type, folio, -1, gen);
> > >         if (lru_refs_is_active(refs))
> > >                 lru += LRU_ACTIVE;
> > >         __update_lru_size(lruvec, lru, zone, delta);
> > > @@ -389,6 +388,7 @@ static inline bool lru_gen_del_folio(struct lruvec *lruvec, struct folio *folio,
> > >  {
> > >         unsigned long flags;
> > >         int gen, refs;
> > > +       int type = folio_is_file_lru(folio);
> > >         int zone = folio_zonenum(folio);
> > >         int delta = folio_nr_pages(folio);
> > >         enum lru_list lru = folio_is_file_lru(folio) * LRU_INACTIVE_FILE;
> > > @@ -411,7 +411,7 @@ static inline bool lru_gen_del_folio(struct lruvec *lruvec, struct folio *folio,
> > >         if (!reclaiming && ((max_seq - gen) % MAX_NR_GENS) < MIN_NR_GENS)
> > >                 folio_set_active(folio);
> > >
> > > -       lru_gen_update_size(lruvec, folio, gen, -1);
> > > +       lru_gen_update_size(lruvec, type, folio, gen, -1);
> > >         if (lru_refs_is_active(refs))
> > >                 lru += LRU_ACTIVE;
> > >         __update_lru_size(lruvec, lru, zone, -delta);
> > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > index e096fd0a5047..d78e3f97423e 100644
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -558,9 +558,10 @@ enum lruvec_flags {
> > >  #define LRU_TIER_MIN           0U
> > >  #define LRU_TIER_MAX           (MAX_NR_TIERS - 1)
> > >
> > > -/* Access source flags for folio_inc_lru_refs() */
> > > +/* Flags for folio_inc_lru_refs() */
> > >  #define LRU_REF_MAPPED         0x1U
> > >  #define LRU_REF_EXEC           0x2U
> > > +#define LRU_REF_FORCE          0x4U
> > >
> > >  #define LRU_REFS_REFERENCED    0x1
> > >  #define LRU_REFS_WORKINGSET    0x2
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index ae4a0522a8d5..9cc06a95d931 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -982,38 +982,33 @@ void folio_inc_lru_refs(struct folio *folio, unsigned int flags)
> > >         long nr_pages = folio_nr_pages(folio);
> > >         struct lru_gen_folio *lrugen;
> > >         struct lruvec *lruvec = NULL;
> > > +       bool aged = false;
> > >
> > > +retry:
> > >         old_flags = READ_ONCE(*folio_flags(folio, 0));
> > >         do {
> > >                 new_flags = old_flags;
> > >                 old_gen = lru_get_gen_flags(old_flags);
> > >                 old_refs = lru_get_refs_flags(old_flags);
> > > -               file = folio_flags_is_file_lru(&old_flags);
> > >                 refs = old_refs + 1;
> > >                 gen = old_gen;
> > > +               file = folio_flags_is_file_lru(&old_flags);
> > >                 if (old_gen < 0)
> > >                         goto out;
> > > -               /*
> > > -                * Lock the lruvec if the folio is on-list. We are already
> > > -                * doing lazy promotion so in theory we don't need this,
> > > -                * but for now, concurrent aging would still corrupt the
> > > -                * size counters.  This is a temporary limitation and
> > > -                * will be lifted very soon, so the lock here is not a
> > > -                * performance concern.
> > > -                */
> > >                 if (!lruvec) {
> > > -                       lruvec = lruvec_live_lock_irq(folio_lruvec(folio));
> > > +                       lruvec = folio_lruvec_live_get(folio);
> > >                         lrugen = &lruvec->lrugen;
> > >                 }
> > > +               /* Failed cmpxchg() does not order old_flags before max_seq. */
> > > +               smp_rmb();
> > >                 max_seq = READ_ONCE(lrugen->max_seq);
> > >                 max_gen = lru_gen_from_seq(max_seq);
> > >                 min_gen = lru_gen_from_seq(READ_ONCE(lrugen->min_seq[file]));
> > >                 if (old_gen == max_gen)
> > >                         goto out;
> > > -
> > > -               if (flags & (LRU_REF_MAPPED | LRU_REF_EXEC)) {
> > > -                       /* Promote second page table access or executable */
> > > -                       if (refs > LRU_REFS_REFERENCED || flags & LRU_REF_EXEC)
> > > +               if (flags & (LRU_REF_MAPPED | LRU_REF_EXEC | LRU_REF_FORCE)) {
> > > +                       if (refs > LRU_REFS_REFERENCED ||
> > > +                           flags & (LRU_REF_EXEC | LRU_REF_FORCE))
> > >                                 gen = max_gen;
> > >                         /* First access only defers eviction from the oldest gen */
> > >                         else if (old_gen == min_gen)
> > > @@ -1037,9 +1032,19 @@ void folio_inc_lru_refs(struct folio *folio, unsigned int flags)
> > >                         break;
> > >         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
> > >
> > > -       if (gen != old_gen)
> > > -               lru_gen_update_size(lruvec, folio, old_gen, gen);
> > > -       if (lru_refs_is_active(old_refs) != lru_refs_is_active(refs) && old_gen >= 0) {
> > > +       if (gen != old_gen) {
> > > +               /*
> > > +                * Gen-index reuse can fool cmpxchg(). Retry locklessly, accepting
> > > +                * an extra reference to avoid folio_activate() latency.
> > > +                */
> > > +               if (unlikely(READ_ONCE(lrugen->max_seq) != max_seq)) {
> > > +                       flags = LRU_REF_FORCE;
> > > +                       aged = true;
> > > +               }
> > > +               lru_gen_update_size(lruvec, file, folio, old_gen, gen);
> > > +       }
> > > +
> > > +       if (lru_refs_is_active(old_refs) != lru_refs_is_active(refs) && gen >= 0) {
> > >                 enum lru_list lru = file * LRU_INACTIVE_FILE;
> > >
> >
> > Hi Kairui,
> >
> > One question about the retry path.
> >
> > When `max_seq` changes after a gen-changing cmpxchg, the retry uses
> > `LRU_REF_FORCE` and promotes the folio directly to `max_gen`. In the
> > normal first `LRU_REF_MAPPED` access path, the same folio is only moved
> > to the next generation when it is in the oldest generation.
> >
> > Is the stronger promotion on the retry intentional? In other words,
> > does detecting that `max_seq` advanced during the update justify
> > promoting the folio all the way to the newest generation, rather than
> > re-evaluating the original promotion policy with the new `max_seq`?
>
> I think we are fine, that window is very very rare already, and the
> problem is we may cause hotness inversion if the gen number lapped
> with each other. To making things easier, if the max_seq changed,
> just force promote it so we won't lose any hotness info. It may
> over protect but the chance is like once in a million so I think
> it's fine.

Makes sense — over-protecting once in a million is clearly the
safer side.
Thanks for the explanation. I'll look at v4 and follow up there.