Re: [PATCH 05/12] mm/collapse: state what a collapse may do in the policy

[Baolin Wang]

On 9/4/26 11:10 PM, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
>
> Tests scattered through the collapse path decide what a collapse is
> allowed to do by asking whether khugepaged started it.  Between them they
> settle:
>
>   - which VMAs are eligible, and how hard to try for a folio;
>   - how many empty, swapped-out or shared PTEs a window may contain, and
>     whether a sub-PMD window is held to a stricter rule than a PMD;
>   - whether a range has to look used, and whether a MADV_FREE'd page is
>     left alone;
>   - whether the PMD is mapped as part of the request, and whether dirty
>     pages are worth writing back and retrying.
>
> None of those is a fact about khugepaged.  Each is something the caller
> decided before asking, and the collapse code should not have to look up
> who called to find out.
>
> Add struct collapse_policy for the caller to fill: khugepaged from its
> own settings, MADV_COLLAPSE from the fact that a user asked explicitly.
> Every test becomes a read of a field, and cc->is_khugepaged goes, having
> no reader left.
>
> khugepaged fills the policy once per scan pass, MADV_COLLAPSE once per
> call.  That is the one change in behaviour.  The max_ptes_* limits and the
> defrag setting behind the allocation mask are sampled once per pass rather
> than on every table.  A table scanned early in a pass and one scanned late
> are then judged alike.
>
> collapse_file() also drops a NULL check on the collapse_control.  It has
> one call site, reached only from collapse_single_pmd(), which dereferences
> cc unconditionally, so the check was already dead.
>
> Assisted-by: Claude-Code:claude-opus-5
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> ---
>   mm/collapse.h   |  40 ++++++++++++++++-
>   mm/khugepaged.c | 114 ++++++++++++++++++++++++++----------------------
>   2 files changed, 102 insertions(+), 52 deletions(-)
>
> diff --git a/mm/collapse.h b/mm/collapse.h
> index 1c40229b9554..05282eed9a35 100644
> --- a/mm/collapse.h
> +++ b/mm/collapse.h
> @@ -48,8 +48,46 @@ enum scan_result {
>   	SCAN_PAGE_DIRTY_OR_WRITEBACK,
>   };
>   
> +/* What a collapse is allowed to do, decided by the caller that asks for it */
> +struct collapse_policy {
> +	/* Limits, stated per PMD; HPAGE_PMD_NR means "no limit" */
> +	unsigned int max_ptes_none;
> +	unsigned int max_ptes_swap;
> +	unsigned int max_ptes_shared;
> +
> +	/*
> +	 * Hold a sub-PMD window to a stricter rule than a PMD: no swapped-out
> +	 * and no shared PTEs at all, and max_ptes_none as
> +	 * collapse_max_ptes_none() scales it.
> +	 */
> +	bool strict_sub_pmd;

This is a bit confusing to me. Actually, the check for mTHP collapse is 
stricter.

How about naming it 'allow_mthp_collapse'? That way we can keep the most 
original comments for the collapse_max_ptes_xxx() functions, which is 
clearer to me.

If others have a better name, please ignore my comment.

> +	/*
> +	 * Collapse only where it looks worth doing: require some sign the
> +	 * range is in use, and leave clean lazyfree folios for reclaim rather
> +	 * than collapsing them into a folio that is not lazyfree.
> +	 */
> +	bool skip_lazyfree;
> +	bool require_referenced;
> +
> +	/*
> +	 * Finish the job rather than leaving it half done for a fault to pick
> +	 * up: map the PMD over a file collapse before returning, and write
> +	 * dirty pages back and retry once instead of refusing them.  Both cost
> +	 * latency the caller has to be willing to pay.
> +	 */

Can you simplify theses comments? I think the 'install_pmd' is easy to 
expalain. :)

> +	bool install_pmd;
> +	bool writeback_dirty;
> +
> +	/* How hard to try for a destination folio */
> +	gfp_t gfp;
> +
> +	/* Which VMAs are eligible, as thp_vma_allowable_orders() spells it */
> +	enum tva_type tva_type;
> +};