Re: [PATCH v3 3/4] mm/mglru: skip cold PUD subtrees during aging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm/mglru: walk_pmd_range() marks PUD young on non-leaf PMD
  accessed bit
- [Medium] mm/mglru: walk_pmd_range() marks PUD young for THP before
  memcg and notifier checks
- [Medium] mm/mglru: PUDs with PMD-mapped THPs are suppressed
  indefinitely once skipped
--

commit 31572808d36982038b4b945e98794001c99a5d21
Author: Baoquan He <hebaoquan@kylinos.cn>

mm/mglru: skip cold PUD subtrees during aging

This commit introduces a PUD-level Bloom filter keyed by (mm, PUD-aligned
address) to skip PUD subtrees that had no young pages in the previous
generation during MGLRU aging walks, and updates it from the rmap feedback
path in lru_gen_look_around().

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index a0332586b3d83..79d533ebfd108 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c

[ ... ]

> @@ -3730,10 +3732,11 @@ static void walk_pmd_range_locked(pud_t *pud, unsigned long addr, struct vm_area
>  	*first = -1;
>  }
>  
> -static void walk_pmd_range(pud_t *pud, unsigned long start, unsigned long end,
> +static bool walk_pmd_range(pud_t *pud, unsigned long start, unsigned long end,
>  			   struct mm_walk *args)
>  {
>  	int i;
> +	bool young = false;
>  	pmd_t *pmd;
>  	unsigned long next;
>  	unsigned long addr;
> @@ -3770,8 +3773,10 @@ static void walk_pmd_range(pud_t *pud, unsigned long start, unsigned long end,
>  
>  			walk->mm_stats[MM_LEAF_TOTAL]++;
>  
> -			if (pfn != -1)
> +			if (pfn != -1) {
>  				walk_pmd_range_locked(pud, addr, vma, args, bitmap, &first);
> +				young = true;
> +			}
>  			continue;
>  		}

[Severity: Medium]
Does setting young = true as soon as get_pmd_pfn() != -1 mark the PUD young
before checking memcg ownership or MMU notifiers?

get_pmd_pfn() only checks that the PFN is on pgdat and that pmd_young() or
mm_has_notifiers() is true, while the memcg check in get_pfn_folio() and
the notifier young check in pmdp_test_and_clear_young_notify() are deferred
to walk_pmd_range_locked().  A PUD with PMD-mapped THPs belonging to
another memcg on the same node, or cold THPs in an mm with MMU notifiers,
will therefore be recorded in BLOOM_FILTER_PUD on every generation and
never skipped.

> @@ -3781,6 +3786,7 @@ static void walk_pmd_range(pud_t *pud, unsigned long start, unsigned long end,
>  				continue;
>  
>  			walk_pmd_range_locked(pud, addr, vma, args, bitmap, &first);
> +			young = true;
>  		}
>  
>  		if (!walk->force_scan &&
> @@ -3793,6 +3799,7 @@ static void walk_pmd_range(pud_t *pud, unsigned long start, unsigned long end,
>  			continue;
>  
>  		walk->mm_stats[MM_NONLEAF_ADDED]++;
> +		young = true;
>  
>  		/* carry over to the next generation */
>  		update_bloom_filter(mm_state, BLOOM_FILTER_PMD, walk->seq + 1, pmd + i);

[Severity: Medium]
Should young = true be omitted from the non-leaf pmd_young(val) block?

When should_clear_pmd_young() is true, setting young = true there happens
before checking BLOOM_FILTER_PMD or calling walk_pte_range().  Because CPU
page table walks set the accessed bit on non-leaf PMDs regardless of which
node or memcg owns the underlying PTEs, any accessed non-leaf PMD will
cause walk_pmd_range() to return true and carry the PUD into
BLOOM_FILTER_PUD even when every PMD in the subtree fails the PMD Bloom
filter or has no folios in walk->lruvec.  The young = true after
walk_pte_range() already handles non-leaf PMDs that pass suitable_to_scan().

> @@ -3802,6 +3809,8 @@ static void walk_pmd_range(pud_t *pud, unsigned long start, unsigned long end,
>  
>  	if (i < PTRS_PER_PMD && get_next_vma(PUD_MASK, PMD_SIZE, args, &start, &end))
>  		goto restart;
> +
> +	return young;
>  }
>  
>  static int walk_pud_range(p4d_t *p4d, unsigned long start, unsigned long end,
> @@ -3812,6 +3821,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long start, unsigned long end,
>  	unsigned long addr;
>  	unsigned long next;
>  	struct lru_gen_mm_walk *walk = args->private;
> +	struct lru_gen_mm_state *mm_state = get_mm_state(walk->lruvec);
>  
>  	VM_WARN_ON_ONCE(p4d_leaf(*p4d));
>  
> @@ -3819,13 +3829,29 @@ static int walk_pud_range(p4d_t *p4d, unsigned long start, unsigned long end,
>  restart:
>  	for (i = pud_index(start), addr = start; addr != end; i++, addr = next) {
>  		pud_t val = pudp_get(pud + i);
> +		void *pud_key;
>  
>  		next = pud_addr_end(addr, end);
>  
>  		if (!pud_present(val) || WARN_ON_ONCE(pud_leaf(val)))
>  			continue;
>  
> -		walk_pmd_range(pud + i, addr, next, args);
> +		/*
> +		 * Use (mm, PUD-aligned address) as the filter key instead of
> +		 * the page table pointer. It is stable across the aging walk
> +		 * and the rmap feedback path, and avoids walking the page
> +		 * tables in lru_gen_look_around().
> +		 */
> +		pud_key = (void *)((unsigned long)args->mm ^ (addr & PUD_MASK));
> +
> +		/* Skip a subtree that had nothing young for this lruvec last gen */
> +		if (!walk->force_scan &&
> +		    !test_bloom_filter(mm_state, BLOOM_FILTER_PUD, walk->seq, pud_key))
> +			continue;
> +
> +		if (walk_pmd_range(pud + i, addr, next, args))
> +			update_bloom_filter(mm_state, BLOOM_FILTER_PUD, walk->seq + 1,
> +					    pud_key);

[Severity: Medium]
Once a PUD is absent from BLOOM_FILTER_PUD, can PMD-mapped THPs inside it
ever be scanned by page-table aging again without force_scan?

Previously, BLOOM_FILTER_PMD was checked after pmd_trans_huge() in
walk_pmd_range(), so PMD-mapped THPs were never gated by a Bloom filter.
Gating walk_pmd_range() on BLOOM_FILTER_PUD skips the entire PUD when
pud_key is not present, and iterate_mm_list() clears the next generation's
filter via reset_bloom_filter(mm_state, BLOOM_FILTER_PUD, walk->seq + 1).
The only rmap feedback that populates BLOOM_FILTER_PUD is
lru_gen_look_around(), which folio_referenced_one() only calls when
pvmw.pte is non-NULL:

mm/rmap.c:folio_referenced_one() {
    ...
		if (lru_gen_enabled() && !lru_gen_switching() && pvmw.pte &&
		    pte_present(pteval)) {
			if (lru_gen_look_around(&pvmw, nr))
				referenced++;
		} else if (pvmw.pte) {
    ...
		} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
			if (pmdp_clear_flush_young_notify(vma, address,
						pvmw.pmd))
				referenced++;
    ...
}

For PMD-mapped THPs (pvmw.pte == NULL), rmap eviction checks never update
BLOOM_FILTER_PUD, so any PUD that goes cold for one generation or receives
newly faulted or collapsed THPs after the initial force_scan will be
skipped by aging indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113084953.412919-1-baoquan.he@linux.dev?part=3