Re: [PATCH v3 11/12] mm/collapse: declare the collapse interface in collapse.h

[Kiryl Shutsemau]

On Wed, Sep 23, 2026 at 03:34:11PM +0200, David Hildenbrand (Arm) wrote:
> On 9/16/26 11:31, Kiryl Shutsemau wrote:
> > From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
> > 
> > A collapse takes four calls:
> > 
> >   - collapse_control_init() - set up the control a caller carries;
> >   - collapse_scan_pmd() - scan one PTE table, under mmap_lock;
> >   - collapse_run_pmd() - collapse what the scan found, no mmap_lock;
> 
> As discussed, having a single collapse_pmd() function might be cleaner if that's
> easily possible.

Answered on patch 9: the two calls are the point.

> >   - collapse_control_release() - done with the control.
> 
> And as discussed, I hope we can just get rid of a release function that's not
> actually supposed to release anything right now (unless I was missing an update
> in one of the patches).

Will remove in v4.

> > All four are static in khugepaged.c, as are collapse_possible_orders(),
> > which says what a VMA allows, and the revalidate a caller needs once a
> > collapse has given the mmap_lock up.  No other file can ask for a collapse
> > without them.
> > 
> > Declare them in collapse.h, with a comment stating the order they are
> > called in and who holds the lock over each step.  Each function says
> > what it needs and what it does where it is defined.
> > 
> > hugepage_vma_revalidate() becomes collapse_vma_revalidate(): it is part of
> > what a collapse offers now, not a helper of the daemon.
> 
> Is it just me or is collapse_vma_revalidate() an odd part of this interface?
> 
> You'd expect a matching function that performs the initial validation on a given
> vma.
> 
> Maybe we should have a
> 
> 	orders = collapse_vma_validate(vma)
> 
> that really just wraps collapse_possible_orders(), an expose that instead to the
> collapse users?
> 
> So they'd use collapse_vma_validate() to then call collapse_vma_revalidate()
> after temporarily dropping the mmap lock?

collapse_vma_revalidate() re-checks what the scan ran under: a VMA at
the address, anonymous if the scan went that way, covering the PMD
range, allowing this order.

A collapse_vma_validate() could bundle what the callers check before a
scan, for the sake of symmetry. Feels like overkill to me.

> > -static enum scan_result hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
> > +enum scan_result collapse_vma_revalidate(struct mm_struct *mm, unsigned long address,
> >  		bool expect_anon, struct vm_area_struct **vmap,
> >  		struct collapse_control *cc, unsigned int order)
> >  {
> > @@ -1264,7 +1264,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm,
> >  	}
> >  
> >  	mmap_read_lock(mm);
> > -	result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > +	result = collapse_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> >  					 &vma, cc, order);
> >  	if (result != SCAN_SUCCEED) {
> >  		mmap_read_unlock(mm);
> > @@ -1299,7 +1299,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm,
> >  	 * mmap_lock.
> >  	 */
> >  	mmap_write_lock(mm);
> > -	result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > +	result = collapse_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> >  					 &vma, cc, order);
> >  	if (result != SCAN_SUCCEED)
> >  		goto out_up_write;
> > @@ -2741,7 +2741,8 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
> >  	return result;
> >  }
> >  
> > -static void collapse_control_init(struct collapse_control *cc)
> > +/* Set up a control before its first scan; cc->policy is the caller's to fill */
> 
> 
> Kerneldoc please. Applies to the other ones exposed as part of the same
> interface as well.

Will do, for all of them. The overview in collapse.h stays; it is what
ties the calls together and says who holds the lock between them.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov