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 4 potential issue(s) to consider:
- [High] damos_va_collapse() accepts mTHP target orders but calls damon_collapse_folio_range() which strictly rejects them.
- [High] Calling damon_collapse_folio_range() in a per-chunk loop triggers severe global lock contention and IPI storms via lru_add_drain_all().
- [High] damos_va_split() skips entire PMD-sized chunks when encountering unmapped pages or small folios, preventing valid mTHPs from being split.
- [Medium] damos_va_collapse() fails to deduplicate overlapping chunk collapses across adjacent DAMON regions, artificially inflating statistics.
--

commit bbcfac46aaf7ca5b18e45240fde04cb5fffba9c1
Author: Lian Wang <lianux.mm@gmail.com>
Subject: mm/damon/vaddr: implement mTHP-aware DAMOS_COLLAPSE handler

This commit implements the DAMOS_COLLAPSE action for mTHP by iterating
over target regions in order-aligned chunks. It expands region boundaries
to the covering aligned range and calls damon_collapse_folio_range()
without holding an external mmap lock to avoid self-deadlocks.

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> --- 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;

[Severity: Medium]
Does this code fail to deduplicate overlapping chunk collapses across
adjacent DAMON regions?

When kdamond splits regions into smaller sizes that fall into the same
aligned chunk, damos_va_collapse() is called for each region. Because
last_chunk is initialized locally to ULONG_MAX on every call, it does
not persist across calls, which artificially inflates the DAMON
statistics (sz_filter_passed and applied) and prematurely exhausts
quotas.

Additionally, because addr is strictly incremented by chunk_sz within
the loop, the addr == last_chunk check appears to be dead code:

	while (addr < end) {
		...
		if (addr == last_chunk)
			goto next;
		last_chunk = addr;
		...
		addr += chunk_sz;
	}

> +	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)
> +		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;
> +
> +		ret = damon_collapse_folio_range(mm, addr, target_order);

[Severity: High]
Does calling damon_collapse_folio_range() with mTHP orders fail silently?

While this loop accepts target_order > 0 and < HPAGE_PMD_ORDER,
damon_collapse_folio_range() explicitly rejects any order that is not
PMD_ORDER:

mm/khugepaged.c:damon_collapse_folio_range() {
	if (target_order != HPAGE_PMD_ORDER) {
		pr_warn_once("... only PMD order ... is supported ...");
		return -EINVAL;
	}
}

This means attempts to collapse regions into mTHP sizes will fail, while
still incrementing sz_filter_passed.

[Severity: High]
Can this tight loop cause severe global lock contention and IPI storms?

damos_va_collapse() iterates over a target memory region in chunk_sz
increments, calling damon_collapse_folio_range() sequentially for every
chunk. However, the wrapper damon_collapse_folio_range() unconditionally
executes lru_add_drain_all():

mm/khugepaged.c:damon_collapse_folio_range() {
	...
	lru_add_drain_all();
	...
}

For a multi-GB VMA region, this places a global synchronization point
(acquiring lru_add_drain_mutex and sending IPIs to all online CPUs)
inside a tight loop hundreds or thousands of times, which can cause
system-wide performance degradation, latency spikes, and potential RCU
lockups.

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

[ ... ]

I also noticed an issue in the related damos_va_split() function:

[Severity: High]
Does damos_va_split() skip entire PMD-sized chunks when encountering
unmapped pages or small folios?

In damos_va_split(), if an unmapped page is encountered, the code jumps
to the unlock label:

	folio = folio_walk_start(&fw, vma, addr, 0);
	if (!folio)
		goto unlock;

And at the unlock label, addr is advanced by chunk_sz (typically 2MB)
instead of PAGE_SIZE:

unlock:
	*sz_filter_passed += chunk_sz;
	addr += chunk_sz;

A similar advancement occurs in the else block when a small folio is
encountered:

	} else {
		folio_walk_end(&fw, vma);
		*sz_filter_passed += chunk_sz;
		addr += chunk_sz;
	}

By advancing addr by 2MB upon encountering a single unmapped or small
page, this prevents valid mTHPs in the remainder of the chunk from being
split.

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