[PATCH] mm/mglru: fix using folio_mark_accessed to replace folio_set_active

[PATCH] mm/mglru: fix using folio_mark_accessed to replace folio_set_active

[Barry Song (Xiaomi)]

Commit 6cbdd9726fb5 ("mm/mglru: use folio_mark_accessed to
replace folio_set_active") attempted to avoid over-protecting
in-fault read-ahead file folios by placing them in the
second-oldest generation rather than the young generations.

However, it repurposes `PG_referenced` on in-fault read-ahead
file folios for this purpose, potentially coupling their
handling with that of regular file accesses through syscalls,
which also use `PG_referenced`.

This could regress some workloads, as reported by Kairui Song[1]
and Ehab Ababneh[2]. Many thanks to both for reporting these
issues!

The long-term fix is Kairui's MGLRU-FG, but in the meantime,
we also need a quick fix that can be backported to stable.

This patch reverts the original commit's abuse of `PG_referenced`
while preserving its goal of avoiding over-protection of
in-fault read-ahead file folios by adjusting only
`lru_gen_folio_seq()`.

Specifically, in-fault read-ahead file folios with `PG_active`
set but `PG_workingset` clear are placed in the second-oldest
generation (the third-youngest generation). Activated file
folios and refaulted workingset file folios remain in the
youngest generations.

Kernel build tests in a memcg show that this approach not only
preserves the original patch's benefits but also further
improves performance.

w/o fix:

real    1m49.094s
user    25m31.484s
sys     3m40.959s
pswpin: 662112
pswpout: 2504415
pgpgin: 4424336
pgpgout: 10355273
refault_file: 359626
refault_anon: 670724

w/ fix:

real    1m48.913s
user    25m31.639s
sys     3m30.875s
pswpin: 689815
pswpout: 2689894
pgpgin: 4422659
pgpgout: 11043464
refault_file: 326566
refault_anon: 697138

Revert original patch:

real    1m50.044s
user    25m37.137s
sys     4m4.493s
pswpin: 1349087
pswpout: 4317933
pgpgin: 7083597
pgpgout: 17602735
refault_file: 337676
refault_anon: 1368542

[1] https://lore.kernel.org/linux-mm/20261003-mglru-fg-v3-0-cbd4546a5bd9@tencent.com/
[2] https://lore.kernel.org/linux-mm/20260901180704.168106-1-ehab.ababneh@intel.com/
Fixes: 6cbdd9726fb5 ("mm/mglru: use folio_mark_accessed to replace folio_set_active")
Cc: <stable@vger.kernel.org>
Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>
---
 * An earlier discussion can be found here:
 https://lore.kernel.org/linux-mm/20261016084256.12955-1-baohua@kernel.org/
 Thanks for Kairui's comment on it.

 include/linux/mm_inline.h |  5 +++--
 mm/folio.c                | 16 +++-------------
 mm/vmscan.c               |  9 ++-------
 mm/workingset.c           | 10 ++++------
 4 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index ab69b9930893..97f157aafd58 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -319,7 +319,8 @@ static inline unsigned long lru_gen_folio_seq(const struct lruvec *lruvec,
 	 * |<---------------------------- MAX_NR_GENS ---------------------------->|
 	 */
 	if (folio_test_active(folio))
-		gen = MIN_NR_GENS - folio_test_workingset(folio);
+		gen = MIN_NR_GENS - folio_test_workingset(folio) +
+		      (type ? !folio_test_workingset(folio) : 0);
 	else if (reclaiming)
 		gen = MAX_NR_GENS;
 	else if ((!folio_is_file_lru(folio) && !folio_test_swapcache(folio)) ||
@@ -327,7 +328,7 @@ static inline unsigned long lru_gen_folio_seq(const struct lruvec *lruvec,
 		  (folio_test_dirty(folio) || folio_test_writeback(folio))))
 		gen = MIN_NR_GENS;
 	else
-		gen = MAX_NR_GENS - (folio_test_workingset(folio) || folio_test_referenced(folio));
+		gen = MAX_NR_GENS - folio_test_workingset(folio);
 
 	return max(READ_ONCE(lrugen->max_seq) - gen + 1, READ_ONCE(lrugen->min_seq[type]));
 }
diff --git a/mm/folio.c b/mm/folio.c
index 35e242b48870..2af724cecb31 100644
--- a/mm/folio.c
+++ b/mm/folio.c
@@ -474,20 +474,10 @@ void folio_add_lru(struct folio *folio)
 			folio_test_unevictable(folio), folio);
 	VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
 
