Skip to content

Commit

Permalink
[arch][riscv] general riscv spinlock cleanup
Browse files Browse the repository at this point in the history
-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 ([email protected]) for catching this one.
  • Loading branch information
travisg committed Dec 29, 2023
1 parent 6b16ef0 commit f7121c7
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 6 deletions.
2 changes: 1 addition & 1 deletion arch/riscv/include/arch/spinlock.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
12 changes: 7 additions & 5 deletions arch/riscv/spinlock.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,39 @@
*/
#include <arch/spinlock.h>

#include <stdint.h>

// 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"
);

return !old;
}

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"
);

Expand Down

0 comments on commit f7121c7

Please sign in to comment.