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.

Sign up for GitHub

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

Enable a panic-free Jefe for the first time #1938

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Enable a panic-free Jefe for the first time #1938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/donglet/app-g031.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ name = "task-jefe"
priority = 0
max-sizes = {flash = 4096, ram = 512}
start = true
stacksize = 368
stacksize = 192
notifications = ["fault", "timer"]
features = ["no-panic", "nano"]

[tasks.sys]
name = "drv-stm32xx-sys"
Expand Down
1 change: 1 addition & 0 deletions task/jefe/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ build-util = { path = "../../build/util" }
[features]
dump = []
nano = [ "ringbuf/disabled" ]
no-panic = [ "userlib/no-panic" ]
Copy link
Member

Choose a reason for hiding this comment

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

take it or leave it: perhaps we ought to add something like:

#[cfg(all(feature = "dump", feature = "no-panic"))]
compile_error!(
      "the \"dump\" feature cannot currently be enabled when \
      \"no-panic\" is enabled,
");

at the top of main.rs?

i realize that enabling them both will get a linker error anyway when humpty tries to link with the panic impl, but it might be nicer to actually tell the person who enabled both features the specific reason Jefe is trying to panic, rather than making them go and figure it out? not a huge deal though.


# This section is here to discourage RLS/rust-analyzer from doing test builds,
# since test builds don't work for cross compilation.
Expand Down
16 changes: 13 additions & 3 deletions task/jefe/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,19 @@ impl idol_runtime::NotificationHandler for ServerImpl<'_> {
let mut next_task = 1;
while let Some(fault_index) = kipc::find_faulted_task(next_task) {
let fault_index = usize::from(fault_index);
next_task = fault_index + 1;

let status = &mut self.task_states[fault_index];
// This addition cannot overflow in practice, because the number
// of tasks in the system is very much smaller than 2**32. So we
// use wrapping add, because currently the compiler doesn't
// understand this property.
next_task = fault_index.wrapping_add(1);

// Safety: `fault_index` is from the kernel, and the kernel will
// not give us an out-of-range task index.
//
// TODO: it might be nice to fold this into a utility function
// in kipc or something
Comment on lines +322 to +323
Copy link
Member

Choose a reason for hiding this comment

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

Hm, what are you imagining here? A newtype representing a validated task index that's in range? IIUC that would also mean having a newtype around a task states array in kipc that has a safe unchecked index operation taking that type, right? And at that point it feels like stuffing a lot of Jefe into the kipc crate...

Or, am I misunderstanding the idea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking maybe a wrapper for find_faulted_task that also takes an array array and indexes it for you, based on this validity assumption. So it'd be like

fn find_faulted_task_plusplus<T>(start: usize, array: &mut [T; NUM_TASKS]) -> Option<(NonZeroUsize, &mut T)>

And at that point it feels like stuffing a lot of Jefe into the kipc crate...

You're not wrong about that, but since kipc is mostly intended for the use of the supervisor... if we find a genuinely repeating pattern, moving it into the crate isn't necessarily bad.

This one might be too special purpose though, which is why I left a TODO instead of attempting to design it.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking maybe a wrapper for find_faulted_task that also takes an array array and indexes it for you, based on this validity assumption.

Oh, that's much simpler than the API I had envisioned, that seems like a good idea!

And at that point it feels like stuffing a lot of Jefe into the kipc crate...

You're not wrong about that, but since kipc is mostly intended for the use of the supervisor... if we find a genuinely repeating pattern, moving it into the crate isn't necessarily bad.

Yeah, I think it's entirely reasonable for kipc to have APIs that might be reasonably used by any supervisor implementation, which I think a find_faulted_task_in_array (or whatever you want to call it) would be. I guess it does kind of make the assumption that a supervisor implementation will want to store per-task states in an array, but I don't know why you wouldn't want to do that, and you can just not use the API if you're doing something weird...

let status =
unsafe { self.task_states.get_unchecked_mut(fault_index) };

// If we're aware that this task is in a fault state, don't
// bother making a syscall to enquire.
Expand Down
Loading