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 4 potential issue(s) to consider:
- [High] `damon_collapse_folio_range()` passes an uninitialized `struct collapse_control` to `collapse_huge_page()`, leaving fields like `node_load` and `alloc_nmask` with heap garbage, potentially corrupting NUMA node targeting during THP allocation.
- [High] Bypassing the read-only page table validation scan forces destructive PMD operations on un-collapsible memory ranges, creating a severe performance degradation vector.
- [Medium] `damon_collapse_folio_range()` silently fails to collapse file-backed memory ranges because it routes directly to the anonymous-only `collapse_huge_page()` function.
- [Medium] `damon_collapse_folio_range()` hardcodes the `unmapped` parameter to 0, which prevents the collapse operation from succeeding if there are any swapped-out pages in the PMD range.
--

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

mm/khugepaged: add damon_collapse_folio_range() for external callers

This commit introduces damon_collapse_folio_range(), providing a wrapper
around khugepaged's collapse_huge_page() for external subsystems like DAMON.
It allows targeted THP collapse on specific address ranges but is currently
limited to PMD order.

> 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 collapse_control struct uninitialized?

When collapse_huge_page() eventually calls alloc_charge_folio(), it uses
collapse_find_target_node(). This function iterates over cc->node_load and
reads/writes cc->alloc_nmask:

collapse_find_target_node()
   ...
   node_set(nid, cc->alloc_nmask);

Since kmalloc_obj() does not zero memory, wouldn't this pass heap garbage
to the folio allocator and corrupt NUMA node targeting?

> +	if (!cc)
> +		return -ENOMEM;
> +	cc->is_khugepaged = false;
> +	cc->progress = 0;
> +
> +	lru_add_drain_all();
> +
> +	result = collapse_huge_page(mm, start_addr, 1, 0, cc, target_order);

[Severity: Medium]
Does this silently fail for file-backed memory regions?

By calling collapse_huge_page() directly instead of the dispatcher
collapse_single_pmd(), the flow reaches hugepage_vma_revalidate() with
expect_anon hardcoded to true:

hugepage_vma_revalidate()
    if (expect_anon && !vma_is_anonymous(vma))
        return SCAN_PAGE_ANON;

Wouldn't this unconditionally reject shmem/tmpfs mappings targeted by DAMON?

[Severity: Medium]
By passing 0 for the unmapped parameter here, does this prevent the collapse
operation if any pages are swapped out?

Because unmapped is 0, collapse_huge_page() skips
__collapse_huge_page_swapin(). Then, when __collapse_huge_page_isolate()
scans the PTEs, it will encounter swapped pages and fail:

__collapse_huge_page_isolate()
    if (!pte_present(pteval)) {
        result = SCAN_PTE_NON_PRESENT;
        goto out;
    }

Could we allow swap-in by passing 1 here instead?

[Severity: High]
Does bypassing the read-only scan phase cause severe performance issues?

Normally, collapse_scan_pmd() performs a read-only validation of the page
tables. By skipping straight to collapse_huge_page(), this code takes the
mmap_write_lock, destructively clears the PMD, and triggers a global TLB
flush before verifying if the PTEs can actually be isolated:

collapse_huge_page()
    _pmd = pmdp_collapse_flush(vma, pmd_addr, pmd);
    ...
    tlb_remove_table_sync_one();

If isolation fails (e.g., due to pinned or unmapped pages), it restores the
PMD and aborts. Because DAMON repeatedly scans and targets memory, wouldn't
this create continuous lock contention and TLB flush storms on
un-collapsible memory ranges?

> +	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/20260702095227.75866-1-lianux.mm@gmail.com?part=2