-	/*
-	 * For refaulted workingset folios, set PG_active so they
-	 * can be added to active generations.
-	 * For prefaulted file folios, folio_mark_accessed() sets
-	 * PG_referenced so lru_gen_folio_seq() places them into
-	 * the second oldest generation.
-	 */
+	/* see the comment in lru_gen_folio_seq() */
 	if (lru_gen_enabled() && !folio_test_unevictable(folio) &&
-	    lru_gen_in_fault() && !(current->flags & PF_MEMALLOC)) {
-		if (folio_test_workingset(folio))
-			folio_set_active(folio);
-		else if (!folio_test_referenced(folio))
-			folio_mark_accessed(folio);
-	}
+	    lru_gen_in_fault() && !(current->flags & PF_MEMALLOC))
+		folio_set_active(folio);
 
 	folio_batch_add_and_move(folio, lru_add);
 }
diff --git a/mm/vmscan.c b/mm/vmscan.c
index daa7e37c2236..e82ba737c028 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -952,13 +952,8 @@ static bool lru_gen_set_refs(struct folio *folio, const vma_flags_t *vma_flags)
 		return false;
 	}
 
-	/* Promote on second access */
-	if (folio_lru_refs(folio) > 1) {
-		folio_set_workingset(folio);
-		folio_set_lru_refs(folio, 0);
-	} else {
-		folio_mark_accessed(folio);
-	}
+	folio_set_workingset(folio);
+	folio_set_lru_refs(folio, 0);
 	return true;
 }
 #else
diff --git a/mm/workingset.c b/mm/workingset.c
index 1504f91cdca5..10d4ce202cca 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -319,13 +319,11 @@ static void lru_gen_refault(struct folio *folio, void *shadow)
 
 	atomic_long_add(delta, &lrugen->refaulted[hist][type][tier]);
 
+	/* see folio_add_lru() where folio_set_active() will be called */
+	if (lru_gen_in_fault())
+		mod_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + type, delta);
+
 	if (workingset) {
-		/*
-		 * see folio_add_lru(), where folio_set_active() is
-		 * called for workingset folios
-		 */
-		if (lru_gen_in_fault())
-			mod_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + type, delta);
 		folio_set_workingset(folio);
 		mod_lruvec_state(lruvec, WORKINGSET_RESTORE_BASE + type, delta);
 	} else
-- 
2.34.1

Re: [PATCH] mm/mglru: fix using folio_mark_accessed to replace folio_set_active

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/mglru: lru_gen_folio_seq() demotes non-workingset active file folios
- [Medium] mm/mglru: lru_gen_refault() miscounts WORKINGSET_ACTIVATE_FILE
--

commit fa50b3cc1b1d64cb457afbbf37dc545c6d4e06f1
Author: Barry Song (Xiaomi) <baohua@kernel.org>

mm/mglru: fix using folio_mark_accessed to replace folio_set_active

