Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the system-wide THP sysfs settings are disabled

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Jun 12, 2025 at 04:09:27PM +0200, David Hildenbrand wrote:
>
>
> > > @@ -265,6 +265,42 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
> > >                                           unsigned long tva_flags,
> > >                                           unsigned long orders);
> > > +/* Strictly mask requested anonymous orders according to sysfs settings. */
> > > +static inline unsigned long __thp_mask_anon_orders(unsigned long vm_flags,
> > > +       unsigned long tva_flags, unsigned long orders)
> > > +{
> > > +       const unsigned long always = READ_ONCE(huge_anon_orders_always);
> > > +       const unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
> > > +       const unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);
> > > +       const unsigned long never = ~(always | madvise | inherit);
> > > +
> > > +       /* Disallow orders that are set to NEVER directly ... */
> > > +       orders &= ~never;
> > > +
> > > +       /* ... or through inheritance (global == NEVER). */
> > > +       if (!hugepage_global_enabled())
> > > +               orders &= ~inherit;
> > > +
> > > +       /*
> > > +        * Otherwise, we only enforce sysfs settings if asked. In addition,
> > > +        * if the user sets a sysfs mode of madvise and if TVA_ENFORCE_SYSFS
> > > +        * is not set, we don't bother checking whether the VMA has VM_HUGEPAGE
> > > +        * set.
> > > +        */
> > > +       if (!(tva_flags & TVA_ENFORCE_SYSFS))
> > > +               return orders;
> >
> > This implicitly does a & mask as per suggested previous version, which I think
> > is correct but worth pointing out.
>
> Yes.
>
> >
> > > +
> > > +       if (!(vm_flags & VM_HUGEPAGE)) {
> >
> > Don't love this sort of mega negation here. I read this as _does_ have huge
> > page...
>
> Well, it's very common to do that, but not objecting to something that is
> clearer ;)
>
> I assume you spotted the
>
> if (!(tva_flags & TVA_ENFORCE_SYSFS))
>
> :P

Lol yeah I know I know, I just think I guess in this case because you're
negating elsewhere it makes it harder...

>
> if (vm_flags & VM_HUGEPAGE)
> 	return orders;
>
>
> Would have been easier.
>
> >
> > > +               /* Disallow orders that are set to MADVISE directly ... */
> > > +               orders &= ~madvise;
> > > +
> > > +               /* ... or through inheritance (global == MADVISE). */
> > > +               if (!hugepage_global_always())
> > > +                       orders &= ~inherit;
> >
> > I hate this implicit 'not hugepage global always so this means either never or
> > madvise and since we cleared orders for never this means madvise' mental
> > gymnastics required here.
> >
> > Yeah I feel this is a bridge too far, we're getting into double negation and I
> > think that's more confusiong.
>
>
> Same here ... I think we should just have hugepage_global_madvise(). :)

Ideally in future not have these stupid globals all over the place and rework
this whole damn thing...

>
> >
> >
> > > +       }
> >
> > I propose a compromise as I rather like your 'exclude never' negation bit.
> >
> > So:
> >
> > /* Strictly mask requested anonymous orders according to sysfs settings. */
> > static inline unsigned long __thp_mask_anon_orders(unsigned long vm_flags,
> >                  unsigned long tva_flags, unsigned long orders)
> > {
> >          const unsigned long always = READ_ONCE(huge_anon_orders_always);
> >          const unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
> >          const unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);;
> > 	const unsigned long never = ~(always | madvise | inherit);
> >          const bool inherit_enabled = hugepage_global_enabled();
>
> Can we just have hugepage_global_never/disabled() to use instead?

This would be nice!

Could be a follow up... though again would be nice to somehow do away with all
this crap altogether.

>
> >
> > 	/* Disallow orders that are set to NEVER directly ... */
> > 	orders &= ~never;
> >
> > 	/* ... or through inheritance (global == NEVER). */
> > 	if (!inherit_enabled)
> > 		orders &= ~inherit;>
> > 	/*
> > 	 * Otherwise, we only enforce sysfs settings if asked. In addition,
> > 	 * if the user sets a sysfs mode of madvise and if TVA_ENFORCE_SYSFS
> > 	 * is not set, we don't bother checking whether the VMA has VM_HUGEPAGE
> > 	 * set.
> > 	 */
> > 	if (!(tva_flags & TVA_ENFORCE_SYSFS))
> > 		return orders;
> >
> > 	if (hugepage_global_always())
> > 		return orders & (always | inherit);
> >
> > 	/* We already excluded never inherit above. */
> > 	if (vm_flags & VM_HUGEPAGE)
> > 		return orders & (always | madvise | inherit);
> >
> > 	return orders & always;
> > }
> >
> > What do you think?
>
> With the fixup, it would work for me. No magical "mask" variables :D

Thanks!

>
> > >
> > > +       return orders;
> > > +}
> > > +
> > >   /**
> > >    * thp_vma_allowable_orders - determine hugepage orders that are allowed for vma
> > >    * @vma:  the vm area to check
> > > @@ -287,16 +323,8 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
> > >                                         unsigned long orders)
> > >   {
> > >          /* Optimization to check if required orders are enabled early. */
> > > -       if ((tva_flags & TVA_ENFORCE_SYSFS) && vma_is_anonymous(vma)) {
> > > -               unsigned long mask = READ_ONCE(huge_anon_orders_always);
> > > -
> > > -               if (vm_flags & VM_HUGEPAGE)
> > > -                       mask |= READ_ONCE(huge_anon_orders_madvise);
> > > -               if (hugepage_global_always() ||
> > > -                   ((vm_flags & VM_HUGEPAGE) && hugepage_global_enabled()))
> > > -                       mask |= READ_ONCE(huge_anon_orders_inherit);
> > > -
> > > -               orders &= mask;
> > > +       if (vma_is_anonymous(vma)) {
> > > +               orders = __thp_mask_anon_orders(vm_flags, tva_flags, orders);
> > >                  if (!orders)
> > >                          return 0;
> >
> > I pointed out to Baolin that __thp_vma_allowable_orders() handles the orders ==
> > 0 case almost immediately so there's no need to do this, it just makes the code
> > noisier.
>
> The reason we added it in the first place was to not do the (expensive)
> function call.

Ack point taken!

>
>
> --
> Cheers,
>
> David / dhildenb
>

For convenience, I enclose the fixed version + tweaked the inherit local bool to
be inherit_never to be clearer:

/* Strictly mask requested anonymous orders according to sysfs settings. */
static inline unsigned long __thp_mask_anon_orders(unsigned long vm_flags,
                unsigned long tva_flags, unsigned long orders)
{
        const unsigned long always = READ_ONCE(huge_anon_orders_always);
        const unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
        const unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);;
        const unsigned long never = ~(always | madvise | inherit);
        const bool inherit_never = !hugepage_global_enabled();

        /* Disallow orders that are set to NEVER directly ... */
        orders &= ~never;

        /* ... or through inheritance (global == NEVER). */
        if (inherit_never)
                orders &= ~inherit;

        /*
         * Otherwise, we only enforce sysfs settings if asked. In addition,
         * if the user sets a sysfs mode of madvise and if TVA_ENFORCE_SYSFS
         * is not set, we don't bother checking whether the VMA has VM_HUGEPAGE
         * set.
         */
        if (!(tva_flags & TVA_ENFORCE_SYSFS))
                return orders;

        /* We already excluded never inherit above. */
        if (vm_flags & VM_HUGEPAGE)
                return orders & (always | madvise | inherit);

        if (hugepage_global_always())
                return orders & (always | inherit);

        return orders & always;
}