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

On 8/12/26 8:28 PM, Kairui Song wrote:
> On Wed, Aug 12, 2026 at 6:06 PM Baolin Wang
> <[email protected]> wrote:
>>
>> On 8/11/26 5:40 PM, Kairui Song via B4 Relay wrote:
>>> From: Kairui Song <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>
>> Good catch. Make sense to me.
>> Reviewed-by: Baolin Wang <[email protected]>
> 
> Hi Baolin
> 
> Thank you very mcuh for the review!
> 
> I just sent a V2 here:
> https://lore.kernel.org/linux-mm/[email protected]/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.