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

[SJ Park <sj@kernel.org>]

On Sun, 01 Nov 2026 17:27:03 +0000 sashiko-bot@kernel.org wrote:

> 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?

Good catch.  I will setup nr and page_idx before calling
damos_va_invalid_folio().

> 
> >  		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?

Makes sense.  I will do so in the next version.

> 
> [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?

That's a good room to improve.  Not a blocker of this patch in my opinion,
though.

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


Thanks,
SJ