| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7c911fff-aaf1-430c-89f2-5e7925e90a04@linux.alibaba.com>
Date: Fri, 13 Jun 2025 10:07:20 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
David Hildenbrand <david@...hat.com>
Cc: akpm@...ux-foundation.org, hughd@...gle.com, Liam.Howlett@...cle.com,
npache@...hat.com, ryan.roberts@....com, dev.jain@....com, ziy@...dia.com,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the
system-wide THP sysfs settings are disabled
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.
Powered by blists - more mailing lists