MeteredExecutor: relax modifyState CAS from seq_cst to acq_rel - #2699
Open
VrihadS wants to merge 1 commit into
Open
MeteredExecutor: relax modifyState CAS from seq_cst to acq_rel#2699VrihadS wants to merge 1 commit into
VrihadS wants to merge 1 commit into
Conversation
state_'s CAS only needs to linearize its own bit-packed counters (which the CAS retry loop guarantees regardless of memory order) and to publish/acquire this thread's surrounding effects to whichever thread next observes the new value -- both satisfied by acq_rel. Nothing here depends on state_ taking part in a seq_cst total order: task transfer through queue_ and worker dispatch through kaInner_->add() already carry their own synchronization. seq_cst costs nothing extra on x86 (LOCK CMPXCHG is already a full barrier) but requires an additional trailing fence on acq_rel on weakly-ordered architectures like ARM. Adds RealThreadConcurrentStress, which hammers add() from real OS threads concurrently with pause()/resume(), to cover what the existing DeterministicSchedule-based PauseResumeStress test cannot: actual hardware memory ordering. Verified under ThreadSanitizer (20 passes x 8 producers x 5000 tasks) with identical results -- same 4 pre-existing, unrelated races in folly::hazptr/folly::Function -- against both this change and the original seq_cst code. Fixes facebook#2656
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
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.
Fixes #2656.
modifyState()'s CAS only needs to (a) linearizestate_'s own bit-packed counters — guaranteed by the CAS retry loop's coherence regardless of memory order — and (b) publish/acquire this thread's surrounding effects to whichever thread next observes the new value. Both are satisfied byacq_rel.Nothing in
MeteredExecutordepends onstate_taking part in aseq_csttotal order: task transfer throughqueue_and worker dispatch throughkaInner_->add()already carry their own synchronization independent ofstate_.pause()already uses a relaxedfetch_oron this same atomic, which is inconsistent with the class ever having relied on aseq_cst-wide ordering guarantee across all ofstate_'s mutations.seq_cstcosts nothing extra here on x86 (LOCK CMPXCHGis already a full barrier), butacq_relis measurably cheaper on weakly-ordered architectures (ARM, POWER) whereseq_cstRMW requires an additional trailing fence.Testing: added
RealThreadConcurrentStress, which drivesadd()from real OS threads concurrently withpause()/resume()— the existingPauseResumeStresstest only exercises interleavings viaDeterministicSchedule's mocked atomics and doesn't exercise actual hardware memory ordering. Verified under ThreadSanitizer (20 passes x 8 producers x 5,000 tasks = 800k tasks) with this change and, as a control, against the originalseq_cstcode: identical results both ways — full correctness, and the same 4 TSan-flagged races in unrelatedfolly::hazptr/folly::Functionmachinery, confirming they're pre-existing and unaffected by this change.