Kernel: Move `PROCS` array to `Kernel` struct #1111

bradjc · 2018-07-10T23:46:37Z

Pull Request Overview

As a part of #1043, this moves the PROCS array to no longer be a static global variable but instead be a member of the Kernel struct. This is needed for at least three reasons:

The Process type needs to be templated across architectures, and that doesn't seem possible with the static array (because the kernel crate doesn't know what type it will actually be).
This will help with the MPU changes.
It is generally better to not have global variables that need unsafe to access.

After #1109, this is relatively straightforward, with a couple things of note:

To access the Kernel struct, many data structures need to keep a reference to the kernel struct, and these have been added.
debug.rs cannot directly access the PROCS array, so it must be passed in during a panic.

~~This is blocked on #1046~~

Testing Strategy

Running hail on hail.

TODO or Help Wanted

Update CHANGELOG
Test

Documentation Updated

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

Formatting

Ran make formatall.

cmcavity · 2018-07-17T22:28:19Z

Is there a rough estimate on when this will be merged? Danilo and I are looking into parameterizing Process so we can add a field with a generic type for chip-specific MPU configuration, and that is dependent on these changes.

Also Brad, Amit mentioned that you're working on making Process a trait object in some branch. Could you point me toward that code?

Thanks!

bradjc · 2018-07-17T23:50:47Z

Check out #1113

bradjc · 2018-07-18T01:23:53Z

Is there a rough estimate on when this will be merged? Danilo and I are looking into parameterizing Process so we can add a field with a generic type for chip-specific MPU configuration, and that is dependent on these changes.

If you can try #1046 on an imix that would help.

bradjc · 2018-07-18T01:24:35Z

As a part of rebasing this I was able to remove quite a few &'static Kernel references. There were many cases where a struct held both a reference to Kernel and some other object that also has a Kernel reference. Now we just use the reference held by the struct we already had.

I also changed grant.rs to use AppId rather than usize when referring to an app.

This removes the need for the static PROCS array.

These were derived before but since the AppId struct had to be expanded they have to be explicitely defined before.

There is no longer a static reference to it in the kernel crate.

bradjc · 2018-07-19T19:05:36Z

Ok! Please have a look at this PR.

bradjc · 2018-07-20T13:52:43Z

One idea that has been floated is changing how grants are created for capsules from:

let grant = kernel::Grant::create(board_kernel);

to

let grant = board_kernel.create_grant();

That would have three benefits:

It makes the API a little more consistent, and makes it clear that board_kernel (the &kernel::Kernel) is the main access to the kernel API. .kernel_loop() is also called from the Kernel struct.
It simplifies where the kernel API is defined. What operations does the kernel provide? Look at the pub functions in the impl Kernel block. Note: this is different than the interfaces and tools the kernel provides (like StaticRef, Component, etc., and the HIL interfaces).
It would be straightforward to move the static variable CONTAINER_COUNTER to the Kernel struct. Doing this would allow the kernel to verify that no grants are created after processes are created (which would be invalid if any code tried to do that because no space would be reserved for them in the process).

Thoughts?

alevy · 2018-07-20T15:21:26Z

@bradjc I'm convinced

This is beneficial because: 1. It makes the API a little more consistent, and makes it clear that board_kernel (the &kernel::Kernel) is the main access to the kernel API. .kernel_loop() is also called from the Kernel struct. 2. It simplifies where the kernel API is defined. What operations does the kernel provide? Look at the pub functions in the impl Kernel block. Note: this is different than the interfaces and tools the kernel provides (like StaticRef, Component, etc., and the HIL interfaces). 3. It would be straightforward to move the static variable CONTAINER_COUNTER to the Kernel struct. Doing this would allow the kernel to verify that no grants are created after processes are created (which would be invalid if any code tried to do that because no space would be reserved for them in the process).

bradjc · 2018-07-20T18:12:32Z

