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.

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

backport: fix ceiling analysis bug #183

Merged
merged 2 commits into from
May 1, 2019
Merged

backport: fix ceiling analysis bug #183

merged 2 commits into from
May 1, 2019

Conversation

japaric
Copy link
Collaborator

@japaric japaric commented May 1, 2019

This commit fixes a ceiling bug where the ceiling of a ready queue will be
incorrectly computed. The analysis was not including the priority of the system
timer interrupt (SysTick) in the analysis resulting in a priority ceiling
lower than what's required for memory safety which led to data races.

The bug can be observed in the following program:

 #[rtfm::app(device = /* .. */)]
const APP: () = {
    #[init]
    fn init() {
        // ..
    }

    #[task(priority = 2)]
    fn foo(x: i32) {
        // ..
    }

    #[task(priority = 1, spawn = [foo], schedule = [foo])]
    fn bar() {
        // ..
    }

    extern "C" {
        fn EXTI0();
        fn EXTI1();
    }
};

Here the framework chooses a priority of 2 for the SysTick interrupt
(because it matches the priority of the schedule-able task foo).

Both SysTick and bar::Spawn.foo need to access the ready queue (which, in
this case, stores the messages sent to task foo) but the framework doesn't
account for the priority of SysTick (2) and chooses a priority ceiling of
1 for the ready queue (because it matches the priority of task bar which can
spawn foo).

The result is that bar::Spawn.foo modifies the ready queue without a
critical section (because bar's priority matches the priority ceiling of the
ready queue) which is wrong because SysTick (priority = 3) can also modify
the ready queue.

This commit fixes a ceiling bug where the ceiling of a ready queue will be
incorrectly computed. The analysis was not including the priority of the system
timer interrupt (`SysTick`) in the analysis resulting in a priority ceiling
lower than what's required for memory safety which led to data races.

The bug can be observed in the following program:

``` rust
 #[rtfm::app(device = /* .. */)]
const APP: () = {
    #[init]
    fn init() {
        // ..
    }

    #[task(priority = 2)]
    fn foo(x: i32) {
        // ..
    }

    #[task(priority = 1, spawn = [foo], schedule = [foo])]
    fn bar() {
        // ..
    }

    extern "C" {
        fn EXTI0();
        fn EXTI1();
    }
};
```

Here the framework chooses a priority of `2` for the `SysTick` interrupt
(because it matches the priority of the `schedule`-able task `foo`).

Both `SysTick` and `bar::Spawn.foo` need to access the ready queue (which, in
this case, stores the messages sent to task `foo`) but the framework doesn't
account for the priority of `SysTick` (`2`) and chooses a priority ceiling of
`1` for the ready queue (because it matches the priority of task `bar` which can
spawn `foo`).

The result is that `bar::Spawn.foo` modifies the ready queue *without* a
critical section (because `bar`'s priority matches the priority ceiling of the
ready queue) which is wrong because `SysTick` (priority = `3`) can also modify
the ready queue.
@japaric
Copy link
Collaborator Author

japaric commented May 1, 2019

bors try

bors bot added a commit that referenced this pull request May 1, 2019
@bors
Copy link
Contributor

bors bot commented May 1, 2019

try

Build failed

@japaric
Copy link
Collaborator Author

japaric commented May 1, 2019

bors try

bors bot added a commit that referenced this pull request May 1, 2019
@bors
Copy link
Contributor

bors bot commented May 1, 2019

try

Build succeeded

@japaric japaric merged commit 4df6292 into v0.4.x May 1, 2019
@bors bors bot deleted the tq-ceiling-bug branch May 1, 2019 18:47
bors bot added a commit that referenced this pull request Sep 15, 2019
205: rtfm-syntax refactor + heterogeneous multi-core support r=japaric a=japaric

this PR implements RFCs #178, #198, #199, #200, #201, #203 (only the refactor
part), #204, #207, #211 and #212.

most cfail tests have been removed because the test suite of `rtfm-syntax`
already tests what was being tested here. The `rtfm-syntax` crate also has tests
for the analysis pass which we didn't have here -- that test suite contains a
regression test for #183.

the remaining cfail tests have been upgraded into UI test so we can more
thoroughly check / test the error message presented to the end user.

the cpass tests have been converted into plain examples

EDIT: I forgot, there are some examples of the multi-core API for the LPC541xx in [this repository](https://github.com/japaric/lpcxpresso54114)

people that would like to try out this API but have no hardware can try out the
x86_64 [Linux port] which also has multi-core support.

[Linux port]: https://github.com/japaric/linux-rtfm

closes #178 #198 #199 #200 #201 #203 #204 #207 #211 #212 
closes #163 
cc #209 (documents how to deal with errors)

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
bors bot added a commit that referenced this pull request Sep 15, 2019
205: rtfm-syntax refactor + heterogeneous multi-core support r=japaric a=japaric

this PR implements RFCs #178, #198, #199, #200, #201, #203 (only the refactor
part), #204, #207, #211 and #212.

most cfail tests have been removed because the test suite of `rtfm-syntax`
already tests what was being tested here. The `rtfm-syntax` crate also has tests
for the analysis pass which we didn't have here -- that test suite contains a
regression test for #183.

the remaining cfail tests have been upgraded into UI test so we can more
thoroughly check / test the error message presented to the end user.

the cpass tests have been converted into plain examples

EDIT: I forgot, there are some examples of the multi-core API for the LPC541xx in [this repository](https://github.com/japaric/lpcxpresso54114)

people that would like to try out this API but have no hardware can try out the
x86_64 [Linux port] which also has multi-core support.

[Linux port]: https://github.com/japaric/linux-rtfm

closes #178 #198 #199 #200 #201 #203 #204 #207 #211 #212 
closes #163 
cc #209 (documents how to deal with errors)

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
andrewgazelka pushed a commit to andrewgazelka/cortex-m-rtic that referenced this pull request Nov 3, 2021
183: Update cortex-m version requirement to allow using 0.6.0 r=therealprof a=adamgreig

See also rust-embedded/cortex-m-semihosting#32

Co-authored-by: Adam Greig <adam@adamgreig.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant