Re: [PATCH] mm/mglru: fix and remove redundant unevictable folio handling

[Baolin Wang]

On 8/12/26 8:28 PM, Kairui Song wrote:
> On Wed, Aug 12, 2026 at 6:06 PM Baolin Wang
> <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
> > On 8/11/26 5:40 PM, Kairui Song via B4 Relay wrote:
> > > From: Kairui Song <kasong@xxxxxxxxxxx>
> > >
> > > sort_folio() has a shortcut for moving folios that are no longer
> > > evictable but are still sitting on a generation list.  However, this
> > > shortcut is buggy.  It does not follow the PG_lru usage convention,
> > > and it has a more serious issue.
> > >
> > > Unevictable folios are not threaded on lists[LRU_UNEVICTABLE], so that
> > > folio->lru can be reused to hold folio->mlock_count (see the comment in
> > > lruvec_init()).  Hence lruvec_add_folio() skips the list_add() for them,
> > > and every other place that turns a folio unevictable initialises
> > > mlock_count explicitly: lru_add() sets it to 0, __mlock_folio() and
> > > __mlock_new_folio() set it to !!folio_test_mlocked(folio).
> > > sort_folio() sets nothing, and the lru_gen_del_folio() right above it
> > > may have already poisoned folio->lru via list_del(), so mlock_count
> > > ends up aliasing LIST_POISON2, which reads as 0x122, i.e. 290.  The
> > > result is user visible.  On munlock, __munlock_folio() decrements that
> > > bogus count, finds it still non-zero and bails out before clearing
> > > PG_mlocked, so the folio remains unevictable and the Mlocked
> > > accounting stays inflated until the folio is freed.
> > >
> > > The shortcut also touches the LRU flags in the wrong order.  It calls
> > > lru_gen_del_folio() while PG_lru is still set, so a concurrent
> > > folio_test_clear_lru() (e.g. compaction, folio_isolate_lru()) can succeed
> > > on a folio that has already been taken off the generation list, may
> > > lead to unexpected behavior.  The generic path gets this right:
> > > isolate_folio() clears PG_lru first, so a racing isolator loses the
> > > atomic and bails.
> > >
> > > And the shortcut is redundant.  A folio left on the generation list is
> > > picked up by isolate_folio(), shrink_folio_list() sends it to
> > > activate_locked on the !folio_evictable() check, and evict_folios()
> > > then hands it to folio_putback_lru(), which sets PG_unevictable and
> > > counts UNEVICTABLE_PGCULLED from lru_add(), with mlock_count
> > > initialised properly.
> > >
> > > There is no performance concern either: such a folio goes through this
> > > once, and then it is off the generation lists for good, since
> > > lru_gen_add_folio() refuses unevictable folios.
> > >
> > > So just remove the shortcut.  This consolidates unevictable handling in
> > > the generic path, and makes maintenance easier.
> > >
> > > Fixes: ac35a4902374 ("mm: multi-gen LRU: minimal implementation")
> > > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > > ---
> >
> > Good catch. Make sense to me.
> > Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>
> Hi Baolin
>
> Thank you very mcuh for the review!
>
> I just sent a V2 here:
> https://lore.kernel.org/linux-mm/20260812-mglru-mlock-fix-v2-1-a3fec5853c08@xxxxxxxxxxx/T/#u
>
> This is a bit different, idea is still the same but I changed the
> code, since the patch is very small so it's basically a rewrite, hence
> I didn't include this review by. Can you help have a look at v2 as
> well?

Sure. Thanks for the fix.