tests: add test for priority inversion using muxes #7444

haukepetersen · 2017-08-04T10:00:51Z

As pointed out in #7365, we do not have any protection against priority inversion in the default kernel.

As a starter, here is a simple test application, that showcases a constructed case of priority inversion. I use this test down the line to test my implementation of priority inheritance, but I think this test deserves to be merged stand-alone so we have something to keep reminding us of this issue...

jnohlgard · 2017-08-04T11:04:21Z

See also #7372

haukepetersen · 2017-08-04T11:05:42Z

oh damn, completely overlooked that one...

haukepetersen · 2017-08-28T12:43:57Z

@geith, @smlng, @gebart: do you guys have any preference towards this PR or towards #7372? Would be nice to get one of them merged and the other one closed.

smlng · 2017-08-28T12:52:28Z

Both look very similar to me, which is not a surprise considering the issue at hand. Thus, no an actual preference, other than first come, first merged 😉

haukepetersen · 2017-08-28T13:12:17Z

I added some more output (e.g. numbering the events to verify the order in which they occur). But you are right, first come first serve should be the way to go. So let's merge #7372 and I can add the improved output once merged.

smlng · 2017-12-21T12:16:52Z

#7372 is merged now, please rebase and adapt as needed

kaspar030 · 2017-12-21T12:38:44Z

tests/mutex_priority_inversion/Makefile

@@ -0,0 +1,9 @@
+APPLICATION = mutex_priority_inversion


remove this line

kaspar030 · 2017-12-21T12:39:08Z

tests/mutex_priority_inversion/main.c

+#include "mutex.h"
+#include "xtimer.h"
+
+#define TICK_LEN            (50 * US_PER_MS)


kaspar030 · 2017-12-21T12:40:36Z

tests/mutex_priority_inversion/main.c

+#define TICK_LEN            (50 * US_PER_MS)
+#define EXSPECTED_RESULT    (4)
+
+static mutex_t res_mtx;


static mutex_t res_mtx = MUTEX_INIT; and remove dynamic initialization in main()?

kaspar030 · 2017-12-21T12:41:39Z

tests/mutex_priority_inversion/main.c

+#include "xtimer.h"
+
+#define TICK_LEN            (50 * US_PER_MS)
+#define EXSPECTED_RESULT    (4)


here and below: EXSPECTED -> EXPECTED

smlng · 2018-01-15T09:22:10Z

@haukepetersen any chance you'll adapt to #7372 and address comments to get this merged for 2018.01 release? Otherwise, please postpone to 2018.04.

haukepetersen · 2018-01-30T19:05:20Z

sorry for the long silence. Merged the previous test application for this matter with my approach. I see the following enhancements:

test is now automatable (and the pexpect script is included)
the numbering of events makes it easier to see if the order is correct
some minor code fixes (e.g. static for global variables, etc)

smlng · 2018-01-31T09:39:27Z

I'm unsure if the (re)naming makes sense, i.e., mutex_ instead of thread_priority_inversion. I mean the threads have priorities and not the mutex. Hence, thread (as used by @geith) makes IMHO more sense.

haukepetersen · 2018-01-31T12:20:00Z

I also tended to this, but we can/should construct a second test that demonstrates the same behavior using msg...

So how about thread_priority_inversion_mutex and (for future use) thread_priority_inversion_msg?

smlng · 2018-01-31T12:24:29Z

So how about thread_priority_inversion_mutex

sounds semantically more right 😉

haukepetersen · 2018-02-06T12:19:34Z

alright, will do.

haukepetersen · 2018-06-07T11:21:03Z

renamed the test to thread_priority_inversion_mutex as discussed above. Also removed the then deprecated thread_priority_inversion test.

IMHO this PR should be good to go, so please have one last good look :-)

haukepetersen · 2018-06-07T11:22:35Z

wait, something went wrong when rebasing, need to re-introduce the test script...

danpetry · 2018-06-07T11:25:06Z

Ok, will have a look when it's rebased

