casaroli commented on code in PR #21342:
URL: https://github.com/apache/nuttx/pull/21342#discussion_r4441922793
##########
include/nuttx/spinlock.h:
##########
@@ -314,10 +334,11 @@ static inline_function void
spin_unlock_notrace(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_TICKET_SPINLOCK
- atomic_add_release(&lock->owner, 1);
+ atomic_set_release(&lock->owner, lock->owner + 1);
#else
atomic_set_release((FAR atomic_t *)lock, SP_UNLOCKED);
#endif
+ UP_SEV();
Review Comment:
SEV is not a memory access, and a store-release does not make the store
visible before the next instruction. Without a DSB the SEV can arrive before
the unlock store is visible: the waiter reads "locked" again, executes WFE and
sleeps with the lock free, usually with IRQs masked. The ARM ARM WFE/SEV lock
example has a DSB between the store and SEV, and commit 8ea5310 added it for
this reason.
Fix: drop commit 3d4225e and keep `UP_DSB()` before every `UP_SEV()` (lines
341, 932, 1171, 1280).
##########
include/nuttx/spinlock.h:
##########
@@ -849,13 +923,18 @@ void spin_unlock_irqrestore_nopreempt(FAR volatile
spinlock_t *lock,
static inline_function
bool rspin_unlock(FAR rspinlock_t *lock)
{
- DEBUGASSERT(lock->owner == this_cpu() + 1);
- DEBUGASSERT(lock->count >= 1);
+ bool ret = false;
+ uint32_t val = lock->local_val;
- if (--lock->count == 0)
+ if (RSPINLOCK_COUNT(val) == 1u)
{
- atomic_set_release((FAR atomic_t *)&lock->val, 0);
- return true;
+ atomic_set_release(&lock->val, 0u);
+ UP_SEV();
+ ret = true;
+ }
+ else
+ {
+ lock->local_val = val - 1u;
}
return false;
Review Comment:
`rspin_unlock()` sets `ret = true` on the last unlock but returns `false`.
So `rspin_unlock_irqrestore()` never calls `irq_restore()`, and the CPU keeps
interrupts disabled after every rspinlock section. With this PR
qemu-armv8a:knsh with 4 CPUs stops at boot, and CI arm64-01 fails
`qemu-armv8a/citest` with "device boot timeout": `clock_initialize()` calls
`clock_notifier_call_chain()`, which uses an rspinlock (master boots).
Fix: `return ret;`.
##########
include/nuttx/spinlock_type.h:
##########
@@ -75,16 +76,19 @@ typedef uint32_t spinlock_t;
typedef union rspinlock_u
{
- /* Which cpu is holding spinlock, and taking recursive count */
-
- uint32_t val;
struct
Review Comment:
`val` and `local_val` are two members of a struct, so they are two words
(`sizeof(rspinlock_t)` is 8, `local_val` is at offset 4), but the code expects
one word (the comment and `compiler_assume()` in `rspin_lock()` and
`rspin_trylock()`). After the CAS `local_val` keeps its old value, so
`rspin_unlock()` sees count 0 and never clears `val`: the lock is never
released. The new layout also breaks `RSPINLOCK_INITIALIZER` users: CI
xtensa-01, xtensa-02 and risc-v-02 fail in esp-hal-3rdparty with
`-Werror=missing-braces` (sleep_modes.c:318, gpio.c:73, esp_timer.c:92).
Fix: put `atomic_t val` and `uint32_t local_val` directly in the union,
without the inner struct.
##########
include/nuttx/irq.h:
##########
@@ -136,6 +136,20 @@
#define IRQ_WAKE_THREAD 1
+/****************************************************************************
Review Comment:
`irq_save()`/`irq_restore()` are plain aliases of
`up_irq_save()`/`up_irq_restore()` with no instrumentation, and only some of
the ~268 files that call `up_irq_save()` change. The rename is not part of the
spinlock change and brings 26 extra files into this PR.
Fix: move this commit (and its nxstyle commits) to a separate PR, together
with the instrumentation that needs it.
##########
include/nuttx/spinlock.h:
##########
@@ -608,40 +662,60 @@ irqstate_t rspin_lock_irqsave_nopreempt(FAR rspinlock_t
*lock)
****************************************************************************/
#ifdef CONFIG_SPINLOCK
-static inline_function bool rspin_trylock(FAR rspinlock_t *lock)
+# ifdef CONFIG_SPINLOCK_INLINE
+SPINLOCK_INLINE bool rspin_trylock(FAR rspinlock_t *lock)
{
- rspinlock_t new_val;
- rspinlock_t old_val = RSPINLOCK_INITIALIZER;
- int cpu = this_cpu() + 1;
+ uint32_t old_val;
+ bool ret = false;
+ uint32_t owner = (uint32_t)this_cpu() + 1u;
/* Already owned this lock. */
- if (lock->owner == cpu)
+ old_val = atomic_read(&lock->val);
+
+ if (RSPINLOCK_OWNER(old_val) == owner)
{
- lock->count += 1;
- return true;
+ DEBUGASSERT(RSPINLOCK_COUNT(old_val) + 1u <= UINT16_MAX);
+
+ lock->local_val = old_val + 1u;
+ ret = true;
}
+ else if (old_val == 0u)
+ {
+ /* Try seize the ownership of the lock using CAS. */
+
+ ret = atomic_cmpxchg_acquire(&lock->val, &old_val,
+ RSPINLOCK_VALUE(owner, 1u));
- new_val.count = 1;
- new_val.owner = cpu;
+ /* Mark locked when the lock is successfully acquired. */
- /* Try seize the ownership of the lock. */
+ if (ret)
+ {
+ /* The local value shares the same memory location with the
+ * global atomic value. So if the atomic operation success,
+ * the local value should also change to the new_val.
+ */
- return atomic_cmpxchg_acquire((FAR atomic_t *)&lock->val,
- (FAR atomic_t *)&old_val.val, new_val.val);
+ compiler_assume(lock->local_val == RSPINLOCK_VALUE(owner, 1u));
+ }
+ }
}
Review Comment:
`rspin_trylock()` has no `return` statement, so its result is undefined on
all three paths.
Fix: add `return ret;` before the closing brace.
##########
include/nuttx/spinlock.h:
##########
@@ -994,11 +1119,14 @@ static inline_function void read_lock(FAR volatile
rwlock_t *lock)
*
****************************************************************************/
-static inline_function bool read_trylock(FAR volatile rwlock_t *lock)
+# ifdef CONFIG_SPINLOCK_INLINE
+SPINLOCK_INLINE bool read_trylock(FAR volatile rwlock_t *lock)
{
- while (true)
+ bool ret = false;
Review Comment:
`bool ret = false;` is not used.
Fix: remove it.
##########
include/nuttx/spinlock.h:
##########
@@ -951,23 +1069,30 @@ void rspin_restorelock(FAR rspinlock_t *lock, uint16_t
count)
*
****************************************************************************/
-static inline_function void read_lock(FAR volatile rwlock_t *lock)
+# ifdef CONFIG_SPINLOCK_INLINE
+SPINLOCK_INLINE void read_lock(FAR volatile rwlock_t *lock)
{
nxsched_critmon_busywait(true, return_address(0));
-
- while (true)
+ for (; ; )
{
- int old = atomic_read(lock);
+ int32_t old = atomic_read(lock);
- if (old > RW_SP_WRITE_LOCKED &&
- atomic_cmpxchg_acquire(lock, &old, old + 1))
+ if (old > RW_SP_WRITE_LOCKED)
{
- break;
+ if (atomic_cmpxchg_acquire(lock, &old, old + 1))
+ {
+ break;
+ }
+
+ UP_WFE();
Review Comment:
This WFE also runs when the CAS fails only because another reader changed
the count. A read acquire sends no SEV, so this reader sleeps until the next
unlock although it could take the lock now.
Fix: call `UP_WFE()` only when `old == RW_SP_WRITE_LOCKED`, and retry the
CAS at once otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
