Draft
Conversation
Cherry-picked from e4fafb13b16b6b573a973ec163b466a1c96f3a21 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cherry-picked from 5b9106431611ef8bfc324bac1368498fd821c8ae Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use shared references instead of copied objects on some functions that don't necessarily require ownership of `FutexKey`. Remove the `Copy` derivation of `FutexKey` to discourage suboptimal copies.
Doing `addr / self.size()` before masking with `(self.size() - 1)` removes the low bits entirely, which causes adjacent addresses (modulo `self.size()`) to map to the same bucket, entailing bad load balance. This patch solves that. Further, make `FutexBucketVec::new` and `FutexBucketVec::get_bucket` private, as they only make sense within the scope of `futex.rs`, where the invariant of `size` being a power of two is guaranteed to hold via `get_bucket_count` (which is also private).
This patch pays the price of making the instantiation of `FutexKey` more expensive to achieve two goals: * Minor: make `match_up` slightly faster * Major: make futex bucket allocation balancing more robust
Replace `VmWriter::atomic_update` with `VmWriter::atomic_compare_exchange`, which takes the old value for comparison and new value instead of a closure to compute it. This version has one less unsafe call. Then use `atomic_compare_exchange` to reimplement the looping logic of `wake_robust_futex` and make it atomic.
If the futex wait operation was interrupted by a signal or timed out, the `FutexItem` must be dequeued and dropped. Otherwise, malicious user programs could repeatedly issue futex wait operations to exhaust kernel memory. Due to asynchronicity, this removal can't be done by queue position nor by futex key match up: * The position might have changed during the pause as some earlier futex might have been dequeued * If two futexes with the same key are enqueued and then one of them times out or is interrupted, a removal by key would likely dequeue the wrong futex Therefore, we need to perform a removal by unique global futex ID.
* Improve the phrasing of some docstrings and comments * Add warning comments about attempts to validate memory addresses at reader/writer instantiation time * Create the `reader_writer` method for ergonomically instantiate a reader/writer pair covering the same memory region. This method is also slightly more efficient than calling `reader` and `writer` separately * Clean up `check_vaddr` for clarity and rename it to `check_vaddr_lowerbound` for explicity * Include the data length check before calling `check_vaddr_lowerbound` in `atomic_load` and `atomic_fetch_update` for further consistency with the delayed buffer validation
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
7 participants
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.
This serves as an example of cherry-picking a required feature from upstream. We can use this to explore the process and see what we would need to do to audit the code correctly.
This took me about a day of work I think. This would be less with a little more experience, but more if I hit some of the code that is harder to work with (see below).
The overall workflow I used is:
sort-cherry-pick-commits(Yep, more AI).This worked OK for this case, but there was a notable commit that should have been included, but I skipped asterinas/asterinas@55ad5d9. I had to skip it (and the fixes it provided) because it relies on the VM refactor that happened upstream. There are several refactors, and some are not just moving things around, but really change the structure of things, making code non-portable between them. In fact, this skipped commit itself is complex enough that future changes to the Futex implementation will be difficult to cherry-pick.