Merged
Conversation
7b8ff79 to
a08e75b
Compare
Contributor
stevenengler
left a comment
There was a problem hiding this comment.
I still want to look at this a little more, but figured I might as well leave the comments I have so far.
stevenengler
approved these changes
Feb 27, 2023
d5b387d to
e51a34b
Compare
e51a34b to
2bc0e12
Compare
Contributor
Author
|
Thanks for the careful review! |
We're going to need to port additional synchronization primitives suitable for use in shared memory to Rust. I think conceptually it makes sense to put these in a crate together separate from the shared memory allocator, which isn't as closely related.
We'll want to reuse these for additional modules in this crate.
This is a VirtualAddressSpaceIndependent implementation of a channel. The intent is to use this to replace `BinarySpinningSem` and `IPCData` to communicate with managed threads from ManagedThread in Shadow.
The previous implementation had potential undefined behavior due to optimistically updating state without checking the previous state. We do away with the `fetch_add` and `fetch_sub` tricks here (with sanity checks after the fact), and instead use e.g. `compare_exchange`. This results in additional atomic operations in the "happy" path, but is easier to understand, especially in failure cases.
This is more general and more closely aligned with `std::mpsc::channel` terminology.
2bc0e12 to
0f16c48
Compare
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.
We need a vasi (
VirtualAddressSpaceIndependent) wrapper or reimplementation ofIPCData(essentially a pair of fixed-size channels) for communication between shadow and the shim before migratingManagedThreadto Rust.This PR:
SelfContainedMutexinto a new crate, where we can putIPCDataand any other vasi sync primitives we end up needing to implement.SelfContainedMutexSelfContainedChanneland corresponding loom-compatible tests. It should be simple to replaceIPCDatawith a pair of these.