Re: [PATCH 1/2] mm/khugepaged: fix spurious -EINVAL from sub-PMD MADV_COLLAPSE range

[Wandun <chenwandun1@gmail.com>]

On 5/8/26 23:04, Lorenzo Stoakes wrote:
> On Fri, May 08, 2026 at 04:02:37PM +0100, Lorenzo Stoakes wrote:
>> On Fri, May 08, 2026 at 02:27:37PM +0200, David Hildenbrand (Arm) wrote:
>>> On 5/7/26 09:05, Chen Wandun wrote:
>>>> madvise_collapse() computes the THP-aligned window:
>>>>
>>>>    hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK  /* round up  */
>>>>    hend   =  end   &  HPAGE_PMD_MASK                    /* round down */
>>>>
>>>> Previously this was done after kmalloc_obj(), so problem arose when
>>>> the range contained no complete PMD-aligned window (hstart >= hend).
>>>>
>>>> When hstart > hend, (hend - hstart) wraps unsigned to a huge value, the
>>>> final comparison fails and -EINVAL is returned instead of 0.  Consider
>> I think both should return -EINVAL.
> Correction: I changed my mind (see below), and think == should return 0 simply
> for compatibility reasons. Though honestly both really should have been -EINVAL
> from the start...
I also hesitated before return -EINVAL or 0, for the two cases mentioned 
above, it
didn't collapse into any THP, so I wanted to return -EINVAL.

Later, I saw the function comment for do_madvise and madvise manual, so 
I thought
the cases mentioned above didn't meet the conditions for returning 
-EINVAL, so
I followed madvise manual and do_madvise comment.

Best regards,
Wandun

>
>>>> two single-page calls on a 2 MiB-aligned address:
>>>>
>>>>      /* hstart == hend == aligned  ->  0 == 0  ->  returns 0 */
>>>>      madvise(aligned, PAGE_SIZE, MADV_COLLAPSE);
>> What's aligned? You're putting a random variable name in there? Presumably a PMD-aligned address?
>>
>>>>      /* hstart = aligned + 2MiB, hend = aligned
>>>>       * (hend - hstart) wraps unsigned  ->  returns -EINVAL */
>>>>      madvise(aligned + PAGE_SIZE, PAGE_SIZE, MADV_COLLAPSE);
>>>>
>>>> Both calls cover less than one THP and collapse nothing; both should
>>>> return 0.
>> Disagree.
>>
>>> Okay, so we talk about a "userspace is being stupid" scenario.
>> Yes!
>>
>> I feel that -EINVAL is correct for hend > hstart, and I think it might even be a
>> userland A[BP]I break to change it (maybe somebody, somewhere is being foolish
>> enough to use this to also validate input ranges).
>>
>> The weirdness is when hstart == hend being 0 but that's sort of established
>> behaviour I guess.
>>
>>>> In addition, kmalloc_obj(), mmgrab() and lru_add_drain_all() were all
>>>> called before discovering there was nothing to do, only for the code
>>>> to kfree() and return immediately after.
>>> Just a comment as you motivate here why this is suboptimal: we do not care about
>>> a "userspace is being stupid" scenario being fast.
>> Yes, in general - so what? The user is doing stupid things, so the user wins
>> stupid prizes?
>>
>>>> Fix both by computing hstart/hend after thp_vma_allowable_order() but
>>>> before kmalloc_obj(), and returning 0 early when hstart >= hend.
>>>>
>>>> Fixes: 7d8faaf15545 ("mm/madvise: introduce MADV_COLLAPSE sync hugepage collapse")
>>> Fixes: is likely ok, but I don't think we want to treat this as a hotfix or CC
>>> stable.
>> I'm not sure I want a fixes here, this isn't really fixing anything. This isn't
>> a bug afaik, it's just us not handling this brilliantly, but (possibly by
>> mistake) getting the right output.
>>
>>>> Signed-off-by: Chen Wandun <chenwandun@lixiang.com>
>> I put this patch through AI detection and it's telling me there's an 80% chance
>> this whole thing is LLM-generated, which is making me grumpy.
>>
>> Can you confirm that this is, in fact, your own work? Plagiarism is not a nice
>> thing to do, and THP doesn't need more traffic, we're overloaded as it is.
>>
>>>> ---
>>>>   mm/khugepaged.c | 9 ++++++---
>>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>>> index b8452dbdb043..92473d93e837 100644
>>>> --- a/mm/khugepaged.c
>>>> +++ b/mm/khugepaged.c
>>>> @@ -2836,6 +2836,12 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>>>>   	if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_FORCED_COLLAPSE, PMD_ORDER))
>>>>   		return -EINVAL;
>>>>
>>>> +	hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
>>>> +	hend = end & HPAGE_PMD_MASK;
>> See below re: conflict.
>>
>>>> +
>>>> +	if (hstart >= hend)
>>>> +		return 0;
>> 	if (hstart > hend)
>> 		return -EINVAL;
>> 	/* For compatibility, users may rely on this. */
>> 	if (hstart == hend)
>> 		return 0;
>>
>> Is probably better.
>>
>> But I'm not sure what the point is if we're already doing this behaviour?
>>
>>>> +
>>>>   	cc = kmalloc_obj(*cc);
>>>>   	if (!cc)
>>>>   		return -ENOMEM;
>>>> @@ -2845,9 +2851,6 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>>>>   	mmgrab(mm);
>>>>   	lru_add_drain_all();
>>>>
>>>> -	hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
>>>> -	hend = end & HPAGE_PMD_MASK;
>>>> -
>>>>   	for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) {
>>>>   		enum scan_result result = SCAN_FAIL;
>>>>
>>> In general, LGTM, but see for conflict:
>>> https://lore.kernel.org/all/20260409014323.2385982-1-ye.liu@linux.dev/
>> Please use mm-unstable as a basis for your mm work Chen, this is something you
>> need to fix, the patch above has been around for a while and is in
>> mm-unstable.
>>
>> You have patches in mm already so you should know better by now.
>>
>> But I'm really not sure I'm in favour of this anyway. I'll defer to David but
>> this feels useless to me.
>>
>>>
>>> --
>>> Cheers,
>>>
>>> David
>> Thanks, Lorenzo