| 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: <976f8544-91b7-49f7-975f-fac29a00d3fa@lucifer.local>
Date: Thu, 12 Jun 2025 15:20:50 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc: David Hildenbrand <david@...hat.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
On Thu, Jun 12, 2025 at 10:13:40PM +0800, Baolin Wang wrote:
>
>
> On 2025/6/12 21:29, Lorenzo Stoakes wrote:
> > On Thu, Jun 12, 2025 at 02:27:06PM +0100, Lorenzo Stoakes wrote:
> > [snip]
> >
> > > 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);
> >
> > Of course... I immediately made a mistake... swap these two statements around. I
> > thought it'd be 'neater' to do the first one first, but of course it means
> > madvise (rather than inherit) orders don't get selected.
> >
> > This WHOLE THING needs refactoring.
>
> Personally, I think the 'exclude never' logic becomes more complicated. I
> made a simpler change without adding a new helper. What do you think?
>
> static inline
> unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
> unsigned long vm_flags,
> unsigned long tva_flags,
> unsigned long orders)
> {
> /* Optimization to check if required orders are enabled early. */
> if (vma_is_anonymous(vma)) {
I hate the level of indentation here. There's really no reason not to have this
as a helper as this just solves this problem and any sane compiler will inline.
> unsigned long mask = READ_ONCE(huge_anon_orders_always);
> bool huge_enforce = !(tva_flags & TVA_ENFORCE_SYSFS);
Huge enforce is when we don't have enforce flag set? This is super confusing.
> bool has_madvise = vm_flags & VM_HUGEPAGE;
>
> /*
> * 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 (huge_enforce || has_madvise)
> mask |= READ_ONCE(huge_anon_orders_madvise);
I find this more confusing, honestly.
I far prefer having the never checks up front, and I prefer David's 'explicitly
deal with never through negations' approach.
I also think adding these READ_ONCE()'s here adds a ton of noise.
> if (hugepage_global_always() ||
> ((has_madvise || huge_enforce) &&
> hugepage_global_enabled()))
This combination of conditions is just horribly confusing. And why are you
giving an explanation above but not for this one...
This is just combining a ton of logic in a confusing way.
> mask |= READ_ONCE(huge_anon_orders_inherit);
>
> orders &= mask;
> if (!orders)
> return 0;
> }
>
> return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
> }
Thanks for the suggestion but I do prefer the proposed compromise solution.
>
> >
> > >
> > > 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
> > > >
> > >
Powered by blists - more mailing lists