Skip to content

Use memory_order_acquire for spinlock instead of acq_rel#169

Open
HFTrader wants to merge 1 commit intoefficient:masterfrom
HFTrader:fix-spinlock-memory-order
Open

Use memory_order_acquire for spinlock instead of acq_rel#169
HFTrader wants to merge 1 commit intoefficient:masterfrom
HFTrader:fix-spinlock-memory-order

Conversation

@HFTrader
Copy link
Copy Markdown

Summary

  • Use memory_order_acquire for lock() and try_lock() instead of memory_order_acq_rel
  • The standard spinlock pattern is acquire on lock, release on unlock. The release semantics on the lock path are unnecessary — when acquiring a lock, you need to ensure subsequent operations stay after the acquisition (acquire), but there is nothing to release at that point
  • This restores the original behavior from the initial commit (e57758e, Jan 2014). The change to acq_rel was introduced in 8530b8a (Dec 2016) with the rationale "since test-and-set is both a read and a write operation, it should [use acq_rel]" — however the "write" in test-and-set is the lock flag itself, which does not need release semantics during acquisition

Impact

  • x86: No change. test_and_set compiles to XCHG which is a full barrier regardless of the memory order specified
  • ARM/POWER: Removes an unnecessary barrier instruction. acq_rel generates both an acquire and release barrier, while acquire generates only the acquire barrier. This is a measurable improvement on weakly-ordered architectures

Context

This aligns with the three open PRs improving the spinlock (#147, #149, #158) and the discussion in issue #146.

Test plan

  • All existing unit tests pass
  • Consistent with the acquire/release pattern used by the existing unlock() method

The standard spinlock pattern is acquire on lock, release on unlock.
The release semantics on the lock path (from acq_rel) are unnecessary
— when acquiring a lock, you need to ensure subsequent operations
stay after the acquisition (acquire), but there is nothing to
release at that point.

This restores the original behavior from the initial commit
(e57758e, Jan 2014), which correctly used memory_order_acquire.
The change to acq_rel was introduced in 8530b8a (Dec 2016).

On x86 this is a no-op (test_and_set compiles to XCHG which is
a full barrier regardless). On ARM and POWER, acq_rel generates
an extra barrier instruction that acquire does not, making this
a measurable improvement on those architectures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant