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

[Baolin Wang]

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>