kernel: Add trait for process restart policies #1565

bradjc · 2020-01-27T17:26:15Z

If a process's fault response is set to "Restart", then currently that process is unconditionally restarted if it crashes. This PR adds a trait so that boards can specify the policy the kernel should use when deciding whether to restart an app or not. There is an Always option to match what we have now, and I also implemented a very simple threshold approach where an app will only be restarted a certain number of times.

This is part of #1082.

Testing Strategy

I tested this earlier, but need to test with the recent PR merges.

TODO or Help Wanted

Thoughts?

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make formatall.

alevy

I have some minor inline naming nits.

Overall, I wonder if this is better to do on a per-process/app basis, rather than an entire kernel. Wouldn't it make more sense to have different policies for different apps than to have a fixed policy for an entire board?

alevy · 2020-01-27T19:49:14Z

kernel/src/process.rs

+
+/// Implementation of `ProcessRestartPolicy` that unconditionally restarts the
+/// app.
+pub struct PrpAlways {}


How about just naming it "AlwaysRestart"?

alevy · 2020-01-27T19:51:21Z

kernel/src/process.rs

+/// Implementation of `ProcessRestartPolicy` that uses a threshold to decide
+/// whether to restart an app. If the app has been restarted more times than the
+/// threshold then the app will no longer be restarted.
+pub struct PrpThreshold {


How about "ThresholdRestart"?

alevy · 2020-01-27T19:53:19Z

kernel/src/process.rs

+    /// Decide whether to restart the `process` or not.
+    ///
+    /// Returns `true` if the process should be restarted, `false` otherwise.
+    fn evaluate(&self, process: &dyn ProcessType) -> bool;


This type signature/name is kind of clunky. If it should return a bool (indicating whether the process should be restarted, maybe it the method should be called should_restart or something.

Alternatively, maybe call it fault_behavior have it return a specialized enum:

enum FaultBehavior { Restart, Terminate }

alevy · 2020-01-27T19:54:14Z

kernel/src/process.rs

+    ///
+    /// The provided restart policy is used to determine whether to reset the
+    /// app, and can be specified on a per-app basis.
+    Restart(&'static dyn ProcessRestartPolicy),


Just noting that this is going to add two words of memory to the kernel, regardless of whether this variant is used.

It will add two words to the process, which we charge to the process's memory region.

kernel/src/process.rs

bradjc · 2020-01-27T20:07:30Z

This is in fact already per-process because yes, that likely makes more sense. The limiting factor is the shared load_processes() function uses the same FaultResponse policy for every app it finds, but the FaultResponse is stored per-process.

alevy · 2020-01-27T21:34:48Z

@bradjc, OK missed that, my bad.

So now it's an additional word in memory for each process (not a huge deal), and my only comments are about the name/type-signature

bradjc · 2020-01-29T22:16:54Z

Added a new commit which updates the naming.

bradjc · 2020-02-15T00:00:20Z

Other thoughts?

hudson-ayers

This looks mostly good to me. Included a few nits that you can address if you like or ignore, I don't think any of them are really worth blocking on if you want to get this merged.

kernel/src/process.rs

This adds a trait so that boards can specify the policy the kernel should use when deciding whether to restart an app or not.

Since restart_count is used in decision making, and not just debugging, it doesn't make sense to leaave it as a debug variable. This promotes it to an actual field in the process struct. Also, remove re-setting the process state if syscall init fails.

bradjc · 2020-02-21T15:28:54Z

I moved the restart count out of debugging since it is now used for more than just debugging. Also rebased.

ppannuto

bors r+

bors · 2020-02-25T02:19:11Z

Build succeeded

continuous-integration/travis-ci/push

1609: ADC Capsule: More work on supporting restarts. r=alevy a=bradjc ### Pull Request Overview This pull request continues to update the ADC capsule to allow applications to restart while using the ADC driver and for the system to continue working afterwards. Highlights of changes: - Handle `InactiveApp` error when checking if a grant region is valid. - Check on `sample_ready()` and `samples_ready()` if the app is still valid, and properly handle it if not. - Check on every command syscall if the app that has exclusive control of the adc driver still exists. - Call stop sampling if a callback from the adc peripheral driver occurs and the app is no longer valid. Note, if/when the ADC capsule is virtualized then this will no longer be an issue. ### Testing Strategy I ran the adc and adc_continuous apps numerous times and used the process console to cause them to fault in various stages of their operation. I can now no longer get the ADC capsule into a state where apps only error. ### TODO or Help Wanted I tested this with a combination of my other PRs #1565 #1566 #1588. Until those are merged this PR/commit alone will not work. ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make formatall`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

bradjc added the kernel label Jan 27, 2020

alevy reviewed Jan 27, 2020

View reviewed changes

bradjc mentioned this pull request Jan 27, 2020

Tracking: Allow Apps to Fault and Restart Successfully #1082

Closed

15 tasks

bradjc force-pushed the process-restart-policy branch 2 times, most recently from 25b1279 to 0a887c5 Compare January 29, 2020 21:51

alevy previously approved these changes Jan 29, 2020

View reviewed changes

ppannuto previously approved these changes Feb 4, 2020

View reviewed changes

bradjc mentioned this pull request Feb 18, 2020

ADC Capsule: More work on supporting restarts. #1609

Merged

2 tasks

hudson-ayers previously approved these changes Feb 21, 2020

View reviewed changes

kernel/src/process.rs Outdated Show resolved Hide resolved

kernel/src/process.rs Outdated Show resolved Hide resolved

kernel/src/process.rs Show resolved Hide resolved

bradjc added 3 commits February 21, 2020 10:14

kernel: process: support process restart policy

f1df216

This adds a trait so that boards can specify the policy the kernel should use when deciding whether to restart an app or not.

kernel: process: update restart policy names

053588d

bradjc dismissed stale reviews from hudson-ayers, ppannuto, and alevy via ac58cda February 21, 2020 15:27

bradjc force-pushed the process-restart-policy branch from 0a887c5 to ac58cda Compare February 21, 2020 15:27

alistair23 approved these changes Feb 21, 2020

View reviewed changes

hudson-ayers approved these changes Feb 21, 2020

View reviewed changes

ppannuto approved these changes Feb 25, 2020

View reviewed changes

bors bot merged commit c6ad079 into master Feb 25, 2020

bors bot deleted the process-restart-policy branch February 25, 2020 02:19

bradjc mentioned this pull request Feb 25, 2020

Add an exit syscall (was: Add a syscall to notify the kernel that the app panicked) #1624

Closed

2 tasks

bradjc mentioned this pull request Apr 30, 2020

Release 2020-04-30 13:11:39 -0400 #1810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: Add trait for process restart policies #1565

kernel: Add trait for process restart policies #1565

bradjc commented Jan 27, 2020 •

edited

Loading

alevy left a comment

alevy Jan 27, 2020

alevy Jan 27, 2020

alevy Jan 27, 2020

alevy Jan 27, 2020

bradjc Jan 29, 2020

bradjc commented Jan 27, 2020

alevy commented Jan 27, 2020

bradjc commented Jan 29, 2020

bradjc commented Feb 15, 2020

hudson-ayers left a comment

bradjc commented Feb 21, 2020

ppannuto left a comment

bors bot commented Feb 25, 2020

kernel: Add trait for process restart policies #1565

kernel: Add trait for process restart policies #1565

Conversation

bradjc commented Jan 27, 2020 • edited Loading

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

alevy left a comment

Choose a reason for hiding this comment

alevy Jan 27, 2020

Choose a reason for hiding this comment

alevy Jan 27, 2020

Choose a reason for hiding this comment

alevy Jan 27, 2020

Choose a reason for hiding this comment

alevy Jan 27, 2020

Choose a reason for hiding this comment

bradjc Jan 29, 2020

Choose a reason for hiding this comment

bradjc commented Jan 27, 2020

alevy commented Jan 27, 2020

bradjc commented Jan 29, 2020

bradjc commented Feb 15, 2020

hudson-ayers left a comment

Choose a reason for hiding this comment

bradjc commented Feb 21, 2020

ppannuto left a comment

Choose a reason for hiding this comment

bors bot commented Feb 25, 2020

Build succeeded

bradjc commented Jan 27, 2020 •

edited

Loading