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: make HAVE_WORK a member variable #1044

Merged
merged 2 commits into from Jul 5, 2018

Conversation

Projects
None yet
3 participants
@bradjc
Copy link
Contributor

bradjc commented Jun 28, 2018

HAVE_WORK is currently a process.rs global static variable. This is probably not what we really want, as it requires unsafe to use. This creates a Kernel struct and stores the number of callbacks waiting as a member variable in that struct. In the process, I counted 5 uses of unsafe that are now gone.

Now, before HAVE_WORK was a VolatileCell. I'm not sure why that is needed, so I made it a NumCell instead (essentially just a Cell). If someone knows why it must be volatile then it can be changed back.

Also, this moves the schedule function to a be a class function of Process, where it probably should have been anyway.

Testing Strategy

This pull request was tested by running hail on hail.

TODO or Help Wanted

n/a

Documentation Updated

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

Formatting

  • Ran make formatall.
@ppannuto
Copy link
Member

ppannuto left a comment

This looks good to me. I think @alevy should look it over before merging too.

I think once upon a time, top-half interrupt handlers incremented HAVE_WORK, but that's not the case any more, so I think it's safe to no longer be volatile

@bradjc bradjc force-pushed the kernel-oop-just-work branch from 6d679ea to 2aa2bc0 Jun 29, 2018

kernel: make HAVE_WORK a member variable
Also move the schedule function to a be a class function.

@ppannuto ppannuto force-pushed the kernel-oop-just-work branch from 2aa2bc0 to 34261e0 Jul 2, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 2, 2018

(Pushed a fix for the formatting, was an extra blank line)

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 3, 2018

I want to note that this PR is in service to tracking issue #1043.

@ppannuto ppannuto requested a review from alevy Jul 3, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

Now, before HAVE_WORK was a VolatileCell

This seems to be important for the compiler to elide writes to static mutables that are not read within the same crate. Or at least was. So it was a hack, and I don't think we should need it if it's a member field.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

In the spirit of our last weekly call, I think it's important to describe how these changes impact interfaces elsewhere.

I think given that #1043 is a tracking issue, it's good to work on these kinds of incremental changes, but would also just be good to document what someone getting an external port or capsule up to speed with a recent Tock version might need to change.

In this case, I believe the only required change is in the board configuration, where instead of calling kernel::kernel_loop(...), you now must first initialize a statically allocated instance of the Kernel struct and call kernel_loop as a method:

let board_kernel = static_init!(kernel::Kernel, kernel::Kernel::new());

board_kernel.kernel_loop(&tm4c1294, &mut chip, &mut PROCESSES, Some(&tm4c1294.ipc));

the arguments to kernel_loop don't change.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

These changes have the Kernel struct stored with an explicit 'static lifetime. That's reasonable since it's probably a reasonable choice to statically allocate it anyway (we want these structures to tax sections of the binary such that an overflow will be caught by the linker, so not on the stack), but I wonder if this is a hard constraint, or if the lifetime can be parameterized.

In general, my intuition is that parameterizing lifetimes will give us more flexibility to eventually take out shared static variables (this is an intuition, I don't have a good argument for why that is).

I think it's fine to leave this as-is, if we convince ourselves that clients of this interface (board main.rss) will mostly likely still statically allocate, and in any case any relaxation of the lifetime constraint will still be compatible with static allocation (I think it is).

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

I added a CHANGELOG file with the changes I think matter for external projects. I'm approving with this addition, but leaving the final word on this to @bradjc. I'm also marking this P-Significant, which gives it another day for someone to raise the alarm bells.

Since 1.2
=========

* #1044 creates a `Kernel` struct with a method for the kernel's main loop,

This comment has been minimized.

@ppannuto

ppannuto Jul 4, 2018

Member

It doesn't like like GH auto-links numbers in generic markdown files :(
https://github.com/tock/tock/blob/kernel-oop-just-work/CHANGELOG.md

@ppannuto ppannuto referenced this pull request Jul 4, 2018

Merged

Specify units for ADC HIL #1032

2 of 2 tasks complete
@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 5, 2018

Looks good to me :)

@ppannuto ppannuto force-pushed the kernel-oop-just-work branch from 02e6538 to 3a6c9e4 Jul 5, 2018

@ppannuto
Copy link
Member

ppannuto left a comment

Fixed the link that was bugging me. Last call was a day ago, I think this is good to merge.

@bradjc bradjc merged commit 4e9b269 into master Jul 5, 2018

2 checks passed

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

@bradjc bradjc deleted the kernel-oop-just-work branch Jul 5, 2018

@ppannuto ppannuto referenced this pull request Jul 6, 2018

Merged

fix dangling reference to NumCell #1078

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment