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 <[email protected]>
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/[email protected]/
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
