| Message ID | 20240731072405.197046-3-alexghiti@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Zacas/Zabha support and qspinlocks | expand |
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. 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. > > 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@rivosinc.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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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. > 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@rivosinc.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@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
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
On 7/31/24 03:23, 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. Note that the native qspinlock code only need halfword atomic cmpxchg operation. However, if you also plan to use paravirtual qspinlock, you need to have byte-level atomic cmpxchg(). Cheers, Longman > > Then instead of failing to build, just fallback to the !Zawrs path. > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.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;
On Thu, Aug 1, 2024 at 1:27 AM Waiman Long <longman@redhat.com> wrote: > > On 7/31/24 03:23, 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. > > Note that the native qspinlock code only need halfword atomic cmpxchg > operation. However, if you also plan to use paravirtual qspinlock, you > need to have byte-level atomic cmpxchg(). Thx for reminding me; I will update paravirt qspinlock after these patches merge. Zabha & Zcas extension provides byte and half-word atomic cmpxchg. > > Cheers, > Longman > > > > > Then instead of failing to build, just fallback to the !Zawrs path. > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.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; >
On Wed, Jul 31, 2024 at 6:14 PM Andrew Jones <ajones@ventanamicro.com> 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@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()? Yes, sure, I'll do that. Thanks, Alex > > Thanks, > drew
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;
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. Then instead of failing to build, just fallback to the !Zawrs path. Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> --- arch/riscv/include/asm/cmpxchg.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)