Memory ordering fixes in StreamTracker.#329
Merged
Merged
Conversation
These bits are a publication mechanism. A set bit routinely means "the data this bit indexes is now valid": StreamTracker's slot payload, an allocated blk. But safe_bits set bits with fetch_or(relaxed) and read them with load(relaxed), so a thread that observes a set bit has no acquire edge to the data that bit names, and its read of that data is a data race. This is reachable today. StreamTracker::do_update() placement-news the entry and then sets its active bit, both under a SHARED lock -- and two shared_lock holders establish no ordering between them. LogDev::prepare_flush() walks foreach_contiguous_active() and reads log_record payloads from the flush thread while LogDev::append_async() concurrently create()s them; the appender holds m_stream_tracker_mtx shared, prepare_flush takes only m_flush_mtx, which appenders never take. Nothing orders the payload stores against the flusher's read. Benign on x86 -- fetch_or lowers to a locked RMW and TSO forbids store-store reordering -- but on aarch64 fetch_or(relaxed) is a bare ldxr/stxr with no barrier and store-store reordering is permitted, so the bit can become visible over a half-written entry. Give safe_bits the ordering its role requires: release on set/set_if/or_with/ and_with, acquire on get. On x86-64 the codegen is byte-for-byte identical (fetch_or is already a lock-prefixed RMW; an acquire load and a release store are plain movs), so this costs nothing on our primary target and is correct on weakly ordered ones. Why not std::atomic_thread_fence around the bit ops instead: it is equally correct and equally invisible to ThreadSanitizer, which does not model fences (compiler-rt's AtomicFence is a documented no-op for happens-before, and GCC warns -Wtsan). Fence-based publication makes TSAN report a false race on every payload scan. That gets suppressed, and the suppression then hides the real races. Verified against a concurrent reader/writer stress over StreamTracker: relaxed bits report 12 races, fences report the same 12, release/acquire reports none. Also drops the now-redundant ordering comments' fence references in stream_tracker.hpp and documents that the slot bit is what publishes the slot.
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.
No description provided.