[PATCH] mm/hugetlb: sort out global lock annotations

[PATCH] mm/hugetlb: sort out global lock annotations

[Mateusz Guzik]

The mutex array pointer shares a cacheline with the spinlock:
ffffffff84187480 B hugetlb_fault_mutex_table
ffffffff84187488 B hugetlb_lock

This is because the former is annotated with a macro forcing cacheline
alignment. I suspect it was meant to be the variant which on top of it
makes sure the object does not share the cacheline with anyone.

Since array pointer itself is de facto read-only such an annotation does
not make sense there anyway. Instead mark it __ro_after_init along with
the size var.

Do however move the spinlock out of the way.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

I did not benchmark any of it, looks like basic sanity to me.

This came up as a side effect of an unrelated discussion.

 mm/hugetlb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 4461d27f7453..1a833f016847 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -72,14 +72,14 @@ static unsigned int default_hugepages_in_node[MAX_NUMNODES] __initdata;
  * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
  * free_huge_pages, and surplus_huge_pages.
  */
-DEFINE_SPINLOCK(hugetlb_lock);
+__cacheline_aligned_in_smp DEFINE_SPINLOCK(hugetlb_lock);
 
 /*
  * Serializes faults on the same logical page.  This is used to
  * prevent spurious OOMs when the hugepage pool is fully utilized.
  */
-static int num_fault_mutexes;
-struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp;
+static __ro_after_init int num_fault_mutexes;
+__ro_after_init struct mutex *hugetlb_fault_mutex_table;
 
 /* Forward declaration */
 static int hugetlb_acct_memory(struct hstate *h, long delta);
-- 
2.43.0

Re: [PATCH] mm/hugetlb: sort out global lock annotations

[Andrew Morton]

On Wed, 28 Aug 2024 18:07:04 +0200 Mateusz Guzik <mjguzik@gmail.com> wrote:

> The mutex array pointer shares a cacheline with the spinlock:
> ffffffff84187480 B hugetlb_fault_mutex_table
> ffffffff84187488 B hugetlb_lock

Fair enough.  My x86_64 defconfig now has

num_fault_mutexes:
	.zero	4
	.globl	hugetlb_lock
	.section	.data..cacheline_aligned,"aw"
	.align 64
	.type	hugetlb_lock, @object
	.size	hugetlb_lock, 4
hugetlb_lock:
	.zero	4
	.section	.init.data
	.align 32
	.type	default_hugepages_in_node, @object
	.size	default_hugepages_in_node, 256
default_hugepages_in_node:
	.zero	256
	.type	parsed_default_hugepagesz, @object
	.size	parsed_default_hugepagesz, 1

which looks good.

> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -72,14 +72,14 @@ static unsigned int default_hugepages_in_node[MAX_NUMNODES] __initdata;
>   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
>   * free_huge_pages, and surplus_huge_pages.
>   */
> -DEFINE_SPINLOCK(hugetlb_lock);
> +__cacheline_aligned_in_smp DEFINE_SPINLOCK(hugetlb_lock);
>  
>  /*
>   * Serializes faults on the same logical page.  This is used to
>   * prevent spurious OOMs when the hugepage pool is fully utilized.
>   */
> -static int num_fault_mutexes;
> -struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp;
> +static __ro_after_init int num_fault_mutexes;
> +__ro_after_init struct mutex *hugetlb_fault_mutex_table;

It's conventional (within MM, at least) to put the section thing at the
end of the definition, so tweak:

--- a/mm/hugetlb.c~mm-hugetlb-sort-out-global-lock-annotations-fix
+++ a/mm/hugetlb.c
@@ -72,14 +72,14 @@ static unsigned int default_hugepages_in
  * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
  * free_huge_pages, and surplus_huge_pages.
  */
-__cacheline_aligned_in_smp DEFINE_SPINLOCK(hugetlb_lock);
+DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
 
 /*
  * Serializes faults on the same logical page.  This is used to
  * prevent spurious OOMs when the hugepage pool is fully utilized.
  */
-static __ro_after_init int num_fault_mutexes;
-__ro_after_init struct mutex *hugetlb_fault_mutex_table;
+static int num_fault_mutexes __ro_after_init;
+struct mutex *hugetlb_fault_mutex_table __ro_after_init;
 
 /* Forward declaration */
 static int hugetlb_acct_memory(struct hstate *h, long delta);
_

Re: [PATCH] mm/hugetlb: sort out global lock annotations

[Mateusz Guzik]

