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

[Mateusz Guzik <mjguzik@gmail.com>]

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>