On Wed, Jul 31, 2024 at 6:14 PM Andrew Jones [off-list ref] wrote:

On Wed, Jul 31, 2024 at 05:52:46PM GMT, Alexandre Ghiti wrote:

quoted

Hi Drew,

On Wed, Jul 31, 2024 at 4:10 PM Andrew Jones [off-list ref] wrote:

quoted

On Wed, Jul 31, 2024 at 09:23:54AM GMT, Alexandre Ghiti wrote:

quoted

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.

quoted

quoted

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

quoted

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