This commit reverts the use of PG_referenced on in-fault readahead file
folios from commit 6cbdd9726fb5 ("mm/mglru: use folio_mark_accessed to
replace folio_set_active"), and instead adjusts lru_gen_folio_seq() to place
file folios with PG_active set and PG_workingset clear into the
third-youngest generation.

> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> index ab69b9930893f..97f157aafd581 100644
> --- a/include/linux/mm_inline.h
> +++ b/include/linux/mm_inline.h
> @@ -319,7 +319,8 @@ static inline unsigned long lru_gen_folio_seq(const struct lruvec *lruvec,
>  	 * |<---------------------------- MAX_NR_GENS ---------------------------->|
>  	 */
>  	if (folio_test_active(folio))
> -		gen = MIN_NR_GENS - folio_test_workingset(folio);
> +		gen = MIN_NR_GENS - folio_test_workingset(folio) +
> +		      (type ? !folio_test_workingset(folio) : 0);

[Severity: High]
Does assigning gen = 3 (lrugen->max_seq - 2) for all file folios (type == 1)
that have PG_active set and PG_workingset clear break other MGLRU paths that
set PG_active expecting the folio to land in an active generation?

First, in mm/vmscan.c, evict_folios() promotes folios rejected by
shrink_folio_list() out of min_seq[type] by clearing LRU_REFS_FLAGS and
setting PG_active:

mm/vmscan.c:evict_folios() {
	...
		folio_set_lru_refs(folio, 0);
		if (lru_gen_folio_seq(lruvec, folio, false) == min_seq[type])
			folio_set_active(folio);
	}

	move_folios_to_lru(&list, false);
}

When 3 generations exist (lrugen->max_seq - lrugen->min_seq[1] == 2),
max(lrugen->max_seq - 2, lrugen->min_seq[1]) equals min_seq[1]. A rejected
!PG_workingset file folio (for example, a mapped file folio spared via
FOLIOREF_KEEP on its first rmap scan in lru_gen_set_refs(), a DMA-pinned
folio, or a folio failing try_to_unmap()) has its PTE accessed bit and
PG_referenced cleared, and move_folios_to_lru() -> lru_gen_add_folio() puts
it right back into min_seq[1] instead of promoting it. Even when 4
generations exist, evict_folios() clears PG_referenced and puts the folio in
lrugen->max_seq - 2, so the next rmap scan in lru_gen_set_refs() sees
!folio_test_referenced(folio) && !folio_test_workingset(folio) again and can
never promote it to PG_workingset.

Second, in mm/vmscan.c, shrink_folio_list() sets PG_reclaim on dirty or
writeback file folios and jumps to activate_locked, which sets PG_active:

mm/vmscan.c:shrink_folio_list() {
	...
		if (folio_test_dirty(folio)) {
			if (folio_is_file_lru(folio)) {
				node_stat_mod_folio(folio, NR_VMSCAN_IMMEDIATE,
						nr_pages);
				if (!folio_test_reclaim(folio))
					folio_set_reclaim(folio);

				goto activate_locked;
			}
	...
activate_locked:
	...
		folio_set_active(folio);
}

Because lru_gen_folio_seq() checks folio_test_active(folio) before the
PG_reclaim && (folio_test_dirty(folio) || folio_test_writeback(folio))
branch (which assigns gen = MIN_NR_GENS = 2), won't setting PG_active on a
!PG_workingset dirty or writeback file folio downgrade it from gen = 2
(active) to gen = 3 (inactive, or min_seq[1] when 3 generations exist)?

Third, when a !PG_workingset file folio is explicitly activated via
folio_activate() -> lru_activate(), or when it is isolated from the active
generation lrugen->max_seq - 1 for migration/compaction via
lru_gen_del_folio() (which sets BIT(PG_active) when !reclaiming &&
lru_gen_is_active(lruvec, gen)), re-adding it via lru_gen_add_folio() now
places it into lrugen->max_seq - 2 (LRU_INACTIVE_FILE). Doesn't this demote
activated and migrated active file folios into an inactive generation?

>  	else if (reclaiming)
>  		gen = MAX_NR_GENS;
>  	else if ((!folio_is_file_lru(folio) && !folio_test_swapcache(folio)) ||
> @@ -327,7 +328,7 @@ static inline unsigned long lru_gen_folio_seq(const struct lruvec *lruvec,
>  		  (folio_test_dirty(folio) || folio_test_writeback(folio))))
>  		gen = MIN_NR_GENS;
>  	else
> -		gen = MAX_NR_GENS - (folio_test_workingset(folio) || folio_test_referenced(folio));
> +		gen = MAX_NR_GENS - folio_test_workingset(folio);
>  
>  	return max(READ_ONCE(lrugen->max_seq) - gen + 1, READ_ONCE(lrugen->min_seq[type]));
>  }

[ ... ]

> diff --git a/mm/workingset.c b/mm/workingset.c
> index 1504f91cdca5f..10d4ce202cca4 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -319,13 +319,11 @@ static void lru_gen_refault(struct folio *folio, void *shadow)
>  
>  	atomic_long_add(delta, &lrugen->refaulted[hist][type][tier]);
>  
> +	/* see folio_add_lru() where folio_set_active() will be called */
> +	if (lru_gen_in_fault())
> +		mod_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + type, delta);
> +
>  	if (workingset) {
> -		/*
> -		 * see folio_add_lru(), where folio_set_active() is
> -		 * called for workingset folios
> -		 */
> -		if (lru_gen_in_fault())
> -			mod_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + type, delta);
>  		folio_set_workingset(folio);
>  		mod_lruvec_state(lruvec, WORKINGSET_RESTORE_BASE + type, delta);
>  	} else

[Severity: Medium]
When a file folio (type == 1) refaults in a page fault with workingset ==
false, should WORKINGSET_ACTIVATE_FILE still be incremented here?

With workingset == false, PG_workingset is not set. Although folio_add_lru()
calls folio_set_active(folio), lru_gen_add_folio() clears PG_active and
lru_gen_folio_seq() now maps a file folio with PG_active and !PG_workingset
to gen = 3 (lrugen->max_seq - 2).

In include/linux/mm_inline.h, lru_gen_update_size() only accounts
lrugen->max_seq and lrugen->max_seq - 1 as active:

include/linux/mm_inline.h:lru_gen_update_size() {
	...
	/* addition */
	if (old_gen < 0) {
		if (lru_gen_is_active(lruvec, new_gen))
			lru += LRU_ACTIVE;
		__update_lru_size(lruvec, lru, zone, delta);
		return;
	}
	...
}

When 3 or more generations exist, the folio is added to LRU_INACTIVE_FILE,
so moving the mod_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + type,
delta) call outside if (workingset) counts an activation for a file folio
that is placed in an inactive generation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016213115.70515-1-baohua@kernel.org?part=1