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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/16/26 11:31, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
> 
> 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.

	collapse_pmd()

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


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


[...]

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

-- 
Cheers,

David