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.

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

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

Merged
merged 7 commits into from Jul 26, 2018

Conversation

Projects
None yet
5 participants
@bradjc
Copy link
Contributor

bradjc commented Jul 10, 2018

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:

  1. 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).
  2. This will help with the MPU changes.
  3. 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.

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

@bradjc bradjc force-pushed the kernel-procs branch from 0cfa69d to ae839a2 Jul 11, 2018

@bradjc bradjc force-pushed the kernel-procs branch from ae839a2 to 24414a6 Jul 12, 2018

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

@bradjc bradjc referenced this pull request Jul 13, 2018

Merged

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

2 of 2 tasks complete

@bradjc bradjc force-pushed the kernel-procs branch 2 times, most recently from 04ca10e to 048c183 Jul 17, 2018

@cmcavity

This comment has been minimized.

Copy link
Contributor

cmcavity commented Jul 17, 2018

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

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 17, 2018

Check out #1113

@alevy alevy added the blocked label Jul 18, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 18, 2018

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

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 18, 2018

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.

bradjc added some commits Jul 9, 2018

kernel: move procs array to Kernel struct
This removes the need for the static PROCS array.
kernel: define Eq, PartialEq, Debug for AppId
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
There is no longer a static reference to it in the kernel crate.

@bradjc bradjc force-pushed the kernel-procs branch from 048c183 to 0cb4875 Jul 19, 2018

@bradjc bradjc removed the blocked label Jul 19, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 19, 2018

Ok! Please have a look at this PR.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 20, 2018

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:

  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).

Thoughts?

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 20, 2018

@bradjc I'm convinced

bradjc added some commits Jul 20, 2018

kernel: add `kernel.create_grant()`
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 bradjc force-pushed the kernel-procs branch from 3d92ee3 to da05189 Jul 20, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 20, 2018

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

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

PR #1111
This removes the need for the static PROCS array.
@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 24, 2018

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

@ppannuto
Copy link
Member

ppannuto left a comment

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.

@bradjc bradjc force-pushed the kernel-procs branch from 447fb7e to 4b1a6ae Jul 25, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 25, 2018

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

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

This comment has been minimized.

@alevy

alevy Jul 25, 2018

Member

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

@bradjc bradjc force-pushed the kernel-procs branch from 4b1a6ae to da05189 Jul 25, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 25, 2018

Removed the commit.

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

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 25, 2018

@bradjc agreed!

@alevy

alevy approved these changes Jul 25, 2018

Copy link
Member

alevy left a comment

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

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 25, 2018

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.

@bradjc bradjc added the last-call label Jul 25, 2018

@dverhaert

This comment has been minimized.

Copy link
Contributor

dverhaert commented Jul 25, 2018

Looks good!

@bradjc bradjc merged commit 7837c2f into master Jul 26, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@bors bors bot deleted the kernel-procs branch Jul 26, 2018

bors bot added a commit that referenced this pull request Aug 13, 2018

Merge #1113
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment