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

[David Hildenbrand <david@redhat.com>]

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

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(). :)

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

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

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


-- 
Cheers,

David / dhildenb