From 8bdabcc46bf80f948461dca4a9100b1d866762ba Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 7 Jan 2020 09:58:46 -0800 Subject: [PATCH] kernel/sched: Move thread suspend and abort under the scheduler lock 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 --- kernel/sched.c | 77 +++++++++++++++++++++++++++++++++++++++---- kernel/thread.c | 52 +---------------------------- kernel/thread_abort.c | 20 +++++------ 3 files changed, 79 insertions(+), 70 deletions(-) diff --git a/kernel/sched.c b/kernel/sched.c index 28e8e1a6d89..0878092b607 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -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) { diff --git a/kernel/thread.c b/kernel/thread.c index 8d5ebb61952..2d5d99cbebb 100644 --- a/kernel/thread.c +++ b/kernel/thread.c @@ -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 #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 diff --git a/kernel/thread_abort.c b/kernel/thread_abort.c index 502986a5f7b..152962ebf38 100644 --- a/kernel/thread_abort.c +++ b/kernel/thread_abort.c @@ -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