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

core/sched: add sched_change_priority() #17093

Merged
merged 3 commits into from
Jan 28, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 1, 2021

Contribution description

This PR is a rebase of the sched_change_priority() function from #7445 (introduced in commit 976d17f) with some changes:

  1. No module required to unlock this
    • The linker does a fine job at garbage collecting the function, if it is unused
  2. The function works as expected in corner cases
    • If the modified thread is the running one, this will work
    • If the modified thread is pending and becomes a higher priority than the active one, it will now preempt the running one

Testing procedure

A test application was added that contains an automatic test. E.g.

$ make BOARD=nucleo-f767zi -C tests/sched_change_priority flash test
[...]
socat - open:/dev/ttyACM0,b115200,echo=0,raw 
main(): This is RIOT! (Version: 2022.01-devel-305-ga9249-core/change_prio)
[t3] Setting my priority to THREAD_PRIORITY_MAIN + 2 = 9
[main] Use shell command "nice" to increase prio of t3.
[main] If it works, it will run again. The "hint" cmd can be useful.
> main(): This is RIOT! (Version: 2022.01-devel-305-ga9249-core/change_prio)
[t3] Setting my priority to THREAD_PRIORITY_MAIN + 2 = 9
[main] Use shell command "nice" to increase prio of t3.
[main] If it works, it will run again. The "hint" cmd can be useful.
> hint

> 
> hint
Run "nice 3 6"
> nice 3 6
nice 3 6
[t3] Running again.
make: Leaving directory '/home/maribu/Repos/software/RIOT/tests/sched_change_priority'

$ echo $?
0

Issues/PRs references

#7445

@github-actions github-actions bot added Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: sys Area: System Area: tests Area: tests and testing framework labels Nov 1, 2021
@maribu maribu added Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: new feature The issue requests / The PR implemements a new feature for RIOT and removed Area: tests Area: tests and testing framework Area: build system Area: Build system Area: sys Area: System labels Nov 1, 2021
core/sched.c Show resolved Hide resolved
@github-actions github-actions bot added Area: build system Area: Build system Area: sys Area: System Area: tests Area: tests and testing framework labels Nov 2, 2021
@kaspar030
Copy link
Contributor

LGTM, let's get some experience for this.
Please uncrustify and squash!

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 2, 2021
@fjmolinas fjmolinas 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 Nov 2, 2021
@fjmolinas
Copy link
Contributor

This might need a rebase to pass static checks when #17104 is in...

Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
@maribu
Copy link
Member Author

maribu commented Nov 2, 2021

This might need a rebase to pass static checks when #17104 is in...

Did so just in case. Also added a Makefile.ci and squashed it right in.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

If @kaspar030 is OK with it on a conceptual level, the change looks goodⁿᶦᶜᵉ to me.

@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
@kaspar030
Copy link
Contributor

There's a slight performance drop (bench_thread_flags_pingpong on nrf52840dk):

main(): This is RIOT! (Version: 2022.01-devel-330-gea1f4-HEAD)
main starting
{ "result" : 112675, "ticks" : 568 }
Help: Press s to start test, r to print it is ready
START
main(): This is RIOT! (Version: 2022.01-devel-333-g034bdf-pr17093)
main starting
{ "result" : 111304, "ticks" : 575 }

(HEAD is the master fork point).

It's ~1%. So room for getting some of that back in the future. :)

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030
Copy link
Contributor

kaspar030 commented Dec 17, 2021

Let me suggest a rebase to get a current CI run. triggering CI.

@kaspar030 kaspar030 added CI: run tests If set, CI server will run tests on hardware for the labeled PR 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 17, 2021
@kfessel
Copy link
Contributor

kfessel commented Dec 20, 2021

there seems to be a slight problem with that Python f-strings (introduced in Python 3.6)

The only test user in master of that python feature in master is the test gnrc_dhcpv6_client, but that is a test with config and therefore not run by murdock

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

Let's not stall this one because the python version in rpi is old...

tests/sched_change_priority/tests/01-run.py Outdated Show resolved Hide resolved
@maribu
Copy link
Member Author

maribu commented Jan 28, 2022

I squashed the suggested fix right in to save a CI cycle.

@benpicco
Copy link
Contributor

Only the usual run_test/tests/pkg_mbedtls/samr21-xpro:gnu is failing.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 28, 2022
@benpicco benpicco merged commit 5a57dec into RIOT-OS:master Jan 28, 2022
@maribu maribu deleted the core/change_prio branch May 27, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK 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.

7 participants