Re: [RESEND RFC PATCH v2 2/5] mm/khugepaged: add damon_collapse_folio_range() for external callers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uninitialized memory in `struct collapse_control` causes undefined behavior and random NUMA node constraints during THP allocation.
- [High] Calling `lru_add_drain_all()` for every PMD creates severe system-wide latency spikes and IPI storms.
- [High] Directly calling collapse_huge_page() bypasses PTE pre-scanning and swap-in, causing unconditional THP allocations and breaking file-backed THP support.
--

commit 03af0641474bb6262efed77c042c17e3c94d4f27
Author: Lian Wang <lianux.mm@gmail.com>

mm/khugepaged: add damon_collapse_folio_range() for external callers

Adds a thin wrapper around collapse_huge_page() to allow external
subsystems like DAMON to trigger THP collapse on a target address range.

> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 617bca76db49b..7fe9ce1e05335 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c

[ ... ]

> @@ -3272,3 +3272,49 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>  	return thps == ((hend - hstart) >> HPAGE_PMD_SHIFT) ? 0
>  			: madvise_collapse_errno(last_fail);
>  }
> +
> +/**
> + * damon_collapse_folio_range() - Collapse base pages in range into a THP
> + * @mm:         mm_struct of the target process
> + * @start_addr: start address (must be order-aligned)
> + * @target_order: page order of the collapse result (currently only
> + *                HPAGE_PMD_ORDER is supported)
> + *
> + * Thin wrapper around collapse_huge_page() for external callers such as
> + * DAMON.  The caller must hold a reference to @mm.  Do not hold mmap
> + * lock: collapse_huge_page() acquires mmap_read_lock for validation,
> + * releases it, then acquires mmap_write_lock for the collapse.  Holding
> + * an outer mmap_read_lock would self-deadlock.
> + *
> + * Return: 0 on success, -EINVAL on bad arguments, negative error from
> + *         madvise_collapse_errno() otherwise.
> + */
> +int damon_collapse_folio_range(struct mm_struct *mm, unsigned long start_addr,
> +			       unsigned int target_order)
> +{
> +	struct collapse_control *cc;
> +	enum scan_result result;
> +
> +	if (target_order != HPAGE_PMD_ORDER) {
> +		pr_warn_once("%s: only PMD order (%u) is supported, got %u\n",
> +			     __func__, HPAGE_PMD_ORDER, target_order);
> +		return -EINVAL;
> +	}
> +	if (start_addr & ((PAGE_SIZE << target_order) - 1))
> +		return -EINVAL;
> +
> +	cc = kmalloc_obj(*cc);

[Severity: High]
Does this leave the cc->node_load array and cc->alloc_nmask nodemask
uninitialized?

Since this code bypasses collapse_scan_pmd() which typically zeros these
fields, collapse_huge_page() will call alloc_charge_folio() and then
collapse_find_target_node().

In collapse_find_target_node(), the code reads cc->node_load[nid] to find
the maximum value and sets bits in cc->alloc_nmask. Passing a garbage
nodemask to __folio_alloc() could cause undefined behavior and random NUMA
node constraints during allocation.

> +	if (!cc)
> +		return -ENOMEM;
> +	cc->is_khugepaged = false;
> +	cc->progress = 0;
> +
> +	lru_add_drain_all();

[Severity: High]
Can this cause system-wide latency spikes and IPI storms?

Because damon_collapse_folio_range() is called for every 2MB chunk by DAMON
iteration logic, unconditionally invoking the heavy cross-CPU
lru_add_drain_all() here means it will execute for every single PMD in the
region.

Unlike madvise_collapse() which hoists this drain out of its scanning loop,
placing it inside the per-PMD API might severely impact system performance.

> +
> +	result = collapse_huge_page(mm, start_addr, 1, 0, cc, target_order);

[Severity: High]
Does this direct call bypass PTE pre-scanning and swap-in logic?

By skipping collapse_scan_pmd() and directly invoking collapse_huge_page(),
this forces an unconditional allocation via alloc_charge_folio() for every
chunk evaluated, before even checking if the PMD is collapsible.

Also, passing 0 for the unmapped parameter skips the swap-in step, meaning
any swapped-out pages in the target region will cause the isolation phase
to abort.

Furthermore, hugepage_vma_revalidate() inside collapse_huge_page()
hardcodes expect_anon=true, so this path will silently fail for any
file-backed memory.

> +	kfree(cc);
> +	if (result == SCAN_SUCCEED || result == SCAN_PMD_MAPPED)
> +		return 0;
> +	return madvise_collapse_errno(result);
> +}
> +EXPORT_SYMBOL_GPL(damon_collapse_folio_range);

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