Re: [PATCH v4 02/13] riscv: Do not fail to build on byte/halfword operations with Zawrs

[Andrew Jones <ajones@ventanamicro.com>]

On Wed, Jul 31, 2024 at 05:52:46PM GMT, Alexandre Ghiti wrote:
> Hi Drew,
> 
> On Wed, Jul 31, 2024 at 4:10 PM Andrew Jones <ajones@ventanamicro.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.

Oh, I see.

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

Ah, missed that '&'...

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

OK, you've got me straightened out now, but can we only add the fallback
for sizes 1 and 2 and leave the default as a BUILD_BUG()?

Thanks,
drew