Re: [PATCH mm-unstable v4 5/5] mm/khugepaged: unify khugepaged and madv_collapse with collapse_single_pmd()

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

On 3/31/26 22:06, Lorenzo Stoakes (Oracle) wrote:
> On Tue, Mar 31, 2026 at 10:00:58PM +0200, David Hildenbrand (Arm) wrote:
>> On 3/31/26 16:15, Lorenzo Stoakes (Oracle) wrote:
>>>
>>> That updates mmap_unlocked which will be handled in the loop or on exit:
>>>
>>> 		if (mmap_unlocked) {
>>> 			...
>>> 			mmap_read_lock(mm);
>>> 			mmap_unlocked = false;
>>> 			*lock_dropped = true;
>>> 			...
>>> 		}
>>>
>>> 		result = collapse_single_pmd(addr, vma, &mmap_unlocked, cc);
>>>
>>> ...
>>>
>>> out_maybelock:
>>> 	/* Caller expects us to hold mmap_lock on return */
>>> 	if (mmap_unlocked) {
>>> 		*lock_dropped = true;
>>> 		mmap_read_lock(mm);
>>> 	}
>>>
>>
>> Right.
>>
>> The original code used
>>
>> 	bool mmap_locked = true;
>>
>> If the fix get squashed, maybe we should revert to that handling to
>> minimize the churn?
> 
> Well collapse_single_pmd() and all descendents would need to be updated to
> invert the boolean, not sure it's worth it.

The code is confusing me.

In the old code, collapse_single_pmd() was called with "mmap_locked ==
true" and set "mmap_locked=false".

Now we call it with "mmap_unlocked=false" and it sets "mmap_unlocked=true".

So far so good.

However, collapse_scan_pmd() consumed "mmap_locked", now it effectively
consumes "mmap_unlocked".

I think I have to squash your fix into Nicos patch to make sense of this. :)


-- 
Cheers,

David