scheduler: threads using k_sleep can be _swap()'d back too early

[mike-scott]

Scheduler uses a function "should_preempt()" to determine if the _current thread should be _swap()'d away from. Part of that calculation uses a function _is_thread_prevented_from_running() which evaluates if the _current thread is "runnable".

Currently, the k_sleep() API doesn't make a thread look like it isn't runnable other than the following expression being true:
thread->base.timeout.delta_ticks_from_prev != _INACTIVE.

Originally, I felt that _is_thread_ready() would be a better replacement for _is_thread_prevented_from_running() as this takes into account active timeouts and would handle the k_sleep() case. But, a patch which changed this behavior is causing problems with the scheduler tests, so more research is needed.

[mike-scott]

@nashif @andyross I'm going to try and document every "issue" concerning the scheduler over the next day so we can see all of the various symptoms still in play. From there we can de-dupe as items are debugged.

This is issue #1. I attempted a patch:
#8126

But it's causing other issues, currently.

[mike-scott]

Found the bug in #8126 and I'm running my own personal soak test with 13 IoT devices to see how well they perform.

[mike-scott]

@andyross I ran a local sanitycheck with #8126 and it's passing now as well.

[andyross]

So.... looking at this from the context of trying to make it happen in a test, I'm flipping back. I thought I had a test case for this (about to submit) that showed the failure, but that was a bad assert. k_sleep() actually works correctly right now, though the logic is distressingly subtle:

• Sleep does not flag the thread PENDING or otherwise unrunable, but it does call _remove_thread_from_ready_q()

• _r_t_f_r_q() then does the removal, and (because _current is the thread being removed) calls update_cache() with preempt_ok as TRUE, indicating that the cache should be updated to reflect whatever next thread was found even if the current thread is otherwise nonpreemptible.

• update_cache() then calls next_up() to retrieve the next thread, which because _current was removed is guaranteed to be something else (an idle thread if nothing else is available).

• Then it calls should_preempt() to see if it should replace _current with whatever it found, which will be 1 because preempt_ok is a short-circuit case there.

• Then, having updated the cache, we return back into k_sleep() and call _Swap(), guaranteeing we stop running and switch to the cache thrad. And because we are no longer in the run queue, we are guaranteed to

Basically, I don't see how this can be the bug. The k_sleep() state management works now for, effecitvely, the same reason it worked before the change. I agree we want to clean it up (ideally to make PENDING and "sleeping" the same state), but I don't see a logic failure.

All that said, I'm leaving the +1 in place and agree we should merge this. The change is correct logically, easily reviewed, and I'm willing to buy that it fixes some other yet-more-subtle state somewhere else, given that it seems to be resolving your problems.

[andyross]

FWIW: if you want to catch the presumptive bug closer to the act, one suggestion would be to enable assertions (if possible, otherwise just a manual test + log/abort/whatever) in should_preempt() that looks like:

__ASSERT_NO_MSG(_is_thread_prevented_from_running(_current) != _is_thread_ready(_current));

I'd be very curious to see the results. A pretty careful re-read and grep of every path I can see to _add_timeout() shows that it's never possible for a thread to have a timeout in a case where it gets inspected as the top of the run queue.

[mike-scott]

@andyross I agree with your assessment that k_sleep does manage to sort of work initially.

However, with patch applied, I've had no oops or hangs in the 5 hours of testing on several devices. Previously, I would have seen at least 1 device go offline by now.

I'll setup an assert to see if I can catch the path that is causing errors.

[pfalcon]

I'll setup an assert to see if I can catch the path that is causing errors.

Not sure if this should be reopened then.