Repository navigation
Parallel step races: allocation counters, the stepping thread's index, stage clocks - #103
Merged
Merged
Conversation
… index, the stage clocks Found by an audit of the parallel step for data races: - basics.alloc and free_bytes bumped the allocation and byte counters with plain read-modify-writes, and the collide pass and finalize allocate from several workers at once (a new mesh contact's triangle cache, an arena's overflow, the continuous pass's sensor hits): updates were lost and a multi-worker world's leak count drifted. They move atomically now. - A world's task contexts (one per worker) are chosen by the thread's worker index. parallel_for ran a one-worker range inline without setting it, so a host stepping a one-worker world from one of its pool's threads (index k > 0) indexed past the single context: heap corruption. A step now runs on the calling thread as worker 0 and gives the thread its index back at the end, and parallel_for does the same for its caller. - The stage clocks (AEPHYSICS_STAGES=1) were allocated lazily by whichever worker timed a block first and summed without atomics. Each worker has its own row now, made on the stepping thread and summed for the report. test_physics_world's test_parallel_bookkeeping: a four-worker world over a mesh frees everything it counted, and a step from a thread at index 5 runs and gives the index back (main crashes).
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.
I audited the parallel step for data races and found three real defects, fixed here:
basics.allocandfree_bytesupdated the allocation and byte counters with plain read-modify-writes. The collide pass and finalize allocate from several workers at once: a new mesh contact's triangle cache, an arena's overflow, the continuous pass's sensor hits. Updates were lost, so a multi-worker world's leak count drifted. The counters are atomic now.parallel_forran a one-worker range inline without setting that index. A host stepping a one-worker world from one of its own pool's threads (index k > 0) therefore indexed past the world's single task context and corrupted the heap. A step now runs on the calling thread as worker 0 and gives the thread its index back at the end;parallel_fordoes the same for its caller.AEPHYSICS_STAGES=1). Whichever worker timed a block first allocated them, and the sums weren't atomic. Each worker now has its own row, created on the stepping thread and summed for the report.test_physics_world'stest_parallel_bookkeepingcovers the first two:Not fixed: the audit also flagged the bullet pass reading another bullet's flags while that bullet sets bits in the same word. The flag being read never changes, and upstream does the same, so it's harmless in practice. It's recorded with the multi-world limits in the issue filed alongside this PR.
All 28 test programs pass locally on aether 0.772.0, with no warnings.