| 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: <ee646bca-e77d-4452-82f8-0bdb4b241f9c@redhat.com>
Date: Thu, 12 Jun 2025 16:09:27 +0200
From: David Hildenbrand <david@...hat.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Baolin Wang <baolin.wang@...ux.alibaba.com>, 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
>> @@ -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
Powered by blists - more mailing lists