kernel: sched: use _is_thread_ready() in should_preempt() #8126
Conversation
mike-scott
added
area: Kernel
bug
|
@andyross Please take a look. |
|
@andyross I'm going to say "I told you so" here. Not really to be mean, but because this took another 24 hours of my time to debug. I was using _is_thread_ready() in the patch I originally posted to fix the scheduler spin. When integrated into the rebase you were doing, you swapped to using _is_thread_prevented_from_running() instead. Then we had a discussion about it and I let you sway me into believing that _is_thread_prevented_from_running() was going to work. So, "I told you so". :) |
|
There could be an argument as to why k_sleep() doesn't pend() a thread. But, I think at some point we have to fix what we have for v1.12. |
|
Looking into the sanity check failures, they seem very appropriate. |
There was a problem hiding this comment.
Actually I stumbled on the k_sleep() issue from the other direction this morning and literally started working on a change to fix it to use the pend API directly. That would obviate the need for this, but is a slightly more invasive change that can wait for 1.13 if this fixes the issues.
To be clear, though: this fixes your issues, right?
|
Grr... except it looks like something is failing with this applied. I'll bang on this some tonight when I get a chance too. I totally buy that this is the right issue. If you're curious about the fix-from-the-other-side I started working on this morning (it doesn't work yet either!) and want to try something like it, it's in andyross@1ca847c |
|
@andyross I can't confirm this fixes the issue entirely, but so far so good (only a few hours into the test tho). |
|
@andyross nope have a few devices fall offline. Will hook a debugger up after dinner. (not that k_sleep isn't an issue, it just isn't the only issue) |
kernel/sched.c
Outdated
| @@ -122,7 +122,7 @@ static int should_preempt(struct k_thread *th, int preempt_ok) | |||
| } | |||
|
|
|||
| /* Or if we're pended/suspended/dummy (duh) */ | |||
| if (!_current || _is_thread_prevented_from_running(_current)) { | |||
| if (!_current || _is_thread_ready(_current)) { | |||
There was a problem hiding this comment.
Logic is backwards here. If !_is_thread_ready() then we should preempt.
We are using _is_thread_prevented_from_running() to see if the _current thread can be preempted in should_preempt(). The idea being that even if the _current thread is a high priority coop thread, we can still preempt it when it's pending, suspended, etc. This does not take into account if the thread is sleeping. k_sleep() merely removes the thread from the ready_q and calls Swap(). The scheduler will swap away from the thread temporarily and then on the next cycle get stuck to the sleeping thread for however long the sleep timeout is, doing exactly nothing because other functions like _ready_thread() use _is_thread_ready() as a check before proceeding. We should use !_is_thread_ready() to take into account when threads are waiting on a timer, and let other threads run in the meantime. Signed-off-by: Michael Scott <[email protected]>
mike-scott
force-pushed
the
master-fix-k_sleep-spin
branch
from
20c890d to
03cd198
Compare
Codecov Report
@@ Coverage Diff @@
## master #8126 +/- ##
=======================================
Coverage 64.54% 64.54%
=======================================
Files 420 420
Lines 40142 40142
Branches 6765 6765
=======================================
Hits 25911 25911
Misses 11110 11110
Partials 3121 3121
Continue to review full report at Codecov.
|
We are using _is_thread_prevented_from_running() to see if the
_current thread can be preempted in should_preempt(). The idea
being that even if the _current thread is a high priority coop
thread, we can still preempt it when it's pending, suspended,
etc.
This does not take into account if the thread is sleeping.
k_sleep() merely removes the thread from the ready_q and calls
Swap(). The scheduler will swap away from the thread temporarily
and then on the next cycle get stuck to the sleeping thread for
however long the sleep timeout is, doing exactly nothing because
other functions like _ready_thread() use _is_thread_ready() as a
check before proceeding.
We should use !_is_thread_ready() to take into account when threads
are waiting on a timer, and let other threads run in the meantime.
Fixes: #8128
Signed-off-by: Michael Scott [email protected]