On Wed, Aug 28, 2024 at 9:49 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 28 Aug 2024 18:07:04 +0200 Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> > The mutex array pointer shares a cacheline with the spinlock:
> > ffffffff84187480 B hugetlb_fault_mutex_table
> > ffffffff84187488 B hugetlb_lock
>
> Fair enough.  My x86_64 defconfig now has
>
> num_fault_mutexes:
>         .zero   4
>         .globl  hugetlb_lock
>         .section        .data..cacheline_aligned,"aw"
>         .align 64
>         .type   hugetlb_lock, @object
>         .size   hugetlb_lock, 4
> hugetlb_lock:
>         .zero   4
>         .section        .init.data
>         .align 32
>         .type   default_hugepages_in_node, @object
>         .size   default_hugepages_in_node, 256
> default_hugepages_in_node:
>         .zero   256
>         .type   parsed_default_hugepagesz, @object
>         .size   parsed_default_hugepagesz, 1
>
> which looks good.
>
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -72,14 +72,14 @@ static unsigned int default_hugepages_in_node[MAX_NUMNODES] __initdata;
> >   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
> >   * free_huge_pages, and surplus_huge_pages.
> >   */
> > -DEFINE_SPINLOCK(hugetlb_lock);
> > +__cacheline_aligned_in_smp DEFINE_SPINLOCK(hugetlb_lock);
> >
> >  /*
> >   * Serializes faults on the same logical page.  This is used to
> >   * prevent spurious OOMs when the hugepage pool is fully utilized.
> >   */
> > -static int num_fault_mutexes;
> > -struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp;
> > +static __ro_after_init int num_fault_mutexes;
> > +__ro_after_init struct mutex *hugetlb_fault_mutex_table;
>
> It's conventional (within MM, at least) to put the section thing at the
> end of the definition, so tweak:
>
> --- a/mm/hugetlb.c~mm-hugetlb-sort-out-global-lock-annotations-fix
> +++ a/mm/hugetlb.c
> @@ -72,14 +72,14 @@ static unsigned int default_hugepages_in
>   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
>   * free_huge_pages, and surplus_huge_pages.
>   */
> -__cacheline_aligned_in_smp DEFINE_SPINLOCK(hugetlb_lock);
> +DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
>

I tried things in this order and this does not compile for me:
In file included from ./arch/x86/include/asm/current.h:10,
                 from ./arch/x86/include/asm/preempt.h:7,
                 from ./include/linux/preempt.h:79,
                 from ./include/linux/spinlock.h:56,
                 from ./include/linux/mmzone.h:8,
                 from ./include/linux/gfp.h:7,
                 from ./include/linux/mm.h:7,
                 from mm/hugetlb.c:8:
