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

[David Hildenbrand (Arm)]

On 9/16/26 11:31, 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.

Agreed. Removing plenty of these is_khugepaged checks is nice.

> 
> 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 treated alike.

Fair.

> 
> 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.

Worth putting this into a separate patch? Likely not.

> 
> Assisted-by: LLM
> Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> ---
>  mm/collapse.h   |  31 ++++++++++++-
>  mm/khugepaged.c | 114 ++++++++++++++++++++++++++----------------------
>  2 files changed, 93 insertions(+), 52 deletions(-)
> 
> diff --git a/mm/collapse.h b/mm/collapse.h
> index b115034d9018..7044dc71c7c2 100644
> --- a/mm/collapse.h
> +++ b/mm/collapse.h
> @@ -45,8 +45,37 @@ 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;
> +
> +	/* Take no swapped-out or shared PTE into a sub-PMD collapse */
> +	bool strict_sub_pmd;

Reading this variable name without the documentation I have no idea what it
means. It looks like the wrong abstraction.

Maybe you instead want to split max_ptes_swap and shared to a PMD and non-PMD case?

I am also confused why you use "strict_sub_pmd" in the collapse_max_ptes_none()
handler below? Something seems odd, as it doesn't amtch the description here.

> +
> +	/* Leave clean lazyfree folios to reclaim rather than collapse them */
> +	bool skip_lazyfree;
> +
> +	/* Refuse a range with no sign of use */
> +	bool require_referenced;
> +
> +	/* Map the PMD over a file collapse instead of leaving it to a fault */
> +	bool install_pmd;

Confusing.

If some of these policies are anon-/ file-specific, the name should indicate
that, so there is less head scratching.

> +
> +	/* Write dirty pages back and retry once instead of refusing them */
> +	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;
> +};
> +
>  struct collapse_control {
> -	bool is_khugepaged;
> +	struct collapse_policy policy;
>  
>  	/* Num pages scanned per node */
>  	u32 node_load[MAX_NUMNODES];
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 8889f75cf45f..cb08789b2d38 100644
> --- a/mm/khugepaged.c


[...]

>  
> +/* khugepaged collapses on its own initiative, so it obeys its own settings */
> +static void collapse_policy_khugepaged(struct collapse_policy *p)
> +{
> +	p->max_ptes_none = READ_ONCE(khugepaged_max_ptes_none);
> +	p->max_ptes_swap = READ_ONCE(khugepaged_max_ptes_swap);
> +	p->max_ptes_shared = READ_ONCE(khugepaged_max_ptes_shared);
> +	p->strict_sub_pmd = true;
> +	p->skip_lazyfree = true;
> +	p->require_referenced = true;
> +	p->install_pmd = false;
> +	p->writeback_dirty = false;
> +	p->gfp = alloc_hugepage_khugepaged_gfpmask();
> +	p->tva_type = TVA_KHUGEPAGED;
> +}
> +
> +/* MADV_COLLAPSE was asked for explicitly, so it is not held to those */
> +static void collapse_policy_forced(struct collapse_policy *p)

Why are we not calling this collapse_policy_madvise to match its description here?


[...]

> @@ -2943,6 +2952,9 @@ static void khugepaged_do_scan(struct collapse_control *cc)
>  
>  	lru_add_drain_all();
>  
> +	/* One policy for the whole pass, so every table is treated the same */

just drop that comment.



-- 
Cheers,

David