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

[David Hildenbrand (Arm)]

On 9/24/26 17:22, Kiryl Shutsemau wrote:
> 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.


I much rather have a consistent collapse_* interface than mixing in other weird
functions.

I saw that you already sent a v4 (too fast ...) so I might complain about the
same thing there once more and we'd end up with the same discussion there once
more. Which is expected when people don't wait for replies.

-- 
Cheers,

David