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 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
>>>
>>