core: add priority inheritance option

[haukepetersen]

This PR allows the scheduler to enable priority inheritance when dealing with critical sections, which is a default way to prevent priority inversion (-> see #7365 and related #7444, #7372).

To enable the use of priority inheritance, just build RIOT adding the core_priority_inheritance pseudo module.

Just to give a first idea about the memory impact: enabling this module for gnrc_networking on the samr21-xpro adds 36b RAM and 184b ROM to the build. So this comes indeed not for free...

EDIT:
Our of offline discussion with @kaspar030 and by thinking about it, I came to the conclusion, that mutexes and msg_send_receive() are the only two things in the kernel that model something like critical sections and where it makes sense to inherit priorities. So I extended this PR also to cover msg_send_receive().

[kaspar030]

Nice!

This PR allows the scheduler to enable priority inheritance when dealing with critical sections,

please add that "critical sections" in this case means "mutexes".

[haukepetersen]

didn't want to add that (yet), as I think I can also do it easily for msg_send_recv in the next 30 min or so :-)

[miri64]

please keep spacing consistent in the license header (I prefer without spaces and it seems to be the goto in the code base)

[haukepetersen]

yepp, I'll remove them

[jnohlgard]

Nice that you are tackling this issue, @haukepetersen!

[haukepetersen]

fixed some minor things and added priority inheritance to msg_send_receive().

[bergzand]

Might be just an edge case, but does this work as intended when there are two partially overlapping critical sections, such that:

1. Mutex A locks
2. Mutex B locks
3. Mutex A unlocks
4. Mutex B unlocks

Assuming that there are two other higher priority threads waiting on A and B respectively.
As far as I can see, the unlock of A would restore the priority to the original priority of the thread instead of the priority needed to prevent priority inversion for mutex B.

If this is an issue, I would not consider this blocking, this solution is better than the current situation.

[haukepetersen]

Yes, on first sight this actually does seem like an issue. I will create a test case later today to make sure. And though I think this scenario can be constructed artificially, I wonder if this situation does actually occur in 'normal' code sequences?

[haukepetersen]

test created in #7455

[bergzand]

To be honest, I don't think it is necessary to take the overlapping mutex scenario into account. I think it is good to have somewhere some documentation that it does not work for overlapping mutexes, but a fix for cleanly nested mutexes should be enough in most situations. Then, if somebody ever manages to trigger this scenario, it is at least possible to extend one of the critical sections to build a nested structure.

[haukepetersen]

I agree. Though I think the test application for that does not hurt, useful for documentation as well as future tracking (maybe someone at some point comes up with an efficient working solution to that problem as well...).

So how do we proceed with this PR? I would suggest that we need at least two very thorough reviews. Also I guess I need to write some comprehensive documentation. And last we need some more code size analysis to make sure that this PR does not change anything if the core_priority_inheritance module is not included. Do you guys agree with this?

[travisgriggs]

Long term, will there be any reason to not ALWAYS have priority inheritance turned on? Any other industrial RTOS I've noticed that brags about this, doesn't offer it as an option, instead it just claims it does it.

[jnohlgard]

I agree with @travisgriggs, priority inheritance should be a default feature of the kernel.

[haukepetersen]

There is one important reason: memory usage. Priority inheritance adds noticeable memory overheads when used, so enabling this per default in RIOT (where many applications don't even have need for real-time features) is at least debatable... The question is is guess: do we per default want many features in that can be disabled when optimizing for memory usage, or do we want a very slim default system and users enable the features they want to have. I tend to the latter here!

[OlegHahm]

@haukepetersen, @kaspar030, couldn't one argue that the same is true for the current IPC (core_msg) which is built-in per default AFAIK.

[bergzand]

I would argue for enabling this module by default. Mainly because of usability. I think it is a lot easier to disable this module when a developer is certain that he really doesn't need it and he does need the memory footprint reduction, than to have to debug scheduling issues until the developer finds out that PIP is not enabled.

This might sound a bit exaggerated. What I want to say is that it is probably easier to slim down a RIOT build by removing modules you're certain you don't need than it is to hunt down issues due to modules you did have to enable explicitly (even more so with scheduling issues).

[jnohlgard]

I agree with @bergzand regarding enabling this module by default.

[haukepetersen]

couldn't one argue that the same is true for the current IPC (core_msg)

Yes, and I would tend to disable this by default, also :-)

[travisgriggs]

I don't care whether it's modular or not, but... I believe this should be bound to the "prioritized threads" module. It's an inherit issue with prioritized threads. A module which adds "threads with priorities" should include this behavior.

[OlegHahm]

I also tend to agree with @bergzand. Disabling features that are not always needed (e.g., IPC) seems legit since a developer will easily figure out that he needs this feature. Disabling certain properties of the kernel may become much harder to detect.

[haukepetersen]

rebased on top of #7444 and #9306 and enabled the core_priority_inheritance module per default

Starting to address comments next

[haukepetersen]

addressed some of the comments, will continue tomorrow.

[kaspar030]

enabled the core_priority_inheritance module per default

Maybe save for another PR?

[kaspar030]

enabled the core_priority_inheritance module per default

Let me elaborate on this.

Long term, will there be any reason to not ALWAYS have priority inheritance turned on? Any other industrial RTOS I've noticed that brags about this, doesn't offer it as an option, instead it just claims it does it.

I agree with @travisgriggs, priority inheritance should be a default feature of the kernel.

I'm really happy that RIOT is about to get priority inheritance. But let's not forget that until know, everything that runs on RIOT did so without until now. noone has shown a real world application where priority inheritance would have saved the day, but cannot be trivially fixed through other means.

There might be a chicken-and-egg problem: anyone who knows his job and does real-time looks at RIOT, sees "no PI", and moves on, and thus we do not have users that might be happier with priority inheritance enabled. That might be the case, but IMO, having priority inheritance available but optional should solve this.

So now we have this feature, but enabling costs RAM, ROM and performance. The proposal is to enable it by default. So every application will now have to pay that price from now on (e.g., on upgrading to the latest release)? To, me that is unacceptable. It is feature bloat that we have to avoid.

I suggest keeping it optional, adding it to the features list, adding prominent documentation notes, and adding some kind of realtime guidelines where we suggest enabling priority inversion.

Apart from that, let's keep it disabled.

I also think that having this enabled by default with the possibility of disabling will be beneficial for everyone.

Beneficial why? I have not yet seen an argument pro "priority inheritance by default" other than "others do it" and "I think it should be done".

@gdoffe you were using RIOT for robotics in a real-time environment? What's your take on this?

[bergzand]

@kaspar030 I would like to have this merged without PI enabled by default and then start a discussion in a separate issue with @gdoffe and @astralien3000. I think we all agree that it is better to have PI as an option in RIOT than to stall this PR more than necessary to discuss whether we want to have it enabled by default.

[kaspar030]

I would like to have this merged without PI enabled by default and then start a discussion in a separate issue

totally!

[danpetry]

Sounds like a plan. I've started an issue #9379 to discuss further. Hopefully everyone feels that their view is accurately represented in the summary, let me know if not.

[stale]

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.

[jia200x]

Let's continue with this!
Besides the rebase, are there any show stoppers?

[OlegHahm]

Do we still need this after #17093 got merged?

[MrKevinWeiss]

ping

[maribu]

Since an alternative implementation is merged by now, this is no longer needed