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

From: Lorenzo Stoakes
Date: Thu Jun 12 2025 - 09:36:42 EST

Next message: kernel test robot: "[tip:perf/urgent] BUILD SUCCESS bc4394e5e79cdda1b0997e0be1d65e242f523f02"
Previous message: Steven Rostedt: "[PATCH] binder: Remove unused binder lock events"
In reply to: Baolin Wang: "Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the system-wide THP sysfs settings are disabled"
Next in thread: Lorenzo Stoakes: "Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the system-wide THP sysfs settings are disabled"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jun 12, 2025 at 03:05:22PM +0200, David Hildenbrand wrote:
> On 12.06.25 14:45, Baolin Wang wrote:
> >
> >
> > On 2025/6/12 16:51, David Hildenbrand wrote:
> > > On 12.06.25 09:51, Baolin Wang wrote:
> > > >
> > > >
> > > > On 2025/6/11 20:34, David Hildenbrand wrote:
> > > > > On 05.06.25 10:00, Baolin Wang wrote:
> > > > > > The MADV_COLLAPSE will ignore the system-wide Anon THP sysfs settings,
> > > > > > which
> > > > > > means that even though we have disabled the Anon THP configuration,
> > > > > > MADV_COLLAPSE
> > > > > > will still attempt to collapse into a Anon THP. This violates the rule
> > > > > > we have
> > > > > > agreed upon: never means never.
> > > > > >
> > > > > > Another rule for madvise, referring to David's suggestion: “allowing
> > > > > > for collapsing
> > > > > > in a VM without VM_HUGEPAGE in the "madvise" mode would be fine".
> > > > > >
> > > > > > To address this issue, should check whether the Anon THP configuration
> > > > > > is disabled
> > > > > > in thp_vma_allowable_orders(), even when the TVA_ENFORCE_SYSFS flag is
> > > > > > set.
> > > > > >
> > > > > > In summary, the current strategy is:
> > > > > >
> > > > > > 1. If always & orders == 0, and madvise & orders == 0, and
> > > > > > hugepage_global_enabled() == false
> > > > > > (global THP settings are not enabled), it means mTHP of that orders
> > > > > > are prohibited
> > > > > > from being used, then madvise_collapse() is forbidden for that orders.
> > > > > >
> > > > > > 2. If always & orders == 0, and madvise & orders == 0, and
> > > > > > hugepage_global_enabled() == true
> > > > > > (global THP settings are enabled), and inherit & orders == 0, it means
> > > > > > mTHP of that
> > > > > > orders are still prohibited from being used, thus madvise_collapse()
> > > > > > is not allowed
> > > > > > for that orders.
> > > > > >
> > > > > > Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
> > > > > > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > >    include/linux/huge_mm.h | 23 +++++++++++++++++++----
> > > > > >    1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > > > > > index 2f190c90192d..199ddc9f04a1 100644
> > > > > > --- a/include/linux/huge_mm.h
> > > > > > +++ b/include/linux/huge_mm.h
> > > > > > @@ -287,20 +287,35 @@ 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 (vma_is_anonymous(vma)) {
> > > > > > +        unsigned long always = READ_ONCE(huge_anon_orders_always);
> > > > > > +        unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
> > > > > > +        unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);
> > > > > > +        unsigned long mask = always | madvise;
> > > > > > +
> > > > > > +        /*
> > > > > > +         * If the system-wide THP/mTHP sysfs settings are disabled,
> > > > > > +         * then we should never allow hugepages.
> > > > > > +         */> +        if (!(mask & orders) &&
> > > > > !(hugepage_global_enabled() && (inherit & orders)))
> > > > > > +            return 0;
> > > > >
> > > > > I'm still trying to digest that. Isn't there a way for us to work with
> > > > > the orders,
> > > > > essentially masking off all orders that are forbidden globally. Similar
> > > > > to below, if !orders, then return 0?
> > > > > /* Orders disabled directly. */
> > > > > orders &= ~TODO;
> > > > > /* Orders disabled by inheriting from the global toggle. */
> > > > > if (!hugepage_global_enabled())
> > > > >     orders &= ~READ_ONCE(huge_anon_orders_inherit);
> > > > >
> > > > > TODO is probably a -1ULL and then clearing always/madvise/inherit. Could
> > > > > add a simple helper for that
> > > > >
> > > > > huge_anon_orders_never
> > > >
> > > > I followed Lorenzo's suggestion to simplify the logic. Does that look
> > > > more readable?
> > > >
> > > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > > > index 2f190c90192d..3087ac7631e0 100644
> > > > --- a/include/linux/huge_mm.h
> > > > +++ b/include/linux/huge_mm.h
> > > > @@ -265,6 +265,43 @@ 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)
> > > > +{
> > > > +       unsigned long always = READ_ONCE(huge_anon_orders_always);
> > > > +       unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
> > > > +       unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);
> > > > +       bool inherit_enabled = hugepage_global_enabled();
> > > > +       bool has_madvise = vm_flags & VM_HUGEPAGE;
> > > > +       unsigned long mask = always | madvise;
> > > > +
> > > > +       mask = always | madvise;
> > > > +       if (inherit_enabled)
> > > > +               mask |= inherit;
> > > > +
> > > > +       /* All set to/inherit NEVER - never means never globally,
> > > > abort. */
> > > > +       if (!(mask & orders))
> > > > +               return 0;
> > >
> > > Still confusing. I am not sure if we would properly catch when someone
> > > specifies e.g., 2M and 1M, while we only have 2M disabled.
> >
> > IIUC, Yes. In your case, we will only allow order 8 (1M mTHP).
> >
> > > I would rewrite the function to only ever substract from "orders".
> > >
> > > ...
> > >
> > > /* Disallow orders that are set to NEVER directly ... */
> > > order &= (always | madvise | inherit);
> > >
> > > /* ... or through inheritance. */
> > > if (inherit_enabled)
> > >     orders &= ~inherit;
> >
> > Sorry, I didn't get you here.
> >
> > If orders = THP_ORDERS_ALL_ANON, inherit = 0x200 (order 9), always and
> > madvise are 0, and inherit_enabled = true. Then orders will be 0 with
> > your logic. But we should allow order 9, right?
>
> Yeah, all confusing, because the temporary variables don't help.
>
> if (!inherit_enabled)
>
> or simply
>
> if (!hugepage_global_enabled();)
>
> Let me try again below.
>
> >
> > >
> > > /*
> > > * 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 (!orders || !(tva_flags & TVA_ENFORCE_SYSFS))
> > >     return orders;
> > >
> > > > +
> > > > +       /*
> > > > +        * 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;
> > > > +
> > > > +       mask = always;
> > > > +       if (has_madvise)
> > > > +               mask |= madvise;
> > > > +       if (hugepage_global_always() || (has_madvise && inherit_enabled))
> > > > +               mask |= inherit;
> > >
> > > Similarly, this can maybe become (not 100% sure if I got it right, the
> > > condition above is confusing)
> >
> > IMO, this is the original logic.
>
> Yeah, and it's absolutely confusing stuff.
>
> Let me try again by only clearing flags. Maybe this would be clearer?
> (and correct? still confused why the latter part is so complicated in existing
> code)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 8b8f353cc7b81..66fdfe06e4996 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -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.

> +
> + if (!(vm_flags & VM_HUGEPAGE)) {

Don't love this sort of mega negation here. I read this as _does_ have huge
page...

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

> + }

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

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
>

Next message: kernel test robot: "[tip:perf/urgent] BUILD SUCCESS bc4394e5e79cdda1b0997e0be1d65e242f523f02"
Previous message: Steven Rostedt: "[PATCH] binder: Remove unused binder lock events"
In reply to: Baolin Wang: "Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the system-wide THP sysfs settings are disabled"
Next in thread: Lorenzo Stoakes: "Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the system-wide THP sysfs settings are disabled"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]