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

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: <CAGudoHH3hFu=7_ouHz+HY8djBxZj3-Zw30gE-F0H_2t1=sv_0A@mail.gmail.com>
Date: Wed, 28 Aug 2024 23:02:39 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: muchun.song@...ux.dev, dave@...olabs.net, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org
Subject: Re: [PATCH] mm/hugetlb: sort out global lock annotations

On Wed, Aug 28, 2024 at 10:44 PM Andrew Morton
<akpm@...ux-foundation.org> wrote:
>
> On Wed, 28 Aug 2024 22:13:49 +0200 Mateusz Guzik <mjguzik@...il.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>