Re: [RFC PATCH v2 2/6] mm/damon/vaddr: use damos->last_applied for migrate action

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mm/damon/vaddr: last_applied not updated when damos_va_filter_out() rejects
- [Medium] mm/damon/vaddr: stale folio in last_applied after damos_va_migrate()
- [Low] mm/damon/vaddr: single-page PTE step in damos_va_migrate_pmd_entry()
--

commit af5ba00f84f8ca12cd15e44abbd02f8e8e38b95b
Author: SJ Park <sj@kernel.org>

mm/damon/vaddr: use damos->last_applied for migrate action

This patch updates damos_va_migrate_pmd_entry() to check and record
s->last_applied via damos_va_invalid_folio() so that DAMOS_MIGRATE_HOT and
DAMOS_MIGRATE_COLD actions are not applied multiple times to folios spanning
adjacent virtual address regions.

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index c271b6d63c027..b1d7bcf7305bb 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -834,6 +834,12 @@ static void damos_va_migrate_dests_add(struct folio *folio,
>  	list_add(&folio->lru, &migration_lists[i]);
>  }
>  
> +static inline bool damos_va_invalid_folio(struct folio *folio,
> +		struct damos *s)
> +{
> +	return !folio || folio == s->last_applied;
> +}
> +
>  static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		unsigned long next, struct mm_walk *walk)
>  {
> @@ -854,10 +860,11 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		if (!pmd_present(pmde))
>  			goto huge_out;
>  		folio = vm_normal_folio_pmd(walk->vma, addr, pmde);
> -		if (!folio)
> +		if (damos_va_invalid_folio(folio, s))
>  			goto huge_out;
>  		if (damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd))
>  			goto huge_out;
> +		s->last_applied = folio;
>  		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
>  				migration_lists);
>  huge_out:
> @@ -879,12 +886,13 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		if (pte_none(ptent) || !pte_present(ptent))
>  			continue;
>  		folio = vm_normal_folio(walk->vma, addr, ptent);
> -		if (!folio)
> +		if (damos_va_invalid_folio(folio, s))
>  			continue;

[Severity: Low]
In damos_va_migrate_pmd_entry(), when damos_va_invalid_folio(folio, s) returns
true because folio == s->last_applied for a PTE-mapped large folio at the
start of the next region, the loop continues before computing page_idx and nr:

	nr = 1;
	ptent = ptep_get(pte);

	if (pte_none(ptent) || !pte_present(ptent))
		continue;
	folio = vm_normal_folio(walk->vma, addr, ptent);
	if (damos_va_invalid_folio(folio, s))
		continue;
	page_idx = folio_page_idx(folio, pte_page(ptent));
	nr = folio_nr_pages(folio) - page_idx;

Doesn't this leave nr at 1 and step through all remaining tail PTEs of that
large folio one page at a time instead of advancing by folio_nr_pages(folio) -
page_idx?

>  		page_idx = folio_page_idx(folio, pte_page(ptent));
>  		nr = folio_nr_pages(folio) - page_idx;
>  		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
>  			continue;
> +		s->last_applied = folio;
>  		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
>  				migration_lists);
>  	}

[Severity: Medium]
Should s->last_applied be updated even when damos_va_filter_out() rejects the
folio in both the PMD and PTE paths of damos_va_migrate_pmd_entry(), matching
damos_va_stat_pmd_entry() and damon_pa_migrate()?

When a PMD-mapped THP or PTE-mapped large folio spans two adjacent regions and
is rejected by damos_va_filter_out() in the first region, jumping to huge_out
or continuing before setting s->last_applied = folio leaves s->last_applied
unchanged. When the second region is walked, damos_va_filter_out() is called
a second time on the same folio.

If an ops filter such as DAMOS_FILTER_TYPE_YOUNG is used to exclude young
folios (for example with DAMOS_MIGRATE_COLD), evaluating
damos_va_filter_young_match() in the first region clears the page table young
bit and marks the folio idle as a side effect:

mm/damon/vaddr.c:damos_va_filter_young_match() {
	...
	if (young && ptep)
		damon_ptep_mkold(ptep, vma, addr);
	else if (young && pmdp)
		damon_pmdp_mkold(pmdp, vma, addr);
	...
}

Wouldn't re-running damos_va_filter_out() on the same folio in the adjacent
region then observe the folio as old, pass the filter, and migrate the young
folio that was just rejected in the first region?

[Severity: Medium]
Also, how does s->last_applied deduplicate a folio across adjacent regions
once damos_va_migrate() has migrated it in the first region?

In damos_va_migrate(), damon_migrate_pages() runs at the end of each region
before the next region is walked:

mm/damon/vaddr.c:damos_va_migrate() {
	...
	damon_va_walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
	mmput(mm);

	for (int i = 0; i < nr_dests; i++) {
		nid = use_target_nid ? s->target_nid : dests->node_id_arr[i];
		applied += damon_migrate_pages(&priv.migration_lists[i], nid);
		cond_resched();
	}
	...
}

When a large folio spanning two adjacent regions is migrated during the first
region, migrate_pages() allocates a new destination folio, remaps all PTEs or
PMDs across the VMA (including those in the second region) to the new folio,
and frees the source folio.

When damos_va_migrate_pmd_entry() then walks the second region,
vm_normal_folio() or vm_normal_folio_pmd() returns the newly allocated
destination folio while s->last_applied still holds the old source folio
pointer. Since the pointers do not match, won't damos_va_invalid_folio()
return false in the second region, causing damos_va_filter_out() and
damos_va_migrate_dests_add() to be evaluated again on the already-migrated
folio?

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