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

[Kiryl Shutsemau]

On Wed, Sep 23, 2026 at 02:24:04PM +0200, David Hildenbrand (Arm) wrote:
> > +/* 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.

The comment undersells it.

The flag stands in for every sub-PMD rule khugepaged applies and
MADV_COLLAPSE does not: no swapped-out PTE, no shared PTE, and
max_ptes_none scaled to the order.

Before this patch all three helpers tested is_khugepaged and then
is_pmd_order; the flag replaced the first test in each. Which makes it
is_khugepaged under another name, so you are right that it is the wrong
abstraction.

Splitting the limits works. Two sets of counts, one per order class:

	struct collapse_limits {
		unsigned int max_ptes_none;
		unsigned int max_ptes_swap;
		unsigned int max_ptes_shared;
	};

	struct collapse_policy {
		struct collapse_limits pmd;
		struct collapse_limits sub_pmd;
		...
	};

For swap and shared the sub-PMD value is a count: 0 for khugepaged, no
limit for MADV_COLLAPSE.

For none it is the knob value, and the helper keeps today's rule: 511
means all but one PTE of the window, anything else means none.

	static unsigned int collapse_max_ptes_none(struct collapse_control *cc,
			struct vm_area_struct *vma, unsigned int order)
	{
		unsigned int max_ptes_none;

		if (vma && userfaultfd_armed(vma))
			return 0;
		if (is_pmd_order(order))
			return cc->policy.pmd.max_ptes_none;

		/* Below PMD order: all but one PTE of the window, or none */
		max_ptes_none = cc->policy.sub_pmd.max_ptes_none;
		if (max_ptes_none == COLLAPSE_MAX_PTES_LIMIT)
			return (1 << order) - 1;
		return 0;
	}

Only the warning for other knob values moves to where khugepaged fills
its policy. MADV_COLLAPSE never reads sub_pmd: it collapses to PMD order
only. Nothing is scaled, so the creep question is untouched.

> > +
> > +	/* 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.

install_pmd and writeback_dirty are read only on the file side,
skip_lazyfree and require_referenced only on the anonymous side.

I will prefix them.

> > +/* 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?

Will do.

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

Ack.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov