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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2025/6/12 22:49, Lorenzo Stoakes wrote:
> 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!

Fair enough. You both prefer the 'exclude never' logic, and I will 
follow that logic.

>>>> +       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;
> }

Thanks Lorenzo. Let me follow this logic and do some testing.