Skip to content

core/sched: add sched_change_priority()#17093

Merged
benpicco merged 3 commits intoRIOT-OS:masterfrom
maribu:core/change_prio
Jan 28, 2022
Merged

core/sched: add sched_change_priority()#17093
benpicco merged 3 commits intoRIOT-OS:masterfrom
maribu:core/change_prio

Conversation

@maribu
Copy link
Copy Markdown
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
kaspar030
kaspar030 reviewed Nov 1, 2021
View reviewed changes
@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
Copy Markdown
Contributor

kaspar030 commented Nov 2, 2021

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
Copy Markdown
Contributor

fjmolinas commented Nov 2, 2021

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

@maribu
Copy link
Copy Markdown
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.

benpicco
benpicco approved these changes Nov 12, 2021
View reviewed changes
Copy link
Copy Markdown
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
@benpicco benpicco requested a review from kfessel December 14, 2021 09:28
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Dec 17, 2021

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. :)

kaspar030
kaspar030 approved these changes Dec 17, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

kaspar030 commented Dec 17, 2021 •
edited
Loading

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
Copy Markdown
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

@chrysn chrysn mentioned this pull request Jan 20, 2022
Open
fjmolinas
fjmolinas reviewed Jan 27, 2022
View reviewed changes
Copy link
Copy Markdown
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...

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 28, 2022

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

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jan 28, 2022

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 enabled auto-merge January 28, 2022 16:45
@benpicco benpicco merged commit 5a57dec into RIOT-OS:master Jan 28, 2022
@OlegHahm OlegHahm mentioned this pull request Mar 9, 2022
Closed
@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

Reviewers

@fjmolinas fjmolinas fjmolinas left review comments

@benpicco benpicco benpicco approved these changes

@kaspar030 kaspar030 kaspar030 approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@smlng smlng Awaiting requested review from smlng

@bergzand bergzand Awaiting requested review from bergzand

@chrysn chrysn Awaiting requested review from chrysn

@kfessel kfessel Awaiting requested review from kfessel

Assignees

No one assigned

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

Milestone

Release 2022.04

Development

Successfully merging this pull request may close these issues.

7 participants

@maribu @kaspar030 @fjmolinas @kfessel @benpicco @emmanuelsearch @haukepetersen