Just a ping on the man page stuff - you will do that right? :>)
Probably best at end of 6.18 cycle when this is about to go to Linus.
I can give you some details on how that all works if it'd be helpful.
On Wed, Jul 30, 2025 at 08:42:04PM +0100, Usama Arif wrote:
> >> Acked-by: Usama Arif <usamaarif642@xxxxxxxxx>
> >> Tested-by: Usama Arif <usamaarif642@xxxxxxxxx>
[snip]
> >> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
All looks good aside from nits, so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> >> +/*
> >> + * Check whether THPs are explicitly disabled for this VMA, for example,
> >> + * through madvise or prctl.
> >> + */
> >> static inline bool vma_thp_disabled(struct vm_area_struct *vma,
> >> vm_flags_t vm_flags)
> >> {
> >> + /* Are THPs disabled for this VMA? */
> >> + if (vm_flags & VM_NOHUGEPAGE)
> >> + return true;
>
> VM_NOHUGEPAGE will cause the THP being disabled here.
>
> >> + /* Are THPs disabled for all VMAs in the whole process? */
> >> + if (test_bit(MMF_DISABLE_THP_COMPLETELY, &vma->vm_mm->flags))
> >> + return true;
> >> /*
> >> - * Explicitly disabled through madvise or prctl, or some
> >> - * architectures may disable THP for some mappings, for
> >> - * example, s390 kvm.
> >> + * Are THPs disabled only for VMAs where we didn't get an explicit
> >> + * advise to use them?
> >> */
> >> - return (vm_flags & VM_NOHUGEPAGE) ||
> >> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags);
> >> + if (vm_flags & VM_HUGEPAGE)
> >> + return false;
> >
> > Hm is this correct? This means that VM_NOHUGEPAGE no longer results in THP being
> > disabled here no?
>
> Lorenzo, pointed to VM_NOHUGEPAGE case above.>
Haha doh! OK cool, obviously I did this review a little too late in the day
when Mr. Brain was not on best form :P
All good, thanks!
And of course this now enforces the 'except advised' logic below it.
> >> +/*
> >> + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
> >> + * VM_HUGEPAGE).
> >> + */
> >> +# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
> >
> > NO space after # please.
> >
>
> I think this is following the file convention, the space is there in other flags
> all over this file. I dont like the space as well.
Yeah yuck. It's not a big deal, but ideally I'd prefer us to be sane even
if the rest of the header is less so here.
>
> >> #define PR_GET_THP_DISABLE 42
> >>
> >> /*
> >> diff --git a/kernel/sys.c b/kernel/sys.c
> >> index b153fb345ada..b87d0acaab0b 100644
> >> --- a/kernel/sys.c
> >> +++ b/kernel/sys.c
> >> @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
> >> return sizeof(mm->saved_auxv);
> >> }
> >>
> >> +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
> >> + unsigned long arg4, unsigned long arg5)
> >> +{
> >> + unsigned long *mm_flags = ¤t->mm->flags;
> >> +
> >> + if (arg2 || arg3 || arg4 || arg5)
> >> + return -EINVAL;
> >> +
> >
> > Can we have a comment here about what we're doing below re: the return
> > value?
> >
>
> Do you mean add returning 1 for MMF_DISABLE_THP_COMPLETELY and 3 for MMF_DISABLE_THP_EXCEPT_ADVISED?
Well more so something about we return essentially flags indicating what is
enabled or not, if bit 0 is set then it's disabled, if bit 1 is set then
it's that with the exception of VM_HUGEPAGE VMAs.
>
> I will add it in next revision.
Thanks!
