schedulers: store references to slots in processes array instead of AppIds #2068
Conversation
This file contains 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
bradjc
approved these changes
Aug 5, 2020
There was a problem hiding this comment.
I'm really glad to see that there is a reasonable fix which doesn't require changing the scheduler trait.
And from working on restartable apps before, I think building something that can handle every combination of dynamically added/removed/moved apps that we can dream up now is too complex, and instead we should implement based on the current usage of the PROCESSES array.
|
Last call as I want to merge this quickly (it really is just a bug fix for the scheduler PR). |
|
bors r+ |
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.
Pull Request Overview
This pull request fixes #2063 , by modifying each scheduler that stored a local copy of an AppId to instead store a pointer to a slot in the processes array. It also modifies initialization of these lists such that each scheduler has a list of all slots in the process array, and simply skips empty slots when scheduling. This makes restartable apps work again, and is also a first step towards supporting dynamically loaded apps (I imagine that will also involve replacing
Option<&'static dyn ProcessType>withOptionalCell<&'static dyn ProcessType>).This PR also changes the components for the mlfq and cooperative scheduler to include improvements I had made to the round robin component.
One downside of this approach is that if a process moves to a new slot in the app array, the scheduler state associated with that process is lost (i.e. position in the round robin queue is changed, and for mlfq the tracker of total CPU time used by the process during the current scheduling period will be inaccurate until the next resurrection). For the current schedulers I think this is fine, as it seems reasonable that moving processes around in the array might reset some state temporarily. If in the future schedulers are added that need to track state for all-time, we may want to reconsider this.
Testing Strategy
This pull request was tested by running two apps with each scheduler on Imix, and verifying that process restarts work using the process console.
TODO or Help Wanted
I ended up choosing this design over alternatives due to its simplicity. Eventually, we may want to move to the Kernel owning a generic
ProcessCollectiontype, but doing this without adding additional trait-object indirection/overhead feels difficult.Documentation Updated
Formatting
make prepush.