Add StoreLoad barriers to ObjectMonitor::try_spin() for ARM64 Dekker …#50
Open
macarte wants to merge 1 commit intomacarte/PR1-winarm64from
Open
Add StoreLoad barriers to ObjectMonitor::try_spin() for ARM64 Dekker …#50macarte wants to merge 1 commit intomacarte/PR1-winarm64from
macarte wants to merge 1 commit intomacarte/PR1-winarm64from
Conversation
…protocol The Dekker protocol between try_spin() (ST _succ -> LD _owner) and exit() requires a StoreLoad barrier on both sides. The exit() side already has one (release_clear_owner + OrderAccess::storeload), but the spinner side was missing the corresponding fence. On ARM64, volatile store (STLR) followed by volatile load (LDAR) to different addresses does NOT imply StoreLoad ordering. Without the explicit barrier, the CPU can reorder the _owner load before the _succ store, causing the exiter to miss the successor designation while the spinner misses the lock release — leading to missed wakeups and thread starvation. Insert OrderAccess::storeload() after set_successor(current) in both places in try_spin(): before the spin loop and at the end of each iteration.
Author
|
baseline results |
Author
|
manually running sanity checks: https://github.com/microsoft/openjdk-jdk/actions/runs/22914389984 |
swesonga
reviewed
Mar 10, 2026
| // the _owner load before the _succ store. On ARM64 with MSVC | ||
| // /volatile:iso, Atomic::store/load are plain STR/LDR with no | ||
| // barrier, so without this fence the Dekker protocol is broken and | ||
| // the exiter may not see our successor designation while we may not |
Member
There was a problem hiding this comment.
Is this fence still needed now that Atomic::store/load are no longer plain STR/LDRs?
Member
There was a problem hiding this comment.
Is this fence still needed now that Atomic::store/load are no longer plain STR/LDRs?
One reason this might not be needed is the explanation in https://github.com/openjdk/jdk/blob/9d4fbbe36d85d71ce850bb83bbfb1ce1d3e8dd23/src/hotspot/share/runtime/objectMonitor.cpp#L1586 - "the try_set_owner_from() below uses cmpxchg() so we get the fence down there." (this would be line 2492 in the right view of this file)
swesonga
reviewed
Mar 10, 2026
| // Here on the spinner's side, we need a StoreLoad barrier between | ||
| // setting _succ and reading _owner to prevent the CPU from reordering | ||
| // the _owner load before the _succ store. On ARM64 with MSVC | ||
| // /volatile:iso, Atomic::store/load are plain STR/LDR with no |
Member
There was a problem hiding this comment.
This comment should probably be reworded to remove the statement that /volatile:iso is in use if we intend to switch to /volatile:ms
Author
|
manual run results: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add StoreLoad barriers to ObjectMonitor::try_spin() for ARM64 Dekker protocol
The Dekker protocol between try_spin() (ST _succ -> LD _owner) and exit() requires a StoreLoad barrier on both sides. The exit() side already has one (release_clear_owner + OrderAccess::storeload), but the spinner side was missing the corresponding fence.
On ARM64, volatile store (STLR) followed by volatile load (LDAR) to different addresses does NOT imply StoreLoad ordering. Without the explicit barrier, the CPU can reorder the _owner load before the _succ store, causing the exiter to miss the successor designation while the spinner misses the lock release — leading to missed wakeups and thread starvation.
Insert OrderAccess::storeload() after set_successor(current) in both places in try_spin(): before the spin loop and at the end of each iteration.