From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 721D03ED124; Fri, 8 May 2026 15:02:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778252558; cv=none; b=YMD4+DKng8Lt/b2KKRGcS5DcxuoSZJrqSdUIN/8Uid4jXmZMihddLwJuxHp/ctaQGGuD+XdihSmx+kY0X8DX6iNYTuMIvCwT1hd+B99/PiWUL5i/thYj70+rkqQUm/R2lz+LYI4thm/rneBDrZ67jVD2mIUlnSMOy3+6xxwnDxc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778252558; c=relaxed/simple; bh=mphj7q9xvUeukrTjcJQdbgg/bdUNv7wggxgvnM+pxdc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dHpCJEiCUAXtaiH6qq5s256MjwEuwzm8wgZd6f5KGyyEC83yhMJtLDi4xQ5IDF+bV8hWwrA2gLlNiqXYjk2v2rfBHsBTVpoJAb8Z0hmD923N/ESLepKoVwuIVcsUEMoJq2vvDGP7we0sCvM0q/4ZZsox2epEL+YZuTtxbfC9Ucw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=b9HrlB1T; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="b9HrlB1T" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 11CCEC2BCB0; Fri, 8 May 2026 15:02:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1778252558; bh=mphj7q9xvUeukrTjcJQdbgg/bdUNv7wggxgvnM+pxdc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=b9HrlB1TqPvXiGkKX53Q3c2fpsAAZUB9kEoCE1eVZjUykNyeQyTRSKw24cSGC1i53 skXSYIGQ0jQs7UdRBRHE3qAhKgWOGHWjtc37Z4u5Lx69Dg05MDoVkOeoT3FBwKb6Fz YsHUseM6F1BmTUCSWLmaWWd0lHZ0xIyTOz+GQpDysIILxeiwcGSdVVWm/XCz9egBil RtZNJkAd47UMLZuEI22XoZOSjj5Ovty3Xvaw2IyOe81Vk8Oy3AK4EhkTRbikSejZgO +mxEtJ+mi3iqBrw/2dfQVX/dtSCJVcfQXkiHmUGOne+NdMxnfV9rJcsrN9T6YmYBKf WwCQNI/z+Q/Bw== Date: Fri, 8 May 2026 16:02:31 +0100 From: Lorenzo Stoakes To: Chen Wandun Cc: "David Hildenbrand (Arm)" , akpm@linux-foundation.org, shuah@kernel.org, zokeefe@google.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH 1/2] mm/khugepaged: fix spurious -EINVAL from sub-PMD MADV_COLLAPSE range Message-ID: References: <20260507070558.3064142-1-chenwandun@lixiang.com> <20260507070558.3064142-2-chenwandun@lixiang.com> <9eea2afb-8c35-47eb-b1de-6a08503c9679@kernel.org> Precedence: bulk X-Mailing-List: linux-kselftest@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9eea2afb-8c35-47eb-b1de-6a08503c9679@kernel.org> 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. > > 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 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