Message-ID: <CAHVXubjrrWVnw1ufXRJh_5N9M5UiOVZseb0C78fjLaYhNKF7eA@mail.gmail.com>
Date: Wed, 31 Jul 2024 17:52:46 +0200
From: Alexandre Ghiti <alexghiti@...osinc.com>
To: Andrew Jones <ajones@...tanamicro.com>
Cc: Jonathan Corbet <corbet@....net>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Conor Dooley <conor@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Andrea Parri <parri.andrea@...il.com>, 
	Nathan Chancellor <nathan@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>, 
	Arnd Bergmann <arnd@...db.de>, Leonardo Bras <leobras@...hat.com>, Guo Ren <guoren@...nel.org>, 
	linux-doc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-arch@...r.kernel.org
Subject: Re: [PATCH v4 02/13] riscv: Do not fail to build on byte/halfword
 operations with Zawrs

Hi Drew,

On Wed, Jul 31, 2024 at 4:10 PM Andrew Jones <ajones@...tanamicro.com> wrote:
>
> On Wed, Jul 31, 2024 at 09:23:54AM GMT, Alexandre Ghiti wrote:
> > riscv does not have lr instructions on byte and halfword but the
> > qspinlock implementation actually uses such atomics provided by the
> > Zabha extension, so those sizes are legitimate.
>
> We currently always come to __cmpwait() through smp_cond_load_relaxed()
> and queued_spin_lock_slowpath() adds another invocation.

atomic_cond_read_relaxed() and smp_cond_load_acquire() also call
smp_cond_load_relaxed()

And here https://elixir.bootlin.com/linux/v6.11-rc1/source/kernel/locking/qspinlock.c#L380,
the size passed is 1.

> However, isn't
> the reason we're hitting the BUILD_BUG() because the switch fails to find
> a case for 16, not because it fails to find cases for 1 or 2? The new
> invocation passes a pointer to a struct mcs_spinlock, which looks like
> it has size 16. We need to ensure that when ptr points to a pointer that
> we pass the size of uintptr_t.

I guess you're refering to this call here
https://elixir.bootlin.com/linux/v6.11-rc1/source/kernel/locking/qspinlock.c#L551,
but it's a pointer to a pointer, which will then pass a size 8.

And the build error that I get is the following:

In function '__cmpwait',
    inlined from 'queued_spin_lock_slowpath' at
../kernel/locking/qspinlock.c:380:3:
./../include/linux/compiler_types.h:510:45: error: call to
'__compiletime_assert_2' declared with attribute error: BUILD_BUG
failed
  510 |         _compiletime_assert(condition, msg,
__compiletime_assert_, __COUNTER__)
      |                                             ^
./../include/linux/compiler_types.h:491:25: note: in definition of
macro '__compiletime_assert'
  491 |                         prefix ## suffix();
         \
      |                         ^~~~~~
./../include/linux/compiler_types.h:510:9: note: in expansion of macro
'_compiletime_assert'
  510 |         _compiletime_assert(condition, msg,
__compiletime_assert_, __COUNTER__)
      |         ^~~~~~~~~~~~~~~~~~~
../include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
   39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
      |                                     ^~~~~~~~~~~~~~~~~~
../include/linux/build_bug.h:59:21: note: in expansion of macro
'BUILD_BUG_ON_MSG'
   59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
      |                     ^~~~~~~~~~~~~~~~
../arch/riscv/include/asm/cmpxchg.h:376:17: note: in expansion of
macro 'BUILD_BUG'
  376 |                 BUILD_BUG();

which points to the first smp_cond_load_relaxed() I mentioned above.

Thanks,

Alex


>
> >
> > Then instead of failing to build, just fallback to the !Zawrs path.
>
> No matter what sizes we're failing on, if we do this then
> queued_spin_lock_slowpath() won't be able to take advantage of Zawrs.
>
> Thanks,
> drew
>
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
> > ---
> >  arch/riscv/include/asm/cmpxchg.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> > index ebbce134917c..9ba497ea18a5 100644
> > --- a/arch/riscv/include/asm/cmpxchg.h
> > +++ b/arch/riscv/include/asm/cmpxchg.h
> > @@ -268,7 +268,8 @@ static __always_inline void __cmpwait(volatile void *ptr,
> >               break;
> >  #endif
> >       default:
> > -             BUILD_BUG();
> > +             /* RISC-V doesn't have lr instructions on byte and half-word. */
> > +             goto no_zawrs;
> >       }
> >
> >       return;
> > --
> > 2.39.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists