From f7121c7b7e1684113823ef23270fdef37a687aa0 Mon Sep 17 00:00:00 2001 From: Travis Geiselbrecht Date: Thu, 28 Dec 2023 16:34:34 -0800 Subject: [PATCH] [arch][riscv] general riscv spinlock cleanup -Hard set the spinlock type to uint32_t to be clearer -Switch the free/held value to a simple 0 or 1 Previously, was writing the current cpu number into the spinlock, which was only useful for debugging purposes. However, since the atomic operation was an amoswap instead of a proper CAS, it would end up overwriting the old cpu number with the new cpu number when it tried to take it. It would still work as a spinlock, since the value was !0, but it was falsely tracking which cpu actually held it. To keep it simple, just switch to 0/1 and stick with the amoswap instruction, which is much more efficient than a LR/SC pair to implement CAS on riscv. Internally still use an unsigned long for the old value, since the amoswap instruction overwrites the entire Rd register, and thus keeps the codegen efficient since it wont try to sign extend it for any comparisons afterwards. Thanks @loqoman (darwin.s.clark@gmail.com) for catching this one. --- arch/riscv/include/arch/spinlock.h | 2 +- arch/riscv/spinlock.c | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/arch/riscv/include/arch/spinlock.h b/arch/riscv/include/arch/spinlock.h index a73b79e79..22ed69049 100644 --- a/arch/riscv/include/arch/spinlock.h +++ b/arch/riscv/include/arch/spinlock.h @@ -15,7 +15,7 @@ __BEGIN_CDECLS #define SPIN_LOCK_INITIAL_VALUE (0) -typedef volatile unsigned int spin_lock_t; +typedef volatile uint32_t spin_lock_t; typedef unsigned long spin_lock_saved_state_t; typedef unsigned int spin_lock_save_flags_t; diff --git a/arch/riscv/spinlock.c b/arch/riscv/spinlock.c index 441f5fdc7..c12f8c3eb 100644 --- a/arch/riscv/spinlock.c +++ b/arch/riscv/spinlock.c @@ -7,17 +7,19 @@ */ #include +#include + // super simple spin lock implementation int riscv_spin_trylock(spin_lock_t *lock) { - unsigned long val = arch_curr_cpu_num() + 1UL; + // use a full 32/64 type since amoswap overwrites the entire register unsigned long old; __asm__ __volatile__( "amoswap.w %0, %2, %1\n" "fence r, rw\n" : "=r"(old), "+A"(*lock) - : "r" (val) + : "r" (1u) : "memory" ); @@ -25,19 +27,19 @@ int riscv_spin_trylock(spin_lock_t *lock) { } void riscv_spin_lock(spin_lock_t *lock) { - unsigned long val = arch_curr_cpu_num() + 1UL; - for (;;) { if (*lock) { + // TODO: use a yield instruction here? continue; } + // use a full 32/64 type since amoswap overwrites the entire register unsigned long old; __asm__ __volatile__( "amoswap.w %0, %2, %1\n" "fence r, rw\n" : "=r"(old), "+A"(*lock) - : "r" (val) + : "r" (1u) : "memory" );