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

[Kairui Song]

Sashiko reported this (TLDR: mostly a false positive, but I will send
a V2 to make things a bit cleaner):

--- Patch [1]: [PATCH] mm/mglru: fix and remove redundant unevictable
folio handling ---
commit 4d1ddce0f76b4c4700b6f963424566b2fce82fd9
Author: Kairui Song <kasong@xxxxxxxxxxx>

mm/mglru: fix and remove redundant unevictable folio handling

This commit removes the !folio_evictable(folio) shortcut in sort_folio()
to rely on the generic LRU culling path instead. The goal is to ensure
unevictable folios are handled by isolate_folio() and shrink_folio_list()
with correctly initialized mlock counts.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 3194da7dcc795..eca5ff64238dd 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4660,16 +4659,6 @@ static bool sort_folio(struct lruvec *lruvec, struct folio *folio, struct scan_c
>
>   VM_WARN_ON_ONCE_FOLIO(gen >= MAX_NR_GENS, folio);
>
> - /* unevictable */
> - if (!folio_evictable(folio)) {
> - success = lru_gen_del_folio(lruvec, folio, true);
> - VM_WARN_ON_ONCE_FOLIO(!success, folio);
> - folio_set_unevictable(folio);
> - lruvec_add_folio(lruvec, folio);
> - __count_vm_events(UNEVICTABLE_PGCULLED, delta);
> - return true;
> - }
> -
>   /* promoted */
>   if (gen != lru_gen_from_seq(lrugen->min_seq[type])) {
>   list_move(&folio->lru, &lrugen->folios[gen][type][zone]);

With the early !folio_evictable() check removed, how does this path handle
actively accessed unevictable folios?

If a folio is added to MGLRU, then the mapping is later marked unevictable
(for example, via shmctl passing SHM_LOCK), and the folio is actively
accessed by userspace, it will accumulate elevated MGLRU references.

When sort_folio() runs, wouldn't it hit this promoted branch (or the
subsequent protected branch) and return true?

sort_folio() {
...
/* promoted */
if (gen != lru_gen_from_seq(lrugen->min_seq[type])) {
list_move(&folio->lru, &lrugen->folios[gen][type][zone]);
return true;
}

/* protected */
if (tier > tier_idx || refs + workingset == BIT(LRU_REFS_WIDTH) + 1) {
gen = folio_inc_gen(lruvec, folio);
...
return true;
}
...
}

If sort_folio() returns true here, the folio is skipped for isolation.
It never reaches isolate_folio() or shrink_folio_list(), which means the
deferred !folio_evictable() check is never evaluated.

Could this cause actively accessed unevictable folios to circulate
indefinitely in the multi-gen LRU lists, artificially inflating the
active generations, instead of being culled once and for all?


========

The answer is we are fine, or at least I don't think that worth a
standalone optimization.

We have two kinds of unevictable folios here:

1. mlock(): mlock_folio() sets PG_mlocked immediately but defers the
list move to the per-CPU mlock folio batch (31/CPU), drained on
batch-full, by every lru_add_drain*(). These folios sit on a gen list
only briefly and are then moved off properly regardless of hotness.

2. SHM_LOCK / unevictable mapping: these do stay on the lists until
scanned. When their generation becomes the oldest, if they're not
worth protecting they're isolated and culled via the generic path. If
they are hot, they may be protected for a few extra passes, but this
protection decays, so it's not an indefinite circulation, and
correctness doesn't depend on when the cull happens.

But to avoid any potential issue or concern, I'll send an V2, let
sort_folio just return false for unevictable folios. I think this
change barely has any visible effect though, just to make it more
consistent with classical LRU which also unconditionally isolates
them.