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

[Baolin Wang]

On 2025/6/12 21:29, Lorenzo Stoakes wrote:
> On Thu, Jun 12, 2025 at 02:27:06PM +0100, Lorenzo Stoakes wrote:
> [snip]
>
> > 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();
> >
> > 	/* 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);
>
> Of course... I immediately made a mistake... swap these two statements around. I
> thought it'd be 'neater' to do the first one first, but of course it means
> madvise (rather than inherit) orders don't get selected.
>
> This WHOLE THING needs refactoring.

Personally, I think the 'exclude never' logic becomes more complicated. 
I made a simpler change without adding a new helper. What do you think?

static inline
unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
                                       unsigned long vm_flags,
                                       unsigned long tva_flags,
                                       unsigned long orders)
{
        /* Optimization to check if required orders are enabled early. */
        if (vma_is_anonymous(vma)) {
                unsigned long mask = READ_ONCE(huge_anon_orders_always);
                bool huge_enforce = !(tva_flags & TVA_ENFORCE_SYSFS);
                bool has_madvise =  vm_flags & VM_HUGEPAGE;

                /*
                 * 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 (huge_enforce || has_madvise)
                        mask |= READ_ONCE(huge_anon_orders_madvise);
                if (hugepage_global_always() ||
                    ((has_madvise || huge_enforce) && 
hugepage_global_enabled()))
                        mask |= READ_ONCE(huge_anon_orders_inherit);

                orders &= mask;
                if (!orders)
                        return 0;
        }

        return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, 
orders);
}

> > 	return orders & always;
> > }
> >
> > What do you think?
> >
> >
> > > +       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.
> >
> > I mean we _could_ keep it but I think it's better not to for cleanliness, what
> > do you think?
> >
> > >          }
> > >
> > >
> > > --
> > > Cheers,
> > >
> > > David / dhildenb