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

[sashiko-bot@kernel.org]

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