If you want to see it in code form it is here: 70f53ca

bradjc · 2018-07-24T17:57:25Z

Doc updates are in #1113, this pr is really more of a refactor.

ppannuto

I thought it was a little weird for app slices to hold references to the kernel which then does a lookup for the process, rather than just directly holding a reference to the process. I started trying to implement the latter on top of this and it proved a more substantial change (can/will follow-on as a subsequent improvement). This is a good step in a good direction and looks good to me.

alevy · 2018-07-25T13:42:20Z

This last change, from holding a reference to the Kernel to holding a direct reference to a Process is very problematic:

As is, there is nothing checking that this Process is indeed still the same one we thought it was when we created the AppId. It could have been replaced with a new instance of the same application (after a restart) or a different application entirely. At least going through the kernel gives us the flexibility to implement this (since an internal idx can just be an opaque identifier).

In general, it could be possible to just explicitly check that the process being referred to is the same version: replacing the idx field with a random/monotonically increasing value for each new instance of a process and comparing that with a field in the process struct.

However, indirecting through a Kernel just gives us way more flexibility about how to store processes

alevy · 2018-07-25T13:43:01Z

kernel/src/callback.rs

 pub struct AppId {
+    crate process: &'static Process<'static>,


I think this is a dangerous change, and that it should be reverted to storing a reference to the Kernel

bradjc · 2018-07-25T14:09:22Z

Removed the commit.

It will probably make sense to move to a more opaque "idx" in AppId/Process soon.

alevy · 2018-07-25T14:27:36Z

@bradjc agreed!

alevy

I would like us to update the CHANGELOG in this PR, but if @bradjc wants to do that in bullk with one or two more PRs, that's OK with me.

bradjc · 2018-07-25T14:29:42Z

I added updating the changelog as a todo item in the tracking PR. Otherwise I think it will get rewritten multiple times with the series of PRs and any potential changes.

dverhaert · 2018-07-25T23:06:46Z

Looks good!

1113: Move architecture-dependent syscall code to arch/cortex-m r=bradjc a=bradjc ### Pull Request Overview This pull request moves all of the code that is cortex-m syscall specific out of process.rs and into the cortex-m crate. This is part of #985. It does this by: - Making a new `SyscallInterface` trait that is implemented in the cortex-m crate. Each process has a reference to the implementing struct. - Making a new `ProcessType` trait that all process types implement (right now we only have one, `Process` in process.rs). This allows the `Process` struct to be templated over the `SyscallInterface` type while the `processes` array in the `Kernel` struct keeps an array of `ProcessType` implementations: ```rust Kernel { processes: &'static [Option<&'static process::ProcessType>] } ``` which means the template parameter stays only in the process.rs file and does not spread to all of Tock. The panic! prints in process.rs are partially commented out because they relied on many architecture-specific fields. Also the `SCB_REGISTERS` global static variable still exists. Moving those to the cortex-m crate is forthcoming in a different PR. This is blocked on #1111. ### Testing Strategy Hail on Hail, but needs more. ### TODO or Help Wanted n/a ### 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> Co-authored-by: Pat Pannuto <pat.pannuto@gmail.com>

bradjc added a commit that referenced this pull request Jul 11, 2018

PR #1111

0401aed

bradjc force-pushed the kernel-procs branch from 0cfa69d to ae839a2 Compare July 11, 2018 16:03

This was referenced Jul 11, 2018

Tracking: Remove static global variables in kernel (process.rs) #1043

Closed

Tracking: Make Tock Architecture Agnostic #985

Closed

bradjc force-pushed the kernel-procs branch from ae839a2 to 24414a6 Compare July 12, 2018 15:36

bradjc mentioned this pull request Jul 13, 2018

Move architecture-dependent syscall code to arch/cortex-m #1113

Merged

2 tasks

bradjc force-pushed the kernel-procs branch 2 times, most recently from 04ca10e to 048c183 Compare July 17, 2018 02:43

alevy added the blocked Waiting on something, like a different PR or a dependency. label Jul 18, 2018

bradjc added 5 commits July 19, 2018 14:58

kernel: move procs array to Kernel struct

3d579a7

This removes the need for the static PROCS array.

kernel: define Eq, PartialEq, Debug for AppId

eed1b35

These were derived before but since the AppId struct had to be expanded they have to be explicitely defined before.

kernel: debug: processes array must be passed in

c8f7550

There is no longer a static reference to it in the kernel crate.

boards: update to new kernel & panic!() interface

fde31a9

kernel: do not need &mut Process references

0cb4875

bradjc force-pushed the kernel-procs branch from 048c183 to 0cb4875 Compare July 19, 2018 19:00

bradjc removed the blocked Waiting on something, like a different PR or a dependency. label Jul 19, 2018

bradjc added 2 commits July 20, 2018 11:46

boards: update to kernel.create_grant()

da05189

bradjc force-pushed the kernel-procs branch from 3d92ee3 to da05189 Compare July 20, 2018 17:29

ppannuto previously approved these changes Jul 24, 2018

View reviewed changes

bradjc dismissed ppannuto’s stale review via 447fb7e July 25, 2018 04:36

bradjc force-pushed the kernel-procs branch from 447fb7e to 4b1a6ae Compare July 25, 2018 04:39

alevy requested changes Jul 25, 2018

View reviewed changes

bradjc force-pushed the kernel-procs branch from 4b1a6ae to da05189 Compare July 25, 2018 14:04

alevy approved these changes Jul 25, 2018

View reviewed changes

bradjc added the last-call Final review period for a pull request. label Jul 25, 2018

bradjc merged commit 7837c2f into master Jul 26, 2018

bors bot deleted the kernel-procs branch July 26, 2018 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel: Move `PROCS` array to `Kernel` struct #1111

Kernel: Move `PROCS` array to `Kernel` struct #1111

bradjc commented Jul 10, 2018 •

edited by ppannuto

cmcavity commented Jul 17, 2018

bradjc commented Jul 17, 2018

bradjc commented Jul 18, 2018

bradjc commented Jul 18, 2018

bradjc commented Jul 19, 2018

bradjc commented Jul 20, 2018 •

edited

alevy commented Jul 20, 2018

bradjc commented Jul 20, 2018

bradjc commented Jul 24, 2018

ppannuto left a comment

alevy commented Jul 25, 2018

alevy Jul 25, 2018

bradjc commented Jul 25, 2018

alevy commented Jul 25, 2018

alevy left a comment

bradjc commented Jul 25, 2018

dverhaert commented Jul 25, 2018

Kernel: Move PROCS array to Kernel struct #1111

Kernel: Move PROCS array to Kernel struct #1111

Conversation

bradjc commented Jul 10, 2018 • edited by ppannuto

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

cmcavity commented Jul 17, 2018

bradjc commented Jul 17, 2018

bradjc commented Jul 18, 2018

bradjc commented Jul 18, 2018

bradjc commented Jul 19, 2018

bradjc commented Jul 20, 2018 • edited

alevy commented Jul 20, 2018

bradjc commented Jul 20, 2018

bradjc commented Jul 24, 2018

ppannuto left a comment

Choose a reason for hiding this comment

alevy commented Jul 25, 2018

alevy Jul 25, 2018

Choose a reason for hiding this comment

bradjc commented Jul 25, 2018

alevy commented Jul 25, 2018

alevy left a comment

Choose a reason for hiding this comment

bradjc commented Jul 25, 2018

dverhaert commented Jul 25, 2018

Kernel: Move `PROCS` array to `Kernel` struct #1111

Kernel: Move `PROCS` array to `Kernel` struct #1111

bradjc commented Jul 10, 2018 •

edited by ppannuto

bradjc commented Jul 20, 2018 •

edited