Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SelfContainedChannel #2757

Merged
merged 8 commits into from
Feb 28, 2023
Merged

Add SelfContainedChannel #2757

merged 8 commits into from
Feb 28, 2023

Conversation

sporksmith
Copy link
Contributor

@sporksmith sporksmith commented Feb 21, 2023

We need a vasi (VirtualAddressSpaceIndependent) wrapper or reimplementation of IPCData (essentially a pair of fixed-size channels) for communication between shadow and the shim before migrating ManagedThread to Rust.

This PR:

  • moves SelfContainedMutex into a new crate, where we can put IPCData and any other vasi sync primitives we end up needing to implement.
  • Extracts some reusable loom abstractions from SelfContainedMutex
  • Adds SelfContainedChannel and corresponding loom-compatible tests. It should be simple to replace IPCData with a pair of these.

@github-actions github-actions bot added Component: Build Build/install tools and dependencies Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable labels Feb 21, 2023
@sporksmith sporksmith force-pushed the vasi-sync branch 6 times, most recently from 7b8ff79 to a08e75b Compare February 22, 2023 21:41
@sporksmith sporksmith changed the title Move SelfContainedMutex into a new crate vasi-sync Add SelfContainedChannel Feb 22, 2023
Copy link
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to look at this a little more, but figured I might as well leave the comments I have so far.

src/lib/vasi-sync/tests/scmutex-tests.rs Outdated Show resolved Hide resolved
src/lib/vasi-sync/src/sync.rs Outdated Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Outdated Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Outdated Show resolved Hide resolved
@stevenengler stevenengler self-requested a review February 24, 2023 22:16
src/lib/vasi-sync/src/scchannel.rs Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Outdated Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

src/lib/vasi-sync/src/scchannel.rs Outdated Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Show resolved Hide resolved
src/lib/vasi-sync/src/scchannel.rs Show resolved Hide resolved
@sporksmith
Copy link
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Build Build/install tools and dependencies Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants