Re: [PATCH v3] mm/damon/ops-common: putback folios on invalid migrate nid

On Sun, 26 Jul 2026 02:00:34 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Invalid address and PTE advancement after page-to-folio conversion for large folios in DAMON scanning loops.
> - [High] DAMON migration causes NR_ISOLATED_ANON and NR_ISOLATED_FILE counter underflow.
> --
> 
> --- Patch [1]: [PATCH v3] mm/damon/ops-common: putback folios on invalid migrate nid ---
> commit 1f243815b710aba6f1b966b6f98b83cb7c51edeb
> Author: liyouhong <[email protected]>
> 
> mm/damon/ops-common: putback folios on invalid migrate nid
> 
> damon_pa_migrate() and damos_va_migrate() isolate folios into a local list
> and then call damon_migrate_pages().  When target_nid is invalid (including
> the scheme default NUMA_NO_NODE / -1), damon_migrate_pages() returns early
> without putting the folios back to the LRU.
> 
> Callers then discard the list head while those folios remain isolated with
> an extra reference taken by folio_isolate_lru().  The pages stay off the
> LRU for as long as the mapping exists (anon active+inactive counts drop
> while RSS does not), and the leftover references can pin the pages after
> the mapping is gone.
> 
> Put the folios back on the invalid-nid path so ignored migration requests
> still return them to the LRU.
> 
> Fixes: 7e6c3130690a ("mm/damon/ops-common: ignore migration request to invalid nodes")
> Cc: <[email protected]>
> Assisted-by: Cursor:grok-4.5
> Reviewed-by: SJ Park <[email protected]>
> Signed-off-by: liyouhong <[email protected]>
> 
> > diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> > index e59f77eca83b2..0981a5e3af172 100644
> > --- a/mm/damon/ops-common.c
> > +++ b/mm/damon/ops-common.c
> [ ... ]
> 
> While reviewing this migration fix, I noticed a couple of pre-existing issues
> in the DAMON migration code:
> 
> mm/damon/vaddr.c:damos_va_migrate_pmd_entry() {
> [ ... ]
> 	for (; addr < next; pte += nr, addr += nr * PAGE_SIZE) {
> 		nr = 1;
> 		ptent = ptep_get(pte);
> 
> 		if (pte_none(ptent) || !pte_present(ptent))
> 			continue;
> 		folio = vm_normal_folio(walk->vma, addr, ptent);
> 		if (!folio)
> 			continue;
> 		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
> 			continue;
> 		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
> 				migration_lists);
> 		nr = folio_nr_pages(folio);
> 	}
> [ ... ]
> }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this iteration overshoot
> the end of large folios?
> 
> If the current PTE corresponds to a tail page (meaning we are at an offset
> into the folio), adding the full folio_nr_pages() to both pte and addr seems
> like it will advance past the end of the folio, skipping subsequent, unrelated
> memory.
> 
> This same pattern appears in damos_va_stat_pmd_entry(), damon_pa_migrate(),
> and damon_pa_stat().

Known issue.  I'm working on it.

> 
> 
> mm/damon/vaddr.c:damos_va_migrate_dests_add() {
> [ ... ]
> isolate:
> 	if (!folio_isolate_lru(folio))
> 		return;
> 
> 	list_add(&folio->lru, &migration_lists[i]);
> }
> 
> [Severity: High]
> This is a pre-existing issue, but does this isolation miss incrementing the
> isolated VM stats?
> 
> Folios are isolated here and added to the migration list, but I don't see
> NR_ISOLATED_ANON or NR_ISOLATED_FILE being updated.
> 
> Later, when the core migrate_pages() code processes these folios via
> damon_migrate_pages():
> 
> mm/migrate.c:migrate_folio_done() {
> [ ... ]
> 	if (likely(!page_has_movable_ops(&src->page)) && reason != MR_DEMOTION)
> 		mod_node_page_state(folio_pgdat(src), NR_ISOLATED_ANON +
> 				    folio_is_file_lru(src), -folio_nr_pages(src));
> [ ... ]
> }
> 
> Will this decrement cause the node page state counters to underflow?
> 
> Since the counters weren't incremented originally during DAMON's isolation, it
> looks like a successful migration will drop the isolated stat and wrap it
> around. This could hang memory reclaim since too_many_isolated() might
> perpetually return true.
> 
> This also seems to apply to damon_pa_migrate().

Good catch.  Sounds correct.  No blocker for this patch.  I will work on this.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1


Thanks,
SJ