michaelh/zephyr
1
0
Fork 0
Code Releases Activity

kernel/sched: Move thread suspend and abort under the scheduler lock

Browse source 
Historically, these routines were placed in thread.c and would use the
scheduler via exported, synchronized functions (e.g. "remove from
ready queue").  But those steps were very fine grained, and there were
races where the thread could be seen by other contexts (in particular
under SMP) in an intermediate state.  It's not completely clear to me
that any of these were fatal bugs, but it's very hard to prove they
weren't.

At best, this is fragile.  Move the z_thread_single_suspend/abort()
functions into the scheduler and do the scheduler logic in a single
critical section.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This commit is contained in:
Andy Ross 2020-01-07 09:58:46 -08:00 • committed by Ioannis Glaropoulos
commit 8bdabcc46b
3 changed files with 79 additions and 70 deletions
Download patch file Download diff file Expand all files Collapse all files

77
kernel/sched.c
View file

@ -383,6 +383,76 @@ void z_move_thread_to_end_of_prio_q(struct k_thread *thread)
}
}
void z_thread_single_suspend(struct k_thread *thread)
{
(void)z_abort_thread_timeout(thread);
LOCKED(&sched_spinlock) {
if (z_is_thread_queued(thread)) {
_priq_run_remove(&_kernel.ready_q.runq, thread);
z_mark_thread_as_not_queued(thread);
}
z_mark_thread_as_suspended(thread);
update_cache(thread == _current);
}
if (thread == _current) {
z_reschedule_unlocked();
}
}
static _wait_q_t *pended_on(struct k_thread *thread)
{
__ASSERT_NO_MSG(thread->base.pended_on);
return thread->base.pended_on;
}
void z_thread_single_abort(struct k_thread *thread)
{
if (thread->fn_abort != NULL) {
thread->fn_abort();
}
(void)z_abort_thread_timeout(thread);
if (IS_ENABLED(CONFIG_SMP)) {
z_sched_abort(thread);
}
LOCKED(&sched_spinlock) {
if (z_is_thread_ready(thread)) {
if (z_is_thread_queued(thread)) {
_priq_run_remove(&_kernel.ready_q.runq,
thread);
z_mark_thread_as_not_queued(thread);
}
update_cache(thread == _current);
} else {
if (z_is_thread_pending(thread)) {
_priq_wait_remove(&pended_on(thread)->waitq,
thread);
z_mark_thread_as_not_pending(thread);
thread->base.pended_on = NULL;
}
}
thread->base.thread_state |= _THREAD_DEAD;
}
sys_trace_thread_abort(thread);
#ifdef CONFIG_USERSPACE
/* Clear initialized state so that this thread object may be re-used
* and triggers errors if API calls are made on it from user threads
*/
z_object_uninit(thread->stack_obj);
z_object_uninit(thread);
/* Revoke permissions on thread's ID so that it may be recycled */
z_thread_perms_all_clear(thread);
#endif
}
void z_remove_thread_from_ready_q(struct k_thread *thread)
{
LOCKED(&sched_spinlock) {
@ -428,13 +498,6 @@ void z_pend_thread(struct k_thread *thread, _wait_q_t *wait_q, s32_t timeout)
pend(thread, wait_q, timeout);
}
static _wait_q_t *pended_on(struct k_thread *thread)
{
__ASSERT_NO_MSG(thread->base.pended_on);
return thread->base.pended_on;
}
ALWAYS_INLINE struct k_thread *z_find_first_thread_to_unpend(_wait_q_t *wait_q,
struct k_thread *from)
{

52
kernel/thread.c
View file

@ -671,20 +671,7 @@ k_tid_t z_vrfy_k_thread_create(struct k_thread *new_thread,
#endif /* CONFIG_USERSPACE */
#endif /* CONFIG_MULTITHREADING */
void z_thread_single_suspend(struct k_thread *thread)
{
if (z_is_thread_ready(thread)) {
z_remove_thread_from_ready_q(thread);
}
(void)z_abort_thread_timeout(thread);
z_mark_thread_as_suspended(thread);
if (thread == _current) {
z_reschedule_unlocked();
}
}
extern void z_thread_single_suspend(struct k_thread *thread);
void z_impl_k_thread_suspend(struct k_thread *thread)
{
@ -735,43 +722,6 @@ static inline void z_vrfy_k_thread_resume(struct k_thread *thread)
#include <syscalls/k_thread_resume_mrsh.c>
#endif
void z_thread_single_abort(struct k_thread *thread)
{
if (thread->fn_abort != NULL) {
thread->fn_abort();
}
if (IS_ENABLED(CONFIG_SMP)) {
z_sched_abort(thread);
}
if (z_is_thread_ready(thread)) {
z_remove_thread_from_ready_q(thread);
} else {
if (z_is_thread_pending(thread)) {
z_unpend_thread_no_timeout(thread);
}
if (z_is_thread_timeout_active(thread)) {
(void)z_abort_thread_timeout(thread);
}
}
thread->base.thread_state |= _THREAD_DEAD;
sys_trace_thread_abort(thread);
#ifdef CONFIG_USERSPACE
/* Clear initialized state so that this thread object may be re-used
* and triggers errors if API calls are made on it from user threads
*/
z_object_uninit(thread->stack_obj);
z_object_uninit(thread);
/* Revoke permissions on thread's ID so that it may be recycled */
z_thread_perms_all_clear(thread);
#endif
}
#ifdef CONFIG_MULTITHREADING
#ifdef CONFIG_USERSPACE

20
kernel/thread_abort.c
View file

@ -27,16 +27,6 @@ extern void z_thread_single_abort(struct k_thread *thread);
#if !defined(CONFIG_ARCH_HAS_THREAD_ABORT)
void z_impl_k_thread_abort(k_tid_t thread)
{
/* We aren't trying to synchronize data access here (these
* APIs are internally synchronized). The original lock seems
* to have been in place to prevent the thread from waking up
* due to a delivered interrupt. Leave a dummy spinlock in
* place to do that. This API should be revisted though, it
* doesn't look SMP-safe as it stands.
*/
struct k_spinlock lock = {};
k_spinlock_key_t key = k_spin_lock(&lock);
__ASSERT((thread->base.user_options & K_ESSENTIAL) == 0U,
"essential thread aborted");
@ -44,7 +34,13 @@ void z_impl_k_thread_abort(k_tid_t thread)
z_thread_monitor_exit(thread);
if (thread == _current && !arch_is_in_isr()) {
z_swap(&lock, key);
/* Direct use of swap: reschedule doesn't have a test
* for "is _current dead" and we don't want one for
* performance reasons.
*/
struct k_spinlock lock = {};
z_swap(&lock, k_spin_lock(&lock));
} else {
/* Really, there's no good reason for this to be a
* scheduling point if we aren't aborting _current (by
@ -54,7 +50,7 @@ void z_impl_k_thread_abort(k_tid_t thread)
* rely on k_thread_abort() scheduling out of
* cooperative threads.
*/
z_reschedule(&lock, key);
z_reschedule_unlocked();
}
}
#endif