On Wed, Jul 31, 2024 at 6:14 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote: > > 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@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. > > 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()? Yes, sure, I'll do that. Thanks, Alex > > Thanks, > drew
