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.

Sign up for GitHub

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

Jump to bottom

posix: sched: ensure min and max priority are schedulable #57161

Merged
merged 4 commits into from
Apr 29, 2023
Merged

posix: sched: ensure min and max priority are schedulable #57161

merged 4 commits into from
Apr 29, 2023

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Apr 24, 2023 •
edited
Loading

Previously, with preemptive scheduling, minimum priority threads were not schedulable. The linked issue appears to highlight an off-by-one error as well, which I believe should be fixed here.

Also add SCHED_OTHER support, which is mandatory (separate commits)

Fixes #56729

@cfriedt cfriedt force-pushed the issues/56729/posix-SCHED-RR-valid-thread-priorities branch from dbe04ca to b32d85d Compare April 24, 2023 02:38
@cfriedt cfriedt requested a review from stephanosio April 24, 2023 02:38
@cfriedt cfriedt marked this pull request as ready for review April 24, 2023 09:44
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Apr 24, 2023
@cfriedt
Copy link
Member Author

cfriedt commented Apr 24, 2023

Cc @ambroise-arm

@cfriedt cfriedt added the area: Tests Issues related to a particular existing or missing test label Apr 24, 2023
@cfriedt cfriedt requested a review from nashif April 24, 2023 13:20
jgl-meta
jgl-meta previously approved these changes Apr 24, 2023
View reviewed changes
lib/posix/pthread.c Outdated Show resolved Hide resolved
lib/posix/pthread_sched.c Show resolved Hide resolved
@cfriedt cfriedt force-pushed the issues/56729/posix-SCHED-RR-valid-thread-priorities branch 2 times, most recently from 62ac742 to bcbc340 Compare April 25, 2023 01:20
@cfriedt
posix: sched: ensure min and max priority are schedulable
9d9cb40 
Previously, there was an off-by-one error for SCHED_RR.

Fixes zephyrproject-rtos#56729

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt force-pushed the issues/56729/posix-SCHED-RR-valid-thread-priorities branch from bcbc340 to 3cd72cb Compare April 25, 2023 01:28
@cfriedt cfriedt requested a review from jgl-meta April 25, 2023 01:28
@cfriedt cfriedt force-pushed the issues/56729/posix-SCHED-RR-valid-thread-priorities branch 3 times, most recently from 3fb5590 to 529134f Compare April 25, 2023 11:46
cfriedt
cfriedt commented Apr 25, 2023
View reviewed changes
lib/posix/pthread.c
@@ -14,16 +14,15 @@
#include <zephyr/sys/slist.h>

#include "posix_internal.h"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, but the posix_internal.h header is unfortunately sensitive to include order 😢 - this should be fixed at some point (#57232)

@cfriedt
Copy link
Member Author

cfriedt commented Apr 25, 2023

Oh wow - something broke all of the tests.. it's not even rendering in GH 😅

@stephanosio
Copy link
Member

Oh wow - something broke all of the tests.. it's not even rendering in GH sweat_smile

In file included from /__w/zephyr/zephyr/include/zephyr/posix/pthread.h:15,
                 from /__w/zephyr/zephyr/tests/posix/common/src/mqueue.c:13:
/__w/zephyr/zephyr/include/zephyr/posix/sched.h:26: error: "SCHED_OTHER" redefined [-Werror]
   26 | #define SCHED_OTHER 3
      | 
In file included from /opt/toolchains/zephyr-sdk-0.16.0/arm-zephyr-eabi/arm-zephyr-eabi/sys-include/sys/_pthreadtypes.h:23,
                 from /opt/toolchains/zephyr-sdk-0.16.0/arm-zephyr-eabi/arm-zephyr-eabi/sys-include/sys/types.h:223,
                 from /opt/toolchains/zephyr-sdk-0.16.0/arm-zephyr-eabi/arm-zephyr-eabi/sys-include/stdio.h:61,
                 from /__w/zephyr/zephyr/subsys/testsuite/ztest/include/zephyr/ztest_assert.h:18,
                 from /__w/zephyr/zephyr/subsys/testsuite/ztest/include/zephyr/ztest.h:49,
                 from /__w/zephyr/zephyr/tests/posix/common/src/mqueue.c:7:
/opt/toolchains/zephyr-sdk-0.16.0/arm-zephyr-eabi/arm-zephyr-eabi/sys-include/sys/sched.h:35: note: this is the location of the previous definition
   35 | #define SCHED_OTHER    0
      | 
cc1: all warnings being treated as errors

@cfriedt cfriedt force-pushed the issues/56729/posix-SCHED-RR-valid-thread-priorities branch from 529134f to f013a34 Compare April 26, 2023 03:41
@cfriedt
Copy link
Member Author

cfriedt commented Apr 26, 2023

/opt/toolchains/zephyr-sdk-0.16.0/arm-zephyr-eabi/arm-zephyr-eabi/sys-include/sys/sched.h:35: note: this is the location of the previous definition
35 | #define SCHED_OTHER 0

You win this time ARM toolchain...

@cfriedt cfriedt force-pushed the issues/56729/posix-SCHED-RR-valid-thread-priorities branch from f013a34 to c0f670a Compare April 26, 2023 11:49
@cfriedt
Copy link
Member Author

cfriedt commented Apr 26, 2023 •
edited
Loading

@jgl-meta, @stephanosio, @keith-packard @galak - should be good to go if you're up for reviewing

@cfriedt cfriedt requested review from keith-packard and galak April 26, 2023 23:38
jgl-meta
jgl-meta previously approved these changes Apr 27, 2023
View reviewed changes
@henrikbrixandersen henrikbrixandersen removed their request for review April 28, 2023 12:02
cfriedt added 3 commits April 29, 2023 07:43
@cfriedt
tests: posix: ensure that min and max priority are schedulable
deef65d 
Verify that threads are actually schedulable for min and max
scheduler priority for both `SCHED_RR` (preemptive) and
`SCHED_FIFO` (cooperative).

Fixes zephyrproject-rtos#56729

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt
posix: sched: add support for SCHED_OTHER
3912781 
The `SCHED_OTHER` scheduling priority is mandatory as part of
POSIX. It must be numerically distinct from `SCHED_FIFO`,
`SCHED_RR`, and `SCHED_SPORADIC`, but is implementation-
defined and may behave identically to `SCHED_FIFO` or
`SCHED_RR`.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt
tests: posix: test support for SCHED_OTHER
ad6200e 
Ensure we test `SCHED_OTHER` functionality along with other
scheduling policies and priorities.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt force-pushed the issues/56729/posix-SCHED-RR-valid-thread-priorities branch from c0f670a to ad6200e Compare April 29, 2023 11:43
@cfriedt
Copy link
Member Author

cfriedt commented Apr 29, 2023 •
edited
Loading

  • reuse create_thread1() instead of defining a second no-op pthread function in tests/posix/common/src/pthread.c

@cfriedt cfriedt requested a review from jgl-meta April 29, 2023 11:46
@stephanosio stephanosio requested a review from andyross April 29, 2023 15:30
stephanosio
stephanosio approved these changes Apr 29, 2023
View reviewed changes
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unable to find anything obviously wrong with the changes in this PR.

@jgl-meta jgl-meta merged commit 27c25e9 into zephyrproject-rtos:main Apr 29, 2023
@cfriedt cfriedt deleted the issues/56729/posix-SCHED-RR-valid-thread-priorities branch April 29, 2023 20:28
@cfriedt cfriedt added the backport v2.7-branch Request backport to the v2.7-branch label Apr 29, 2023
@zephyrbot zephyrbot mentioned this pull request Apr 29, 2023
Closed
This was referenced Apr 30, 2023
Closed
Merged
@PatrickM-ZS PatrickM-ZS mentioned this pull request Jul 17, 2023
Merged
@golowanow golowanow mentioned this pull request Dec 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanosio stephanosio stephanosio approved these changes

@jgl-meta jgl-meta jgl-meta approved these changes

@nashif nashif Awaiting requested review from nashif

@keith-packard keith-packard Awaiting requested review from keith-packard

@galak galak Awaiting requested review from galak

@andyross andyross Awaiting requested review from andyross

Assignees

@cfriedt cfriedt

Labels
area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test backport v2.7-branch Request backport to the v2.7-branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

posix: SCHED_RR valid thread priorities
4 participants
@cfriedt @stephanosio @jgl-meta @zephyrbot