haukepetersen · 2018-06-07T11:30:43Z

done.

danpetry

The two code convention comments should be addressed, the others don't need to be (although I think a thread timing diagram would be nice)

danpetry · 2018-06-11T12:30:43Z

tests/thread_priority_inversion_mutex/main.c

+static kernel_pid_t _pid_high;
+
+static int _result = 0;
+static int _res_addsub = 1;


This could be local to event(), although for a test it doesn't really matter

could be done, be so far we don't really use local static variables in RIOT - though I can't recall the reasons. Poking the Internet I found quotes like

Local statics may look useful, however they cause major problems when trying to port code to a multi-task/multi-threading environment, and should generally be avoided where possible.

Although this might no apply to this particular situation, IMHO it does not hurt either to have those variables in the compilation unit scope. On top I think declaring them like this makes it much easier to see the actual memory consumption of the module.

Ok cool. Thanks for the info.

danpetry · 2018-06-11T12:41:07Z

tests/thread_priority_inversion_mutex/README.md

+Event  7:  t_high - unlocking mutex
+
+   *** result: SUCCESS ***
+```


A thread diagram could be useful, either here or in the code. Perhaps with some times along the x axis

could be, but don't have the time to draw one...

also mind: this is a test and not some user-friendly example application. So once verified that it actually tests for the intended behavior its not like people dig into this app on a daily basis.

danpetry · 2018-06-11T13:10:21Z

tests/thread_priority_inversion_mutex/main.c

+{
+    /* record event */
+    _result += (_res_addsub * num);
+    _res_addsub *= -1;


Just wondering - why this approach, rather than e.g. raising a flag _success = true or something if the high priority thread finishes? That would be more readable IMO - unless there's something I'm missing re optimization or something like that?

because this approach can actually detect if all threads were run in the correct order (at least to a large extend)

Ok I see - IMO even with an order there might be a more readable way, e.g. keeping an i++ counter and comparing it with each event number or something. But that's your style/choice and it's not critical here anyway

danpetry · 2018-06-11T13:49:42Z

tests/thread_priority_inversion_mutex/README.md

+Priority inversion is a known problem in real-time systems, leading in certain
+constellations to lower priority threads indirectly blocking higher priority
+threads. This test application constructs a simple situation, where exactly this
+occurs: t_low owns a mutex, which t_high is waiting on. Now t_mid gets all the


Instead of "Now t_mid gets all the CPU time" I suggest starting this sentence with "At that point, t_mid starts and pre-empts t_low"

danpetry · 2018-06-11T13:52:52Z

tests/thread_priority_inversion_mutex/main.c

+    event(7, "t_high", "unlocking mutex");
+    mutex_unlock(&_res_mtx);
+
+  return NULL;


indentation

danpetry · 2018-06-11T14:14:27Z

tests/thread_priority_inversion_mutex/main.c

+    delay(2);
+
+    event(4, "t_mid", "starting infinite loop, potentially starving others");
+    while(1){


while (1) {

danpetry · 2018-06-11T14:19:43Z

Re the premise/design/etc of the PR, all seems good

haukepetersen · 2018-06-13T09:19:24Z

addressed comments and rebased.

danpetry · 2018-06-13T09:35:46Z

tests/thread_priority_inversion_mutex/README.md

-unlocking the mutex shared with t_high. So t_mid is indirectly preventing t_high
-from running.
+occurs: t_low owns a mutex, which t_high is waiting on. At that point, t_mid
+starts and pre-emts t_low (as t_mid has a higher priority than t_low),


sorry, should be pre-empts. Last comment

danpetry · 2018-06-13T09:37:28Z

After that, if you address Murdock and Travis complaints and squash (and rebase if necessary) then I'll merge it.

haukepetersen · 2018-06-13T14:58:42Z

fixed and squashed.

haukepetersen · 2018-06-14T14:29:17Z

fixed and squashed board blacklisting due to changed names of nucleo boards.

stale · 2019-08-10T06:08:10Z

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.

MrKevinWeiss · 2022-08-30T18:07:25Z

Is this still relevant after #17040 or can we close it?

kaspar030 · 2024-02-05T17:40:42Z

next time please state reason for closing (even if it seems obvious to you).

haukepetersen added the Area: tests Area: tests and testing framework label Aug 4, 2017

haukepetersen added this to the Release 2017.10 milestone Aug 4, 2017

haukepetersen assigned jnohlgard and kaspar030 Aug 4, 2017

haukepetersen mentioned this pull request Aug 4, 2017

scheduler: priority inversion problem #7365

Closed

haukepetersen force-pushed the add_test_prioinversion branch from 323f38d to 88fdcf8 Compare August 4, 2017 10:14

haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 4, 2017

haukepetersen mentioned this pull request Aug 4, 2017

core: add priority inheritance option #7445

Closed

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

kaspar030 requested changes Dec 21, 2017

View reviewed changes

mmkonrad mentioned this pull request Jan 6, 2018

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

Closed

smlng modified the milestones: Release 2018.01, Release 2018.04 Jan 18, 2018

haukepetersen force-pushed the add_test_prioinversion branch from 88fdcf8 to 0b08816 Compare January 30, 2018 19:03

kaspar030 removed this from the Release 2018.04 milestone Apr 16, 2018

haukepetersen force-pushed the add_test_prioinversion branch from 0b08816 to ed12c91 Compare June 7, 2018 11:16

danpetry self-requested a review June 7, 2018 11:25

haukepetersen force-pushed the add_test_prioinversion branch from 0658194 to 2f03d68 Compare June 7, 2018 11:30

haukepetersen mentioned this pull request Jun 7, 2018

tests: test for prio inversion using msg_send_recv #9306

Open

danpetry suggested changes Jun 11, 2018

View reviewed changes

haukepetersen force-pushed the add_test_prioinversion branch from 2f03d68 to 7367676 Compare June 13, 2018 09:19

danpetry suggested changes Jun 13, 2018

View reviewed changes

haukepetersen force-pushed the add_test_prioinversion branch from 7367676 to 0a3180b Compare June 13, 2018 14:58

haukepetersen added 2 commits June 14, 2018 16:28

tests: add priority inversion test using mutexes

eb73769

tests: remove depr. thread_priority_inversion test

75af392

haukepetersen force-pushed the add_test_prioinversion branch from 0a3180b to 75af392 Compare June 14, 2018 14:28

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

haukepetersen added the State: don't stale State: Tell state-bot to ignore this issue label Aug 12, 2019

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 12, 2019

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

fjmolinas mentioned this pull request Oct 23, 2021

core: implement core_mutex_priority_inheritance #17040

Merged

Teufelchen1 closed this Feb 5, 2024

tests: add test for priority inversion using muxes #7444

tests: add test for priority inversion using muxes #7444

Conversation

haukepetersen commented Aug 4, 2017

jnohlgard commented Aug 4, 2017

haukepetersen commented Aug 4, 2017

haukepetersen commented Aug 28, 2017

smlng commented Aug 28, 2017 • edited

haukepetersen commented Aug 28, 2017

smlng commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlng commented Jan 15, 2018

haukepetersen commented Jan 30, 2018

smlng commented Jan 31, 2018

haukepetersen commented Jan 31, 2018

smlng commented Jan 31, 2018

haukepetersen commented Feb 6, 2018

haukepetersen commented Jun 7, 2018

haukepetersen commented Jun 7, 2018

danpetry commented Jun 7, 2018

haukepetersen commented Jun 7, 2018

danpetry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpetry commented Jun 11, 2018

haukepetersen commented Jun 13, 2018

Choose a reason for hiding this comment

danpetry commented Jun 13, 2018

haukepetersen commented Jun 13, 2018

haukepetersen commented Jun 14, 2018

stale bot commented Aug 10, 2019

MrKevinWeiss commented Aug 30, 2022

kaspar030 commented Feb 5, 2024

smlng commented Aug 28, 2017 •

edited