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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <201a1cc4-93fc-48e3-aeab-759ba8c8a47c@lucifer.local>
Date: Thu, 12 Jun 2025 14:29:57 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.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

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.

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