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

["Zi Yan" <ziy@nvidia.com>]

On Mon Sep 7, 2026 at 6:49 AM EDT, Kiryl Shutsemau wrote:
> On Fri, Sep 04, 2026 at 10:44:58PM -0400, Zi Yan wrote:
>> On Fri Sep 4, 2026 at 11:10 AM EDT, Kiryl Shutsemau wrote:
>> > From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
>> >
>> > 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@kernel.org>
>> > ---
>> >  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;
>> > +
>> > +	/*
>> > +	 * 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.
>> > +	 */
>> > +	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;
>> > +};
>> > +
>> >  struct collapse_control {
>> > -	bool is_khugepaged;
>> > +	struct collapse_policy policy;
>> 
>> Can it be made const since it seems to be read-only?
>
> Not as a member: khugepaged fills it once per pass from the sysfs knobs,
> so there is no constant object to point at. And madvise has to
> initialize it somehow after cc is allocated.
>
> It could become a const pointer to a struct the caller owns, but that
> adds a lifetime to track for a struct that is written in one place and
> read everywhere else.  I would rather keep it embedded.

Got it. Thanks for the explanation.

-- 
Best Regards,
Yan, Zi