Re: [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:
- [High] DAMOS_COLLAPSE silently drops support for file-backed memory when target_order is explicitly set.
- [High] damos_va_collapse() spins uselessly for mTHP orders because the backend function explicitly rejects them.
- [High] Expanding sub-regions to chunk boundaries without deduplication causes massive redundant processing and quota inflation.
- [High] Stack exhaustion risk due to allocating a large `struct collapse_control` on the kernel stack.
- [Medium] ALIGN() boundary calculation can overflow to 0 for high memory regions, causing them to be silently skipped.
- [Low] Missing stub for damon_collapse_folio_range() causes build failures when CONFIG_TRANSPARENT_HUGEPAGE is disabled.
--

commit 54fd1422ea9f2a5e501241c8565e081399639772
Author: Wang Lian <lianux.mm@gmail.com>

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

This commit updates the DAMOS_COLLAPSE handler to support collapsing memory
into requested mTHP sizes. It iterates over target regions in chunk-aligned
ranges and falls back to the existing MADV_COLLAPSE path if no target order
is provided.

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index e73ec1ce016e5..eeaa403eb79e4 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
[ ... ]
> @@ -897,6 +898,40 @@ 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 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);

[Severity: Medium]
Does this calculation overflow to 0 if the region ends near the top of the
address space?

If the address is close enough to ULONG_MAX, ALIGN() wrapping around to 0
would cause the subsequent while loop condition to immediately evaluate to
false, silently skipping those memory regions.

> +
> +	mm = damon_get_mm(target);
> +	if (!mm)
> +		return 0;
> +
> +	while (addr < end) {
> +		ret = damon_collapse_folio_range(mm, addr, target_order);

[Severity: Low]
Does this cause a build failure when CONFIG_TRANSPARENT_HUGEPAGE is disabled?

The declaration for damon_collapse_folio_range() in
include/linux/khugepaged.h appears to lack an empty stub for the disabled
case, which would break the unconditional call here.

[Severity: High]
Is there a risk of stack exhaustion in the backend?

Looking at damon_collapse_folio_range() in mm/khugepaged.c, it allocates
struct collapse_control directly on the stack:

mm/khugepaged.c:damon_collapse_folio_range() {
        struct collapse_control cc = {
                .is_khugepaged = false,
        };
        ...
}

Because this struct contains bitmaps sized by MAX_NUMNODES, it can easily
exceed safe kernel stack limits on typical configurations.

[Severity: High]
Can this loop waste CPU by repeatedly attempting operations that will always
fail for mTHP orders?

While this function allows target orders from 2 up to HPAGE_PMD_ORDER,
damon_collapse_folio_range() in mm/khugepaged.c appears to explicitly reject
anything other than HPAGE_PMD_ORDER:

mm/khugepaged.c:damon_collapse_folio_range() {
        if (target_order != HPAGE_PMD_ORDER) {
                pr_warn_once("... only PMD order (%u) is supported, got %u\n",
                             __func__, HPAGE_PMD_ORDER, target_order);
                return -EINVAL;
        }
        ...
}

This would cause immediate -EINVAL failures for valid mTHP target orders.

> +		if (!ret)
> +			applied += chunk_sz;
> +		*sz_filter_passed += chunk_sz;
> +		addr += chunk_sz;

[Severity: High]
Can multiple small DAMON sub-regions that fall within the same chunk cause
redundant processing and inflate the quota?

Since the boundaries are expanded earlier using ALIGN_DOWN() and ALIGN()
without deduplicating adjacent sub-regions, the same chunk might be
collapsed and accounted for multiple times.

> +		cond_resched();
> +	}
> +
> +	mmput(mm);
> +	return applied;
> +}
> +
>  static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  		struct damon_target *t, struct damon_region *r,
>  		struct damos *scheme, unsigned long *sz_filter_passed)
> @@ -920,6 +955,9 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  		madv_action = MADV_NOHUGEPAGE;
>  		break;
>  	case DAMOS_COLLAPSE:
> +		if (scheme->target_order)
> +			return damos_va_collapse(t, r, scheme,
> +						 sz_filter_passed);

[Severity: High]
Does setting the target order silently break support for file-backed memory?

The original MADV_COLLAPSE path supports file-backed regions, but
damon_collapse_folio_range() delegates to collapse_huge_page() in
mm/khugepaged.c, which unconditionally enforces expect_anon=true.

>  		madv_action = MADV_COLLAPSE;
>  		break;
>  	case DAMOS_MIGRATE_HOT:

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