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

Causing deadlocks while using priority inheritance that are avoidable with priority ceiling #8330

Closed

Conversation

mmkonrad
Copy link

@mmkonrad mmkonrad commented Jan 6, 2018

The sole purpose of this PR is to raise awareness of the possible necessity to implement a priority ceiling module.

It started with #7365 where it was discovered by @geith that RIOT OS is not yet protected against the priority inversion problem.

As a result @haukepetersen implemented tests for this problem (-> see #7444 and #7372) and implemented a module for priority inheritance fixing this problem as stated in PR #7445 .

As a part of a university project I had to cause deadlocks while using this module. Since it is easy to cause deadlocks by having two threads trying to lock e.g. a mutex the respective other holds, the idea was provided to construct a case where a deadlock could be prevented whith priority ceiling.

The code used in this scenario is based on the example provided by @geith in #7365. As can be seen in the graphic, having a module for priority inheritance wouldn't cause this program to get stuck.

riot_os_priority_inheritance_vs_ceiling

Of course this constructed scenario could easily get avoided by a capable programer but having the option to use priority ceiling would increase the latitude of the users when facing edge cases.

@tcschmidt tcschmidt requested a review from danpetry May 29, 2018 08:51
@danpetry
Copy link
Contributor

danpetry commented Jun 7, 2018

Hi @mmkonrad . Sorry for the delay on addressing your PR. Thanks for your contribution :)
We are first addressing the PRs that introduce priority inversion and address (passing) tests for this. That is, #7445, #7444, and #9306.
To address the question of whether we need priority ceiling, rather than priority inversion. My brief investigation and your results seem to indicate that priority ceiling is often superior in preventing deadlocks. However, it would require an API change to the mutexes so that a ceiling could be programmed for each mutex. Since this is a core API, there would have to be a very compelling reason for it, and in general this issue (priority inversion) is not something that is really a problem for us at the moment, so I don't think there is such a reason. Also, one of our design principles is ease of use - so the tradeoff would land in favour of a transparent solution like priority inheritance.
The question is whether we would want to introduce a test which is broken (eg. as a bookmark) and this is something I'll get back to you on.

@miri64 miri64 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 17, 2018
@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! State: stale State: The issue / PR has no activity for >185 days Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants