added test program for priority inversion problem by geith · Pull Request #7372 · RIOT-OS/RIOT

geith · 2017-07-17T11:24:25Z

added simple test program for priority inversion problem as discussed in #7365

smlng

Thanks for tackling this issue and provide a suitable test! Some minor corrections, see comment.

Please also add a README.md to shortly describe what the test does and what output to expect in case of either success or failure.

smlng · 2017-07-17T13:31:16Z

tests/thread_priority_inversion/Makefile

+USEMODULE += shell
+USEMODULE += shell_commands
+USEMODULE += ps
+USEMODULE += xtimer


only xtimer is needed for your test, the 3 modules above can safely be removed.

smlng · 2017-07-17T13:31:53Z

tests/thread_priority_inversion/Makefile

@@ -0,0 +1,25 @@
+# name of your application
+APPLICATION = Scheduling-Test


this should be the same as the directory name -> thread_priority_inversion

smlng · 2017-07-17T13:34:11Z

tests/thread_priority_inversion/main.c

+{
+    /* starting working loop immediately */
+	while(1){
+		printf("t_low: allocating resource...\n");


replace printf withputs here and below. puts can output static strings only, but is less expensive than printf.

IIRC GCC should replace printf by puts automatically when optimizing if the string is static and ends in a newline.

geith · 2017-07-17T18:01:33Z

requested changes done.

smlng

sorry for leaving my review unattended for such a long time. However, please revise coding style, you seem to use tabs (size 8) instead of spaces with indention 4 spaces per level. Please also review coding style in the Wiki

smlng · 2017-12-06T21:09:19Z

tests/thread_priority_inversion/README.md

+
+This application uses three threads for demonstrating the
+priority inversion problem. In theory, the highest priority thread (**t_high**)
+should periodically and produce some output:


missing word? ... should [be scheduled] periodically ...

smlng · 2017-12-06T21:09:47Z

tests/thread_priority_inversion/README.md

+```
+During this phase of 1s, **t_high** lockes the mutex **res_mtx** which
+represents a shared ressource. After unlocking the mutex, **t_high** waits 1s
+before restaring tis cycle again.


typo: /tis/its/

smlng · 2017-12-06T21:10:09Z

tests/thread_priority_inversion/README.md

+before restaring tis cycle again.
+
+A low priority thread **t_low** is doing the same. Since both threads sharing
+the same resource **res_mtx**, they have to wait for wach other until the mutex


typo: /wach/each/

smlng · 2017-12-06T21:10:45Z

tests/thread_priority_inversion/README.md

+
+After 3s, a third thread with medium priority (**t_mid**) is started. This
+thread does not touch **res_mtx**, but it runs an infinite loop without leaving
+come CPU time to lower priority tasks. This prevents **t_low** from freeing the


typo: /come/some/

smlng · 2017-12-06T21:11:41Z

tests/thread_priority_inversion/main.c

+void *t_low_handler(void *arg)
+{
+    /* starting working loop immediately */
+	while(1){


indention seems off

smlng · 2017-12-06T21:11:56Z

tests/thread_priority_inversion/main.c

+void *t_mid_handler(void *arg)
+{
+    /* starting working loop after 3s */
+	xtimer_sleep(3);


indention seem off

smlng · 2017-12-06T21:14:46Z

also consider squashing

geith · 2017-12-07T15:22:43Z

requested changes done...

smlng · 2017-12-07T19:24:41Z

Looks better now, however Murdock isn't happy - some minor issues with unused variables (add (void)arg; for instance) and a whitespace error in Makefile. Please fix, Pre-ACK.

geith · 2017-12-11T13:37:46Z

unused variables and makefile issues fixed...

smlng · 2017-12-11T16:49:27Z

we are nearly there, you need to add the following directive to the Makefile of your test

BOARD_INSUFFICIENT_MEMORY := nucleo32-f031 nucleo32-f042

that should fix last problem!

geith · 2017-12-12T12:31:52Z

done.

smlng

on a second review round I found some minor issues.

smlng · 2017-12-13T19:44:41Z

tests/thread_priority_inversion/main.c

+    (void) arg;
+
+    /* starting working loop immediately */
+    while(1){


coding style here and 2x below: add missing spaces around parentheses, should be while(1) {

smlng · 2017-12-13T19:44:48Z

tests/thread_priority_inversion/main.c

+    xtimer_sleep(3);
+
+    puts("t_mid: doing some stupid stuff...");
+    while(1){


smlng · 2017-12-13T19:44:55Z

tests/thread_priority_inversion/main.c

+
+    /* starting working loop after 500 ms */
+    xtimer_usleep(500);
+    while(1){


abd here, too

smlng · 2017-12-13T19:46:17Z

tests/thread_priority_inversion/main.c

+    (void) arg;
+
+    /* starting working loop after 500 ms */
+    xtimer_usleep(500);


this actually only 500 microseconds, if you want milliseconds change to: 500U * US_PER_MS

smlng · 2017-12-13T19:47:54Z

tests/thread_priority_inversion/main.c

+
+mutex_t res_mtx;
+
+char stack_high[THREAD_STACKSIZE_MAIN];


IMHO THREAD_STACKSIZE_DEFAULT will be sufficient, too and will lower required memory considerably.

geith · 2017-12-15T14:34:38Z

changes done; compiles now for the nucleo32-f042 too (but still not for the nucleo32-f031)

smlng · 2017-12-18T10:43:00Z

tested on macOS and PhyNode works (or actually for now: fails) as expected 😉

smlng · 2017-12-21T10:45:51Z

ACK & GO

smlng · 2017-12-21T10:49:12Z

ah damn, @geith can you reword you commit message? We typically have the module name first, i.e., tests: add program for priority inversion problem would fit here.

smlng · 2017-12-21T10:49:42Z

otherwise we are clear to merge here ...

geith · 2017-12-21T11:01:43Z

done ;-)

smlng suggested changes Jul 17, 2017

View reviewed changes

smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework labels Jul 17, 2017

smlng added this to the Release 2017.10 milestone Jul 17, 2017

jnohlgard mentioned this pull request Aug 4, 2017

tests: add test for priority inversion using muxes #7444

Closed

haukepetersen mentioned this pull request Aug 4, 2017

core: add priority inheritance option #7445

Closed

miri64 assigned smlng Sep 19, 2017

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

smlng approved these changes Dec 6, 2017

View reviewed changes

smlng suggested changes Dec 6, 2017

View reviewed changes

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 6, 2017

geith force-pushed the priority_inversion branch 2 times, most recently from fdd7084 to 1deb82b Compare December 7, 2017 15:21

smlng approved these changes Dec 7, 2017

View reviewed changes

geith force-pushed the priority_inversion branch 2 times, most recently from 84f6813 to 0f43ccd Compare December 11, 2017 13:28

geith force-pushed the priority_inversion branch from 0f43ccd to 9621eb3 Compare December 12, 2017 12:25

smlng suggested changes Dec 13, 2017

View reviewed changes

geith force-pushed the priority_inversion branch from 9621eb3 to bbbdb9f Compare December 15, 2017 14:25

smlng approved these changes Dec 18, 2017

View reviewed changes

smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 21, 2017

tests: add program for priority inversion

1ebecc7

geith force-pushed the priority_inversion branch from bbbdb9f to 1ebecc7 Compare December 21, 2017 10:51

smlng merged commit 1003cb9 into RIOT-OS:master Dec 21, 2017

mmkonrad mentioned this pull request Jan 6, 2018

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

Closed

		@@ -0,0 +1,25 @@
		# name of your application
		APPLICATION = Scheduling-Test


		mutex_t res_mtx;

		char stack_high[THREAD_STACKSIZE_MAIN];

Conversation

geith commented Jul 17, 2017

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geith commented Jul 17, 2017

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Dec 6, 2017

Uh oh!

geith commented Dec 7, 2017

Uh oh!

smlng commented Dec 7, 2017

Uh oh!

geith commented Dec 11, 2017

Uh oh!

smlng commented Dec 11, 2017

Uh oh!

geith commented Dec 12, 2017

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geith commented Dec 15, 2017

Uh oh!

smlng commented Dec 18, 2017

Uh oh!

smlng commented Dec 21, 2017

Uh oh!

smlng commented Dec 21, 2017

Uh oh!

smlng commented Dec 21, 2017

Uh oh!

geith commented Dec 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants