Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the system-wide THP sysfs settings are disabled
From: Baolin Wang
Date: Thu Jun 12 2025 - 22:07:40 EST
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.