Add kipc::find_faulted_task. #1931
Merged
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
cbiffle
force-pushed
the
cbiffle/find-faulted-task
branch
2 times, most recently
from
47735cb to
d6a6e67
Compare
mkeeter
reviewed
Nov 25, 2024
doc/kipc.adoc
Outdated
|
|
||
| ==== Preconditions | ||
|
|
||
| The `starting_index` must be a valid index for this system. |
There was a problem hiding this comment.
Nit: we also allow the last valid index + 1, which is guaranteed to return 0.
mkeeter
reviewed
Nov 25, 2024
sys/kern/src/kipc.rs
Outdated
Comment on lines
494
to
504
| for i in index..tasks.len() { | ||
| if let TaskState::Faulted { .. } = tasks[i].state() { | ||
| let response_len = | ||
| serialize_response(&mut tasks[caller], response, &(i as u32))?; | ||
| tasks[caller] | ||
| .save_mut() | ||
| .set_send_response_and_length(0, response_len); | ||
| return Ok(NextTask::Same); | ||
| } | ||
| } | ||
|
|
||
| let response_len = | ||
| serialize_response(&mut tasks[caller], response, &0_u32)?; | ||
| tasks[caller] | ||
| .save_mut() | ||
| .set_send_response_and_length(0, response_len); | ||
| Ok(NextTask::Same) |
There was a problem hiding this comment.
Take it or leave it, but this could be written more concisely as
Suggested change
| for i in index..tasks.len() { | |
| if let TaskState::Faulted { .. } = tasks[i].state() { | |
| let response_len = | |
| serialize_response(&mut tasks[caller], response, &(i as u32))?; | |
| tasks[caller] | |
| .save_mut() | |
| .set_send_response_and_length(0, response_len); | |
| return Ok(NextTask::Same); | |
| } | |
| } | |
| let response_len = | |
| serialize_response(&mut tasks[caller], response, &0_u32)?; | |
| tasks[caller] | |
| .save_mut() | |
| .set_send_response_and_length(0, response_len); | |
| Ok(NextTask::Same) | |
| let i = tasks[index..] | |
| .iter() | |
| .position(|task| matches!(task.state(), TaskState::Faulted { .. })) | |
| .map(|i| i + index) // relative -> global task index | |
| .unwrap_or(0); | |
| let response_len = | |
| serialize_response(&mut tasks[caller], response, &(i as u32))?; | |
| tasks[caller] | |
| .save_mut() | |
| .set_send_response_and_length(0, response_len); | |
| return Ok(NextTask::Same); |
There was a problem hiding this comment.
Rejiggered this a little to use enumerate and find, to ensure that we don't get a silly overflow check added to the i+index expression.
There was a problem hiding this comment.
Heh, amusingly, that totally broke it, due to a brain-o on my part. Switched back to your method. :-)
mkeeter
approved these changes
Nov 25, 2024
hawkw
requested changes
Nov 25, 2024
Comment on lines
+335
to
+354
| The "task ID or zero" return value is represented as an `Option<NonZeroUsize>` | ||
| in the Rust API, so a typical use of this kipc looks like: | ||
|
|
||
| [source,rust] | ||
| ---- | ||
| let mut next_task = 1; // skip supervisor | ||
| while let Some(fault) = kipc::find_faulted_task(next_task) { | ||
| let fault = usize::from(fault); | ||
| // do things with the faulted task | ||
|
|
||
| next_task = fault + 1; | ||
| } | ||
| ---- |
sys/userlib/src/kipc.rs
Outdated
| let mut response = 0_u32; | ||
| let (_, _) = sys_send( | ||
| TaskId::KERNEL, | ||
| Kipcnum::ReadTaskStatus as u16, |
There was a problem hiding this comment.
uhh...this looks like the wrong Kipcnum variant?
Suggested change
| Kipcnum::ReadTaskStatus as u16, | |
| Kipcnum::FindFaultedTask as u16, |
There was a problem hiding this comment.
Good catch! I've mostly tested this with exhubris where I used the right number. The perils of a fork!
cbiffle
force-pushed
the
cbiffle/find-faulted-task
branch
from
d6a6e67 to
38771da
Compare
cbiffle
force-pushed
the
cbiffle/find-faulted-task
branch
3 times, most recently
from
45ae34f to
2b1ddde
Compare
This resolves a four-year-old TODO in El Jefe asking for a way to process faulted tasks without making so many kipcs. The original supervisor kipc interface was, by definition, designed before we knew what we were doing. Now that we have some miles on the system, some things are more clear: 1. The supervisor doesn't use the TaskState data to make its decisions. 2. The TaskState data is pretty expensive to serialize/deserialize, and produces code containing panic sites. 3. Panic sites in the supervisor are bad, since it is not allowed to panic. The new find_faulted_task operation can detect all N faulted tasks using N+1 kipcs, instead of one per potentially faulted task, and the request and response messages are trivial to serialize (one four-byte integer each way). This has allowed me to write (out-of-tree) "minisuper," a supervisor in 256 bytes that cannot panic. In-tree, this has the advantage of knocking 33% off Jefe's flash size and reducing statically-analyzable max stack depth by 20%.
cbiffle
force-pushed
the
cbiffle/find-faulted-task
branch
from
2b1ddde to
123b749
Compare
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.
This resolves a four-year-old TODO in El Jefe asking for a way to process faulted tasks without making so many kipcs. The original supervisor kipc interface was, by definition, designed before we knew what we were doing. Now that we have some miles on the system, some things are more clear:
The new find_faulted_task operation can detect all N faulted tasks using N+1 kipcs, instead of one per potentially faulted task, and the request and response messages are trivial to serialize (one four-byte integer each way). This has allowed me to write (out-of-tree) "minisuper," a supervisor in 256 bytes that cannot panic.
In-tree, this has the advantage of knocking 33% off Jefe's flash size and reducing statically-analyzable max stack depth by 20%.