Re: [PATCH v3 1/4] mm/mglru: add MM_WALK_EMPTY stats and tracepoint

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm/mglru: get_pte_pfn() and get_pmd_pfn() skip non-young
  entries in walk stats
- [Medium] mm/mglru: walk_pte_range() undercounts MM_LEAF_ASSOCIATED on
  large folios
- [Low] mm/mglru: mm_vmscan_lru_gen_walk tracepoint defined outside
  CONFIG_LRU_GEN
--

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

mm/mglru: add MM_WALK_EMPTY stats and tracepoint

This commit adds per-walk statistics and a tracepoint to track MGLRU aging
page-table walks that find no folios belonging to the target lruvec.

> diff --git a/mm/trace_vmscan.h b/mm/trace_vmscan.h
> index 9dafdb5bd3ce8..2b268fd0f89e8 100644
> --- a/mm/trace_vmscan.h
> +++ b/mm/trace_vmscan.h
> @@ -763,6 +763,34 @@ TRACE_EVENT(mm_vmscan_kswapd_clear_hopeless,
>  		__entry->nid,
>  		__print_symbolic(__entry->reason, kswapd_clear_hopeless_reason_ops))
>  );
> +TRACE_EVENT(mm_vmscan_lru_gen_walk,
> +
> +	TP_PROTO(int nid, unsigned long seq, int leaf_total,
> +		 int leaf_associated, bool empty),

[Severity: Low]
Should mm_vmscan_lru_gen_walk be placed inside the #ifdef CONFIG_LRU_GEN
block alongside mm_mglru_scan_folios and mm_mglru_aging?  Its only caller in
try_to_inc_max_seq() is built under CONFIG_LRU_GEN, so defining it here
generates unused tracepoint code and tracefs entries when CONFIG_LRU_GEN is
disabled.

[ ... ]

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 25a224746c36d..6034bae03dc21 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3582,6 +3582,8 @@ static bool walk_pte_range(pmd_t *pmd, unsigned long start, unsigned long end,
>  		if (!folio)
>  			continue;
>  
> +		walk->mm_stats[MM_LEAF_ASSOCIATED]++;
> +
>  		if (folio_test_large(folio)) {
>  			const unsigned int max_nr = (end - addr) >> PAGE_SHIFT;

[Severity: Medium]
Does walk_pte_range() undercount MM_LEAF_ASSOCIATED when a large folio spans
a batch of contiguous PTEs?

When folio_test_large(folio) is true, folio_pte_batch_flags() sets nr to the
number of batched PTEs and adds nr - 1 to walk->mm_stats[MM_LEAF_TOTAL] and
nr to walk->mm_stats[MM_LEAF_YOUNG], before the loop advances by nr entries:

mm/vmscan.c:walk_pte_range() {
    ...
		if (folio_test_large(folio)) {
			const unsigned int max_nr = (end - addr) >> PAGE_SHIFT;

			nr = folio_pte_batch_flags(folio, NULL, cur_pte, &ptent,
						   max_nr, FPB_MERGE_YOUNG_DIRTY);
			total += nr - 1;
			walk->mm_stats[MM_LEAF_TOTAL] += nr - 1;
		}
    ...
		young += nr;
		walk->mm_stats[MM_LEAF_YOUNG] += nr;
	}

Because MM_LEAF_ASSOCIATED is only incremented by 1 before the batch size is
computed, the remaining nr - 1 PTEs in the batch are not counted, and
MM_LEAF_YOUNG can end up larger than MM_LEAF_ASSOCIATED.

[ ... ]

> @@ -4158,8 +4162,28 @@ static bool try_to_inc_max_seq(struct lruvec *lruvec, unsigned long seq,
>  
>  	do {
>  		success = iterate_mm_list(walk, &mm);
> -		if (mm)
> +		if (mm) {
> +			bool empty = false;
> +
>  			walk_mm(mm, walk);
> +			/*
> +			 * A walk that traversed page tables but found no folio
> +			 * belonging to this lruvec (node+memcg) is pure waste.
> +			 */
> +			if (walk->mm_stats[MM_LEAF_TOTAL]) {
> +				walk->mm_stats[MM_WALK_TOTAL]++;
> +				if (walk->mm_stats[MM_LEAF_ASSOCIATED] == 0) {
> +					walk->mm_stats[MM_WALK_EMPTY]++;
> +					walk->mm_stats[MM_LEAF_EMPTY_WALKS] +=
> +						walk->mm_stats[MM_LEAF_TOTAL];
> +					empty = true;
> +				}
> +			}
> +			trace_mm_vmscan_lru_gen_walk(
> +					lruvec_pgdat(lruvec)->node_id, walk->seq,
> +					walk->mm_stats[MM_LEAF_TOTAL],
> +					walk->mm_stats[MM_LEAF_ASSOCIATED], empty);
> +		}
>  	} while (mm);

[Severity: Medium]
Does this count a walk as empty whenever an mm has folios in this lruvec
whose PTEs and PMDs are all non-young?

In walk_pte_range() and walk_pmd_range_locked(), MM_LEAF_ASSOCIATED is
incremented only after get_pte_pfn() or get_pmd_pfn() succeeds, but both
helpers reject non-young entries up front when the mm has no MMU notifiers:

mm/vmscan.c:get_pte_pfn() {
    ...
	if (!pte_young(pte) && !mm_has_notifiers(vma->vm_mm))
		return -1;

As a result, non-young leaf entries mapping folios in this lruvec are never
counted in MM_LEAF_ASSOCIATED, and any walk over an mm whose mapped folios in
this lruvec are all cold has MM_LEAF_ASSOCIATED == 0 and is recorded in
MM_WALK_EMPTY, MM_LEAF_EMPTY_WALKS, and trace_mm_vmscan_lru_gen_walk() with
empty=1.

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