Hi Drew, On Wed, Jul 31, 2024 at 4:10 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxx > > http://lists.infradead.org/mailman/listinfo/linux-riscv
