Repository navigation
Conversation
simple spinlock time: 21 ns ticked spinlock time: 35 ns ticked spinlock time(opt): 29 ns Assisted-By: mimo-v2.6-pro Signed-off-by: hujun5 <hujun5@xiaomi.com>
af4d82e to
b835f04
Compare
github-actions
Bot
added
Area: Build system
Size: L
b835f04 to
21f29a0
Compare
|
21f29a0 to
2eabe5e
Compare
531f176 to
5d57a8b
Compare
github-actions
Bot
added
Area: OS Components
This commit added compiler_assume hint for rspinlock to do more aggresive optimization. Assisted-By: mimo-v2.6-pro Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
RSPINLOCK_COUNT(old_val + 1u) expands to ((uint32_t)(old_val + 1U) & 0xffffU) <= 0xffff which is always true, making the DEBUGASSERT ineffective. Change to RSPINLOCK_COUNT(old_val) + 1u so the check correctly validates that the 16-bit count field will not overflow when incremented. Assisted-By: mimo-v2.6-pro Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
Spinlock helpers (spin_lock_notrace, rspin_lock_notrace, rspin_trylock, rspin_unlock_notrace, read_lock, read_trylock, write_lock) are always force-inlined today, which bloats every call site on small targets. disabling it emits a single callable copy of each helper from irq_spinlock.c, trading a function-call overhead for reduced code size. When CONFIG_SPINLOCK_INLINE is disabled, irq_spinlock.c defines SPINLOCK_INLINE as empty and forces CONFIG_SPINLOCK_INLINE on, causing the header bodies to compile as ordinary (non-static-inline) definitions. Additionally: - Inline read_lock_irqsave/read_unlock_irqrestore/write_lock_irqsave/ write_unlock_irqrestore in the header (trivial wrappers, previously extern in irq_spinlock.c) to avoid call overhead for simple irq_save + lock sequences. - Fix rspin_lock_is_hold to only save/restore IRQs under CONFIG_SPINLOCK, avoiding unnecessary irq_save on UP builds. Size reduction: 1400 bytes on a TriCore flat build with the option disabled. Assisted-By: mimo-v2.6-pro Signed-off-by: hujun5 <hujun5@xiaomi.com>
rspin_trylock() has three branches:
1. Lock owned by this CPU -> ret = true
2. Lock free (old_val == 0) -> ret = CAS result
3. Lock owned by other CPU -> ret is uninitialized
Branch 3 is reachable when another CPU holds the lock, but ret
is never assigned in this path. This causes undefined behavior
and a compile error with clang -Wsometimes-uninitialized.
The bug was introduced in commit 7133388 ("spinlock: Improve
the performance of rspinlock") which refactored rspin_trylock and
changed "bool ret = false" to "bool ret", losing the default
initialization. The paired function rspin_unlock() in the same
commit correctly kept "bool ret = false", confirming this was an
oversight rather than a deliberate optimization.
Fix by restoring the "= false" initializer, which is the correct
return value for trylock failure (lock held by another CPU).
Assisted-By: mimo-v2.6-pro
Signed-off-by: pengxianghao <pengxianghao@xiaomi.com>
Add ARM Wait For Event (WFE) and Send Event (SEV) instructions to spinlock implementation to reduce CPU contention and prevent livelock issues: WFE/SEV semantics: - For CAS-based locks, WFE is called only when lock is actually held, not on STREX failure (which can happen due to contention even when lock is free). - SEV is called after releasing lock to wake waiting CPUs from WFE state - DSB is added before SEV to ensure memory is written This optimization reduces CPU utilization and power consumption during lock contention while maintaining backward compatibility. Assisted-By: mimo-v2.6-pro Signed-off-by: xuxingliang <xuxingliang@xiaomi.com>
85f2052 to
e02fce1
Compare
UP_SEV() is empty on RISC-V, x86_64, Xtensa and sim, but UP_DSB() is fence iorw,iorw on RISC-V and mfence on x86_64, so every spinlock release paid a full memory barrier that only the ARM WFE/SEV lock sequence needs. Move the DSB into the UP_SEV() definitions of the ARM profiles whose WFE waiters rely on SEV (armv7-a, armv7-r, armv8-r, the generic ARM fallback used by Cortex-M, and arm64) and drop the unconditional UP_DSB() calls from the spinlock release paths. SEV is not a memory access and is not subject to memory ordering, so a waiter may observe the event before the lock release store becomes visible, re-read the stale lock and sleep again with the event already consumed (missed wakeup). DSB makes the store visible before SEV. Exception: a waiter sleeping with an exclusive monitor armed wakes on the monitor clear from the store itself and needs neither SEV nor DSB. See "Use of Wait For Event (WFE) and Send Event (SEV) with Locks" in the Barrier Litmus Tests appendix of ARM DDI 0406C: https://support.arm.com/documentation/ddi0406/c/Appendices/Barrier-Litmus-Tests/Exclusive-accesses-and-barriers/Use-of-Wait-For-Event--WFE--and-Send-Event--SEV--with-Locks?lang=en Assisted-By: mimo-v2.6-pro Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
e02fce1 to
93e055d
Compare
github-actions
Bot
added
Arch: arm64
WFE and SEV are baseline instructions in ARMv7-A/R and ARMv8-R. ARCH_ARMV7A already selects ARM_HAVE_WFE_SEV, but ARCH_ARMV7R and ARCH_ARMV8R did not, so the spinlock WFE/SEV waiting was inactive there and UP_WFE() fell back to an empty macro. Select ARM_HAVE_WFE_SEV for ARCH_ARMV7R and ARCH_ARMV8R as well, so the common UP_WFE()/UP_SEV() definitions in arch/arm/include/arch.h cover all of these profiles, and remove the per-profile UP_SEV() overrides from the armv7-a/armv7-r/armv8-r barriers.h along with the now unused arm_sev() helpers. This leaves a single UP_SEV() definition per arch, keeping the required DSB before SEV. Assisted-By: mimo-v2.6-pro Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
github-actions
Bot
added
Size: M
ef5c1dc to
c4e3890
Compare
casaroli
left a comment
casaroli
left a comment
There was a problem hiding this comment.
Speed note, not a blocker. ostest spinlock average throughput, op/s, on qemu-armv8a:nsh_smp with 4 CPUs under HVF on an Apple core, one run each:
| threads, lock | master | this PR merged with master |
|---|---|---|
| 1, spinlock | 139 M | 17.3 M |
| 1, rspinlock | 108 M | 17.1 M |
| 2, spinlock | 33.4 M | 5.7 M |
| 4, spinlock | 11.4 M | 2.4 M |
| 4, rspinlock | 8.7 M | 2.9 M |
The host was running other builds during these runs, so this comparison is inconclusive. On arm64 the unlock now has DSB and SEV, which master does not have. A measurement on an idle host or on real arm64 hardware is needed to say what this costs.
casaroli
left a comment
casaroli
left a comment
There was a problem hiding this comment.
We have concerns about the performance of this change: on arm64 every unlock now runs DSB and SEV, and no option turns them off. Our numbers above are inconclusive, so we will measure first, on an idle host, before we approve.
casaroli
left a comment
casaroli
left a comment
There was a problem hiding this comment.
Measured on a quiet host: qemu-armv8a:nsh_smp, 4 CPUs, HVF on an Apple core, ostest spinlock test, median of 3 runs, M op/s. This replaces my earlier numbers.
| threads, lock | master | this PR | no WFE/SEV in spinlock.h | SEV without DSB |
|---|---|---|---|---|
| 1, spinlock | 155.9 | 11.3 | 155.2 | 115.8 |
| 1, rspinlock | 153.6 | 11.8 | 155.7 | 114.2 |
| 2, spinlock | 48.8 | 8.9 | 44.3 | 40.4 |
| 2, rspinlock | 49.5 | 9.0 | 41.1 | 30.9 |
| 3, spinlock | 30.6 | 4.8 | 29.3 | 21.9 |
| 3, rspinlock | 28.7 | 5.1 | 25.1 | 18.1 |
| 4, spinlock | 26.9 | 3.5 | 24.6 | 18.5 |
| 4, rspinlock | 21.7 | 4.0 | 22.0 | 13.5 |
The last column is for measurement only; it is not correct code. Most of the cost is the DSB before SEV in every unlock. This is one platform and a lock/unlock loop.
Fix: on arm64, wait with the exclusive monitor armed, the exception that 93e055d describes, so that the unlock needs neither SEV nor DSB; or make WFE/SEV an option, off by default on arm64.
Thanks for the numbers — they make it clear the wait policy is a platform trade-off and must be opt-in. We replaced the unconditional WFE/SEV with a configurable lock wait policy, in two new commits:
This is essentially your "make WFE/SEV an option, off by default on arm64", generalized. On arm64 the default NOPWAIT restores the master unlock path (plain release store, no DSB/SEV), so the regression in your table is gone — the "no WFE/SEV in spinlock.h" column is what to expect. On Intel 64, WFESEV is only PAUSE in the wait loop with an empty wake event, so unlock stays free; on Arm v7-A the DSB before SEV remains folded into |
github-actions
Bot
added
Arch: x86_64
The WFE/SEV lock waiting made every unlock pay for a DSB before SEV, a full barrier on the unlock hot path (measured 155.9 -> 11.3 M lock/unlock ops/s single-threaded on qemu-armv8a), and no platform wants that cost unconditionally: armv7a and SMP Cortex-M prefer the power saving, Intel 64 only wants PAUSE in the wait loop, and arm64, RISC-V and realtime profiles prefer the raw throughput. Add a choice between busy wait (NOPWAIT) and WFE/SEV wait (WFESEV) with SPINLOCK_WAIT()/SPINLOCK_WAKE() as the single policy point. The choice defaults to NOPWAIT; each architecture declares where WFE/SEV waiting pays off by selecting SPINLOCK_WFESEV_DEFAULT when SMP (armv7-a, Intel 64 and the M-profiles). This commit adds the configuration and the macros only; the lock paths switch to them in the next commit. Assisted-By: mimo-v2.6-pro Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
Replace the direct UP_WFE()/UP_SEV() calls in the spinlock paths with the policy macros. Under the default NOPWAIT policy the unlock path becomes a plain release store again on arm64, RISC-V, Xtensa, sim and the realtime profiles, restoring master throughput; armv7a, Intel 64 (PAUSE only) and SMP Cortex-M keep the WFE/SEV waiting via WFESEV. Assisted-By: mimo-v2.6-pro Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
2a2c427 to
758f22e
Compare
Note: Please adhere to Contributing Guidelines.
Summary
This PR improves spinlock/rspinlock performance and adds inline control:
ticked_spinlock: Improve the performance of ticket spinlock— optimize ticket spinlock performance.spinlock: Improve the performance of rspinlock— restructure rspinlock withval/local_valfields andRSPINLOCK_*macros.spinlock: Add compiler_assume for rspinlock— add compiler hint optimization.spinlock: Fix Coverity CONSTANT_EXPRESSION_RESULT in rspin_lock— Coverity fix.spinlock: add CONFIG_SPINLOCK_INLINE to control inlining— addCONFIG_SPINLOCK_INLINEoption to control inline vs non-inline helpers.spinlock: fix uninitialized ret in rspin_trylock— fix uninitialized return value.spinlock: Add WFE/SEV optimization to prevent livelock— addUP_WFE()/UP_SEV()optimization to prevent livelock.Impact
CONFIG_SPINLOCK_INLINEKconfig option (default!DEFAULT_SMALL).UP_WFE()/UP_SEV()(ARM/RISC-V low-power wait/event instructions) to prevent livelock; removes redundantUP_DSB()calls.local_val(local copy) to avoid atomic read/write; ticket spinlock optimized.irq_save()/irq_restore()wrapper replacesup_irq_save()/up_irq_restore()calls (behavior unchanged).Testing
rv-virt:smpmake) + QEMU ostest passed