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

kernel/sched: Fix preemption logic #8077

Merged
merged 1 commit into from
May 31, 2018

Conversation

andyross
Copy link
Contributor

@andyross andyross commented May 31, 2018

The should_preempt() code was catching some of the "unrunnable" cases
but not all of them, opening the possibility of failing to preempt a
just-pended thread and thus waking it up synchronously. There are
reports of this causing spin loops over k_poll() in the network stack
work queues (see #8049).

Note that the previous _is_dummy() call is folded into (the somewhat
verbosely named) _is_thread_prevented_from_running(), and that the
order of tests has been changed/optimized to hopefully catch common
cases earlier.

Fixes #8049

Suggested-by: Michael Scott michael@opensourcefoundries.com
Signed-off-by: Andy Ross andrew.j.ross@intel.com

The should_preempt() code was catching some of the "unrunnable" cases
but not all of them, opening the possibility of failing to preempt a
just-pended thread and thus waking it up synchronously.  There are
reports of this causing spin loops over k_poll() in the network stack
work queues (see zephyrproject-rtos#8049).

Note that the previous _is_dummy() call is folded into (the somewhat
verbosely named) _is_thread_prevented_from_running(), and that the
order of tests has been changed/optimized to hopefully catch common
cases earlier.

Suggested-by: Michael Scott <michael@opensourcefoundries.com>
Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Copy link
Contributor

@mike-scott mike-scott left a comment

Choose a reason for hiding this comment

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

Initial testing of this LGTM. Letting it run for a while on HW to see if it stalls out anywhere.

@mike-scott
Copy link
Contributor

Can we add this to the PR description?
Fixes: #8049

@codecov-io
Copy link

Codecov Report

Merging #8077 into master will not change coverage.
The diff coverage is 75%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8077   +/-   ##
=======================================
  Coverage   64.55%   64.55%           
=======================================
  Files         420      420           
  Lines       40141    40141           
  Branches     6763     6763           
=======================================
  Hits        25913    25913           
  Misses      11106    11106           
  Partials     3122     3122
Impacted Files Coverage Δ
kernel/sched.c 81.17% <75%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c84b37b...749b4ff. Read the comment docs.

@andrewboie andrewboie requested a review from nashif May 31, 2018 19:14
Copy link
Collaborator

@lpereira lpereira left a comment

Choose a reason for hiding this comment

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

LGTM

@ulfalizer
Copy link
Collaborator

This code actually is performance-critical, though this close to release I'd prefer to keep this clearer rather than optimized. Scheduler performance is an item for 1.13 for sure (we've regressed a bit, though this patch turns out to repair most of the damage). In this case though I'd expect the compiler to have figured out the proper boolean path here. Note that both _is_idle() and is_preempt() are comparisons on the thread priority, so it might be able to fold them together.

Yeah, I just did a dumb local check. GCC always simplifies the non-SMP return thread == _idle_thread case, but the SMP case is a bit trickier. I don't know the context here though. Just something random I spotted.

@nashif nashif merged commit 43553da into zephyrproject-rtos:master May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants