From 6fb6d3cfbe0c615dfecdc13692d79cc9a20ae1c2 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Fri, 19 Feb 2021 15:32:19 -0800 Subject: [PATCH] kernel: Add new k_thread_abort()/k_thread_join() Add a newer, much smaller and simpler implementation of abort and join. No need to involve the idle thread. No need for a special code path for self-abort. Joining a thread and waiting for an aborting one to terminate elsewhere share an implementation. All work in both calls happens under a single locked path with no unexpected synchronization points. This fixes a bug with the current implementation where the action of z_sched_single_abort() was nonatomic, releasing the lock internally at a point where the thread to be aborted could self-abort and confuse the state such that it failed to abort at all. Note that the arm32 and native_posix architectures, which have their own thread abort implementations, now see a much simplified "z_thread_abort()" internal API. Signed-off-by: Andy Ross --- arch/arm/core/aarch32/cortex_m/thread_abort.c | 7 +- arch/posix/core/posix_core.c | 10 +- arch/x86/core/x86_mmu.c | 2 +- include/kernel/thread.h | 3 + kernel/include/kernel_internal.h | 2 +- kernel/include/ksched.h | 3 +- kernel/sched.c | 127 ++++++++++++++++++ kernel/thread.c | 1 + 8 files changed, 136 insertions(+), 19 deletions(-) diff --git a/arch/arm/core/aarch32/cortex_m/thread_abort.c b/arch/arm/core/aarch32/cortex_m/thread_abort.c index 941d3baa4151b8..028a62fed85b78 100644 --- a/arch/arm/core/aarch32/cortex_m/thread_abort.c +++ b/arch/arm/core/aarch32/cortex_m/thread_abort.c @@ -40,13 +40,8 @@ void z_impl_k_thread_abort(k_tid_t thread) * is not an implicit scheduler invocation. */ SCB->ICSR |= SCB_ICSR_PENDSVSET_Msk; - } else { - z_self_abort(); /* Never returns */ } } - z_thread_single_abort(thread); - - /* The abort handler might have altered the ready queue. */ - z_reschedule_unlocked(); + z_thread_abort(thread); } diff --git a/arch/posix/core/posix_core.c b/arch/posix/core/posix_core.c index d32fb85b8f994c..49bcf46473bd1e 100644 --- a/arch/posix/core/posix_core.c +++ b/arch/posix/core/posix_core.c @@ -506,17 +506,9 @@ void z_impl_k_thread_abort(k_tid_t thread) threads_table[thread_idx].thead_cnt, thread_idx, __func__); - - if (arch_is_in_isr()) { - z_thread_single_abort(thread); - return; - } - - z_self_abort(); - CODE_UNREACHABLE; /* LCOV_EXCL_LINE */ } - z_thread_single_abort(thread); + z_thread_abort(thread); if (tstatus->aborted == 0) { PC_DEBUG("%s aborting now [%i] %i\n", diff --git a/arch/x86/core/x86_mmu.c b/arch/x86/core/x86_mmu.c index efad000c86a16d..ed1463a2605beb 100644 --- a/arch/x86/core/x86_mmu.c +++ b/arch/x86/core/x86_mmu.c @@ -1598,7 +1598,7 @@ void arch_mem_domain_thread_remove(struct k_thread *thread) if ((thread->base.thread_state & _THREAD_DEAD) == 0) { /* Thread is migrating to another memory domain and not * exiting for good; we weren't called from - * z_thread_single_abort(). Resetting the stack region will + * z_thread_abort(). Resetting the stack region will * take place in the forthcoming thread_add() call. */ return; diff --git a/include/kernel/thread.h b/include/kernel/thread.h index d3f3f82ffa3f24..6cdef33a128a79 100644 --- a/include/kernel/thread.h +++ b/include/kernel/thread.h @@ -204,6 +204,9 @@ struct k_thread { /** static thread init data */ void *init_data; + /** threads waiting in k_thread_join() */ + _wait_q_t join_queue; + #if defined(CONFIG_POLL) struct z_poller poller; #endif diff --git a/kernel/include/kernel_internal.h b/kernel/include/kernel_internal.h index 7ca6c9f99476e5..8ced6540f5caf1 100644 --- a/kernel/include/kernel_internal.h +++ b/kernel/include/kernel_internal.h @@ -151,7 +151,7 @@ bool z_stack_is_user_capable(k_thread_stack_t *stack); /* Memory domain setup hook, called from z_setup_new_thread() */ void z_mem_domain_init_thread(struct k_thread *thread); -/* Memory domain teardown hook, called from z_thread_single_abort() */ +/* Memory domain teardown hook, called from z_thread_abort() */ void z_mem_domain_exit_thread(struct k_thread *thread); /* This spinlock: diff --git a/kernel/include/ksched.h b/kernel/include/ksched.h index 23e48a511cf394..8045429d13f5f8 100644 --- a/kernel/include/ksched.h +++ b/kernel/include/ksched.h @@ -64,10 +64,9 @@ void z_sched_abort(struct k_thread *thread); void z_sched_ipi(void); void z_sched_start(struct k_thread *thread); void z_ready_thread(struct k_thread *thread); -void z_thread_single_abort(struct k_thread *thread); -FUNC_NORETURN void z_self_abort(void); void z_requeue_current(struct k_thread *curr); struct k_thread *z_swap_next_thread(void); +void z_thread_abort(struct k_thread *thread); static inline void z_pend_curr_unlocked(_wait_q_t *wait_q, k_timeout_t timeout) { diff --git a/kernel/sched.c b/kernel/sched.c index 2fe36a19a26331..7e30486560deeb 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -52,6 +52,7 @@ struct z_kernel _kernel; struct k_spinlock sched_spinlock; static void update_cache(int); +static void end_thread(struct k_thread *thread); static inline int is_preempt(struct k_thread *thread) { @@ -216,6 +217,11 @@ void z_requeue_current(struct k_thread *curr) } #endif +static inline bool is_aborting(struct k_thread *thread) +{ + return (thread->base.thread_state & _THREAD_ABORTING) != 0; +} + static ALWAYS_INLINE struct k_thread *next_up(void) { struct k_thread *thread; @@ -259,6 +265,10 @@ static ALWAYS_INLINE struct k_thread *next_up(void) * "ready", it means "is _current already added back to the * queue such that we don't want to re-add it". */ + if (is_aborting(_current)) { + end_thread(_current); + } + int queued = z_is_thread_queued(_current); int active = !z_is_thread_prevented_from_running(_current); @@ -1427,6 +1437,123 @@ int k_thread_cpu_mask_disable(k_tid_t thread, int cpu) #endif /* CONFIG_SCHED_CPU_MASK */ +static inline void unpend_all(_wait_q_t *wait_q) +{ + struct k_thread *thread; + + while ((thread = z_waitq_head(wait_q)) != NULL) { + unpend_thread_no_timeout(thread); + (void)z_abort_thread_timeout(thread); + arch_thread_return_value_set(thread, 0); + ready_thread(thread); + } +} + +static void end_thread(struct k_thread *thread) +{ + /* We hold the lock, and the thread is known not to be running + * anywhere. + */ + if ((thread->base.thread_state & _THREAD_DEAD) == 0) { + thread->base.thread_state |= _THREAD_DEAD; + thread->base.thread_state &= ~_THREAD_ABORTING; + if (z_is_thread_queued(thread)) { + dequeue_thread(&_kernel.ready_q.runq, thread); + } + if (thread->base.pended_on != NULL) { + unpend_thread_no_timeout(thread); + } + (void)z_abort_thread_timeout(thread); + unpend_all(&thread->join_queue); + update_cache(1); + + sys_trace_thread_abort(thread); + z_thread_monitor_exit(thread); + +#ifdef CONFIG_USERSPACE + z_mem_domain_exit_thread(thread); + z_thread_perms_all_clear(thread); + z_object_uninit(thread->stack_obj); + z_object_uninit(thread); +#endif + } +} + +void z_thread_abort(struct k_thread *thread) +{ + k_spinlock_key_t key = k_spin_lock(&sched_spinlock); + + if (thread->base.thread_state & _THREAD_DEAD) { + k_spin_unlock(&sched_spinlock, key); + return; + } + +#ifdef CONFIG_SMP + if (is_aborting(thread) && thread == _current && arch_is_in_isr()) { + /* Another CPU is spinning for us, don't deadlock */ + end_thread(thread); + } + + bool active = thread_active_elsewhere(thread); + + if (active) { + /* It's running somewhere else, flag and poke */ + thread->base.thread_state |= _THREAD_ABORTING; + arch_sched_ipi(); + } + + if (is_aborting(thread) && thread != _current) { + if (arch_is_in_isr()) { + /* ISRs can only spin waiting another CPU */ + k_spin_unlock(&sched_spinlock, key); + while (is_aborting(thread)) { + } + } else if (active) { + /* Threads can join */ + add_to_waitq_locked(_current, &thread->join_queue); + z_swap(&sched_spinlock, key); + } + return; /* lock has been released */ + } +#endif + end_thread(thread); + if (thread == _current && !arch_is_in_isr()) { + z_swap(&sched_spinlock, key); + __ASSERT(false, "aborted _current back from dead"); + } + k_spin_unlock(&sched_spinlock, key); +} + +#if !defined(CONFIG_ARCH_HAS_THREAD_ABORT) +void z_impl_k_thread_abort(struct k_thread *thread) +{ + z_thread_abort(thread); +} +#endif + +int z_impl_k_thread_join(struct k_thread *thread, k_timeout_t timeout) +{ + k_spinlock_key_t key = k_spin_lock(&sched_spinlock); + int ret = 0; + + if (thread->base.thread_state & _THREAD_DEAD) { + ret = 0; + } else if (K_TIMEOUT_EQ(timeout, K_NO_WAIT)) { + ret = -EBUSY; + } else if (thread == _current || + thread->base.pended_on == &_current->join_queue) { + ret = -EDEADLK; + } else { + __ASSERT(!arch_is_in_isr(), "cannot join in ISR"); + add_to_waitq_locked(_current, &thread->join_queue); + add_thread_timeout(_current, timeout); + return z_swap(&sched_spinlock, key); + } + + k_spin_unlock(&sched_spinlock, key); + return ret; +} + #ifdef CONFIG_USERSPACE /* Special case: don't oops if the thread is uninitialized. This is because * the initialization bit does double-duty for thread objects; if false, means diff --git a/kernel/thread.c b/kernel/thread.c index 8939f0b99f0fdc..989f17f1349328 100644 --- a/kernel/thread.c +++ b/kernel/thread.c @@ -552,6 +552,7 @@ char *z_setup_new_thread(struct k_thread *new_thread, /* Any given thread has access to itself */ k_object_access_grant(new_thread, new_thread); #endif + z_waitq_init(&new_thread->join_queue); /* Initialize various struct k_thread members */ z_init_thread_base(&new_thread->base, prio, _THREAD_PRESTART, options);