./include/linux/cache.h:80:3: error: expected ‘,’ or ‘;’ before ‘__attribute__’
   80 |   __attribute__((__aligned__(SMP_CACHE_BYTES),                  \
      |   ^~~~~~~~~~~~~
./include/linux/cache.h:86:36: note: in expansion of macro ‘__cacheline_aligned’
   86 | #define __cacheline_aligned_in_smp __cacheline_aligned
      |                                    ^~~~~~~~~~~~~~~~~~~
mm/hugetlb.c:75:31: note: in expansion of macro ‘__cacheline_aligned_in_smp’
   75 | DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~

I'm at next-20240828 with gcc 13.2.0

>  /*
>   * Serializes faults on the same logical page.  This is used to
>   * prevent spurious OOMs when the hugepage pool is fully utilized.
>   */
> -static __ro_after_init int num_fault_mutexes;
> -__ro_after_init struct mutex *hugetlb_fault_mutex_table;
> +static int num_fault_mutexes __ro_after_init;
> +struct mutex *hugetlb_fault_mutex_table __ro_after_init;
>
>  /* Forward declaration */
>  static int hugetlb_acct_memory(struct hstate *h, long delta);
> _
>
>


-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] mm/hugetlb: sort out global lock annotations

[Andrew Morton]

On Wed, 28 Aug 2024 22:13:49 +0200 Mateusz Guzik <mjguzik@gmail.com> wrote:

> > It's conventional (within MM, at least) to put the section thing at the
> > end of the definition, so tweak:
> >
> > --- a/mm/hugetlb.c~mm-hugetlb-sort-out-global-lock-annotations-fix
> > +++ a/mm/hugetlb.c
> > @@ -72,14 +72,14 @@ static unsigned int default_hugepages_in
> >   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
> >   * free_huge_pages, and surplus_huge_pages.
> >   */
> > -__cacheline_aligned_in_smp DEFINE_SPINLOCK(hugetlb_lock);
> > +DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
> >
> 
> I tried things in this order and this does not compile for me:
> In file included from ./arch/x86/include/asm/current.h:10,
>                  from ./arch/x86/include/asm/preempt.h:7,
>                  from ./include/linux/preempt.h:79,
>                  from ./include/linux/spinlock.h:56,
>                  from ./include/linux/mmzone.h:8,
>                  from ./include/linux/gfp.h:7,
>                  from ./include/linux/mm.h:7,
>                  from mm/hugetlb.c:8:
> ./include/linux/cache.h:80:3: error: expected ‘,’ or ‘;’ before ‘__attribute__’
>    80 |   __attribute__((__aligned__(SMP_CACHE_BYTES),                  \
>       |   ^~~~~~~~~~~~~
> ./include/linux/cache.h:86:36: note: in expansion of macro ‘__cacheline_aligned’
>    86 | #define __cacheline_aligned_in_smp __cacheline_aligned
>       |                                    ^~~~~~~~~~~~~~~~~~~
> mm/hugetlb.c:75:31: note: in expansion of macro ‘__cacheline_aligned_in_smp’
>    75 | DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
>       |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~

Well that's annoying.  It's because DEFINE_SPINLOCK includes an initializer.

--- a/mm/hugetlb.c~mm-hugetlb-sort-out-global-lock-annotations-fix-fix
+++ a/mm/hugetlb.c
@@ -72,7 +72,7 @@ static unsigned int default_hugepages_in
  * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
  * free_huge_pages, and surplus_huge_pages.
  */
-DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
+spinlock_t hugetlb_lock __cacheline_aligned_in_smp = __SPIN_LOCK_UNLOCKED(hugetlb_lock);
 
 /*
  * Serializes faults on the same logical page.  This is used to
_

We'd need a new DEFINE_SPINLOCK_ALIGNED() or something.

Ho hum, I'll fix.

Re: [PATCH] mm/hugetlb: sort out global lock annotations

[Mateusz Guzik]

On Wed, Aug 28, 2024 at 10:44 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Wed, 28 Aug 2024 22:13:49 +0200 Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> > > It's conventional (within MM, at least) to put the section thing at the
> > > end of the definition, so tweak:
> > >
> > > --- a/mm/hugetlb.c~mm-hugetlb-sort-out-global-lock-annotations-fix
> > > +++ a/mm/hugetlb.c
> > > @@ -72,14 +72,14 @@ static unsigned int default_hugepages_in
> > >   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
> > >   * free_huge_pages, and surplus_huge_pages.
> > >   */
> > > -__cacheline_aligned_in_smp DEFINE_SPINLOCK(hugetlb_lock);
> > > +DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
> > >
> >
> > I tried things in this order and this does not compile for me:
> > In file included from ./arch/x86/include/asm/current.h:10,
> >                  from ./arch/x86/include/asm/preempt.h:7,
> >                  from ./include/linux/preempt.h:79,
> >                  from ./include/linux/spinlock.h:56,
> >                  from ./include/linux/mmzone.h:8,
> >                  from ./include/linux/gfp.h:7,
> >                  from ./include/linux/mm.h:7,
> >                  from mm/hugetlb.c:8:
> > ./include/linux/cache.h:80:3: error: expected ‘,’ or ‘;’ before ‘__attribute__’
> >    80 |   __attribute__((__aligned__(SMP_CACHE_BYTES),                  \
> >       |   ^~~~~~~~~~~~~
> > ./include/linux/cache.h:86:36: note: in expansion of macro ‘__cacheline_aligned’
> >    86 | #define __cacheline_aligned_in_smp __cacheline_aligned
> >       |                                    ^~~~~~~~~~~~~~~~~~~
> > mm/hugetlb.c:75:31: note: in expansion of macro ‘__cacheline_aligned_in_smp’
> >    75 | DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
> >       |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Well that's annoying.  It's because DEFINE_SPINLOCK includes an initializer.
>
> --- a/mm/hugetlb.c~mm-hugetlb-sort-out-global-lock-annotations-fix-fix
> +++ a/mm/hugetlb.c
> @@ -72,7 +72,7 @@ static unsigned int default_hugepages_in
>   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
>   * free_huge_pages, and surplus_huge_pages.
>   */
> -DEFINE_SPINLOCK(hugetlb_lock) __cacheline_aligned_in_smp;
> +spinlock_t hugetlb_lock __cacheline_aligned_in_smp = __SPIN_LOCK_UNLOCKED(hugetlb_lock);
>
>  /*
>   * Serializes faults on the same logical page.  This is used to
> _
>
> We'd need a new DEFINE_SPINLOCK_ALIGNED() or something.
>
> Ho hum, I'll fix.

that would be a nice addition

so as is this triviality grew to 3 patches which I consider rather
extreme, and the middle one breaks the build

In the vfs land this would get squashed into one commit with a
maintainer note that some tweaking was performed, which I would
suggest here

alternatively, given the trivial nature of the entire thing, if you
add DEFINE_SPINLOCK_ALIGNED and do the annotation tweak, you may as
well commit this as your own patch. I don't need any credit

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] mm/hugetlb: sort out global lock annotations

[Andrew Morton]

On Wed, 28 Aug 2024 23:02:39 +0200 Mateusz Guzik <mjguzik@gmail.com> wrote:

> > We'd need a new DEFINE_SPINLOCK_ALIGNED() or something.
> >
> > Ho hum, I'll fix.
> 
> that would be a nice addition
> 
> so as is this triviality grew to 3 patches which I consider rather
> extreme, and the middle one breaks the build
> 
> In the vfs land this would get squashed into one commit with a
> maintainer note that some tweaking was performed, which I would
> suggest here

Yep.  In mm land they get piled up as base+fix+fix-2 etc and then
squashed before being moved into mm.git's non-rebasing mm-stable
branch.

> alternatively, given the trivial nature of the entire thing, if you
> add DEFINE_SPINLOCK_ALIGNED and do the annotation tweak, you may as
> well commit this as your own patch. I don't need any credit

eh, it's very minor.