bridge: fence SeqQuote's payload write inside the odd-version window (ibx#267) - #389
Open
userFRM wants to merge 1 commit into
Open
bridge: fence SeqQuote's payload write inside the odd-version window (ibx#267)#389userFRM wants to merge 1 commit into
userFRM wants to merge 1 commit into
Conversation
…ndow SeqQuote is a seqlock: the hot loop marks the version odd, copies the quote into the shared cell, then marks the version even; readers spin past an odd version and re-check it after copying the cell out, retrying on any change. The odd transition was a plain Release store. Release only orders what precedes it in the writing thread; it places no constraint on the payload write that follows, so nothing stopped that write from being reordered ahead of the version becoming odd. A reader landing in that gap could see a stable, even version while the quote underneath it was still being overwritten, which is exactly the guarantee the neighboring SAFETY comment claims the version counter provides. The odd transition is now version.fetch_add(1, Ordering::AcqRel). The Acquire half is the piece a plain Release store cannot express: it pins every access sequenced after it, including the payload write, so nothing can surface ahead of the marker going odd. The closing transition keeps Release, which already correctly holds the payload write behind it. Both transitions read as bare increments now, which also drops the intermediate relaxed load the two-store version needed to compute v+1 and v+2. seqquote_no_torn_reads stresses four reader threads against one writer across twenty thousand quotes, comparing every field of Quote against a single sentinel value per write rather than the two fields the existing concurrent test checks, so a torn read on any field trips it. x86 does not reorder a core's own stores or loads relative to each other, so the ordering this fixes is invisible to a stress test on this architecture under either version of the code; the defect is a property of the language's memory model rather than something an x86 run can be expected to surface. Closes deepentropy#267.
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
1 participant
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.
Problem
SeqQuoteis a seqlock: the writer makes the version odd, writes the payload, then makes it even, and a reader retries while the version is odd or changed under it. The odd-version transition used aReleasestore.Releaseorders what comes before the store. It places no constraint on what comes after, so nothing stopped the payload write being reordered above the store that was supposed to announce it — a reader could observe an even version and a payload already half-updated, with the retry loop none the wiser.What this changes
The odd transition is a
fetch_addwithAcqRel. The Acquire half is what pins the following payload write inside the odd window. The closing transition keepsRelease, which was already correct.What the test can and cannot show
seqquote_no_torn_readsruns four readers against one writer over twenty thousand quotes, comparing every field against a sentinel rather than the two the existing test checked. It is a genuine regression guard on the retry contract and strictly stronger than what was there.It does not fail when the ordering annotation alone is reverted, and no test on this target would. x86 does not reorder a core's own accesses relative to each other, so the defect is real by the Rust abstract machine — where the compiler is free to hoist the payload write — but not something an x86 execution can be expected to expose. Thirty million writes across twelve threads, in debug and release, showed no divergence between the two orderings.
This is stated rather than papered over: the guard here is the reasoning about the memory model, and the test protects the surrounding contract.
Separately
Miri reports a data race in this type regardless of how the version is ordered: the payload itself is read and written non-atomically, so a torn read is undefined before the version check can reject it. That needs per-field atomic or volatile access and is materially larger than this — filed as #388.
Closes #267.