Re: [RESEND RFC PATCH v2 3/5] mm/damon/vaddr: implement mTHP-aware DAMOS_COLLAPSE handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] The `collapse_control` struct `cc` is allocated without zeroing, causing uninitialized memory usage.
- [Critical] Calling `lru_add_drain_all()` and allocating huge pages per-chunk in a tight loop without VMA validation causes system-wide IPI storms and severe CPU stalls.
- [High] damon_collapse_folio_range explicitly rejects non-PMD orders, breaking the advertised mTHP support.
- [High] DAMOS_COLLAPSE via `damos_va_collapse` silently fails on all file-backed VMAs.
- [Medium] Local `last_chunk` variable fails to deduplicate cross-region chunks, artificially inflating DAMON statistics.
- [Low] Build break when CONFIG_TRANSPARENT_HUGEPAGE is disabled due to unconditional use of HPAGE_PMD_ORDER.
--

commit 1758a6da0e4869c672603d9b92add29e2cab16d6
Author: Lian Wang <lianux.mm@gmail.com>

mm/damon/vaddr: implement mTHP-aware DAMOS_COLLAPSE handler

The commit implements an mTHP-aware DAMOS_COLLAPSE handler that iterates over
the target region in order-aligned chunks when a target mTHP order is provided.
It utilizes damon_collapse_folio_range() for the underlying memory collapse
operations while falling back to the madvise path for default configurations.

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 2058db9c01d52..8bfc6dcc635d5 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -895,6 +896,50 @@ static unsigned long damos_va_stat(struct damon_target *target,
>  	return 0;
>  }
>  
> +static unsigned long damos_va_collapse(struct damon_target *target,
> +		struct damon_region *r, struct damos *s,
> +		unsigned long *sz_filter_passed)
> +{
> +	unsigned long addr, end, chunk_sz;
> +	unsigned long last_chunk = ULONG_MAX;
> +	unsigned int target_order = s->target_order;
> +	unsigned long applied = 0;
> +	struct mm_struct *mm;
> +	int ret;
> +
> +	if (target_order < 2 || target_order > HPAGE_PMD_ORDER)

[Severity: Low]
Is this safe to use when CONFIG_TRANSPARENT_HUGEPAGE is disabled?

In damos_va_collapse(), HPAGE_PMD_ORDER is unconditionally referenced. Without
THP, HPAGE_PMD_ORDER evaluates to an expression containing BUILD_BUG(), which
can cause a build breakage.

> +		return 0;
> +
> +	chunk_sz = PAGE_SIZE << target_order;
> +	addr = ALIGN_DOWN(r->ar.start, chunk_sz);
> +	end = ALIGN(r->ar.end, chunk_sz);
> +	if (end < addr)
> +		return 0;
> +
> +	mm = damon_get_mm(target);
> +	if (!mm)
> +		return 0;
> +
> +	while (addr < end) {
> +		if (addr + chunk_sz < addr)
> +			break;
> +		if (addr == last_chunk)
> +			goto next;
> +		last_chunk = addr;

[Severity: Medium]
Will this reliably deduplicate chunks across multiple DAMON regions?

Because last_chunk is local to damos_va_collapse() and resets for each region,
adjacent or overlapping regions that map to the same chunk might bypass this
check. This can result in the same range being processed multiple times,
artificially inflating the sz_filter_passed and applied statistics.

> +
> +		ret = damon_collapse_folio_range(mm, addr, target_order);

[Severity: High]
Will this function correctly handle mTHP orders smaller than PMD?

While damos_va_collapse() allows target_order values down to 2,
damon_collapse_folio_range() in mm/khugepaged.c strictly rejects non-PMD
orders:

	if (target_order != HPAGE_PMD_ORDER) {
		...
		return -EINVAL;
	}

Doesn't this unconditionally reject smaller mTHP orders, causing the collapse
to fail while still incorrectly incrementing the sz_filter_passed statistic?

[Severity: Critical]
Can this code cause uninitialized memory usage during the collapse?

When damon_collapse_folio_range() allocates the collapse_control struct:

	cc = kmalloc_obj(*cc);
	if (!cc)
		return -ENOMEM;
	cc->is_khugepaged = false;
	cc->progress = 0;

The cc->node_load array and cc->alloc_nmask are left uninitialized because
kmalloc_obj() does not zero memory. Since this bypasses collapse_scan_pmd()
which normally initializes these fields, collapse_find_target_node() will
later read garbage data to make NUMA node placement decisions.

[Severity: Critical]
Does this code handle unmapped virtual address ranges efficiently?

Iterating over purely virtual address ranges without validating if they belong
to a VMA can be problematic. For each chunk, damon_collapse_folio_range()
in mm/khugepaged.c executes heavy operations like lru_add_drain_all() and
attempts huge page allocations.

If a large unmapped memory hole is encountered, could this lead to severe
CPU stalls and system-wide IPI storms before the VMA validation eventually
fails?

[Severity: High]
Does this silently fail on file-backed VMAs?

Looking at damon_collapse_folio_range() in mm/khugepaged.c, it directly wraps
collapse_huge_page() which uses hugepage_vma_revalidate() with expect_anon
set to true. This unconditionally rejects file-backed VMAs without falling
back to collapse_file(), potentially breaking the collapse functionality for
file-backed memory.

> +		if (!ret)
> +			applied += chunk_sz;
> +		*sz_filter_passed += chunk_sz;
> +next:
> +		addr += chunk_sz;
> +		cond_resched();
> +	}
> +
> +	mmput(mm);
> +	return applied;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702095227.75866-1-lianux.mm@gmail.com?part=3