From 43ffae2a52d5ceef2fbbf0b647b82a902ce99232 Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Mon, 17 Jul 2023 19:24:53 -0400 Subject: [PATCH 1/4] kernel: dynamic: remove unnecessary size assignment Previously, the kernel stack size was adjusted for no apparent reason. Signed-off-by: Christopher Friedt --- kernel/dynamic.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/kernel/dynamic.c b/kernel/dynamic.c index 3c7a624f644e..23e64e802b5d 100644 --- a/kernel/dynamic.c +++ b/kernel/dynamic.c @@ -41,8 +41,6 @@ static k_thread_stack_t *z_thread_stack_alloc_pool(size_t size) size_t offset; k_thread_stack_t *stack; - size = Z_KERNEL_STACK_SIZE_ADJUST(size); - if (size > CONFIG_DYNAMIC_THREAD_STACK_SIZE) { LOG_DBG("stack size %zu is > pool stack size %d", size, CONFIG_DYNAMIC_THREAD_STACK_SIZE); From e5e79923e2334fd5001aec6206bc9487e983f981 Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Tue, 18 Jul 2023 08:33:41 -0400 Subject: [PATCH 2/4] kernel: dynamic: declare dynamic stubs when disabled With some of the recent work to disable unnecessary system calls, there is a scenario where `z_impl_k_thread_stack_free()` is not defined and an undefined symbol error occurs. Safety was very concerned that dynamic thread stack code might touch other code that does not malloc, so add a separate file for the stack alloc and free stubs. Signed-off-by: Christopher Friedt --- include/zephyr/kernel.h | 1 + kernel/CMakeLists.txt | 10 +++++----- kernel/dynamic_disabled.c | 25 +++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) create mode 100644 kernel/dynamic_disabled.c diff --git a/include/zephyr/kernel.h b/include/zephyr/kernel.h index 9a88baeb5bdf..07428d18ca8b 100644 --- a/include/zephyr/kernel.h +++ b/include/zephyr/kernel.h @@ -289,6 +289,7 @@ __syscall k_thread_stack_t *k_thread_stack_alloc(size_t size, int flags); * @retval 0 on success. * @retval -EBUSY if the thread stack is in use. * @retval -EINVAL if @p stack is invalid. + * @retval -ENOSYS if dynamic thread stack allocation is disabled * * @see CONFIG_DYNAMIC_THREAD */ diff --git a/kernel/CMakeLists.txt b/kernel/CMakeLists.txt index 2dfcaee9af4c..9e7602bfbd87 100644 --- a/kernel/CMakeLists.txt +++ b/kernel/CMakeLists.txt @@ -123,11 +123,11 @@ target_sources_ifdef( userspace.c ) -target_sources_ifdef( - CONFIG_DYNAMIC_THREAD - kernel PRIVATE - dynamic.c - ) +if(${CONFIG_DYNAMIC_THREAD}) + target_sources(kernel PRIVATE dynamic.c) +else() + target_sources(kernel PRIVATE dynamic_disabled.c) +endif() target_include_directories(kernel PRIVATE ${ZEPHYR_BASE}/kernel/include diff --git a/kernel/dynamic_disabled.c b/kernel/dynamic_disabled.c new file mode 100644 index 000000000000..47ce077304f7 --- /dev/null +++ b/kernel/dynamic_disabled.c @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2022, Meta + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#include +#include + +k_thread_stack_t *z_impl_k_thread_stack_alloc(size_t size, int flags) +{ + ARG_UNUSED(size); + ARG_UNUSED(flags); + + return NULL; +} + +int z_impl_k_thread_stack_free(k_thread_stack_t *stack) +{ + ARG_UNUSED(stack); + + return -ENOSYS; +} From d0da8c38079971533e8e5e60d78cff0ae5c2910d Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Thu, 22 Oct 2020 22:41:58 -0400 Subject: [PATCH 3/4] pthread: facilitate dynamically allocated thread stacks This change allows users to call pthread_create() with the pthread_attr_t argument equal to NULL. If Zephyr is configured with `CONFIG_DYNAMIC_THREAD`, then a suitable thread stack will be allocated via k_thread_stack_alloc(). The allocated thread stack is automatically freed via k_thread_stack_free(). This makes the Zephyr implementation of pthread_create() compliant with the normative spec. Signed-off-by: Christopher Friedt --- lib/posix/Kconfig.pthread | 21 +++++++ lib/posix/posix_internal.h | 3 + lib/posix/pthread.c | 118 ++++++++++++++++++++++++++++--------- 3 files changed, 114 insertions(+), 28 deletions(-) diff --git a/lib/posix/Kconfig.pthread b/lib/posix/Kconfig.pthread index 9b2541c6631b..388a30c5fa40 100644 --- a/lib/posix/Kconfig.pthread +++ b/lib/posix/Kconfig.pthread @@ -7,3 +7,24 @@ TYPE = PTHREAD type = pthread_t type-function = pthread_create source "lib/posix/Kconfig.template.pooled_ipc_type" + +if PTHREAD + +config PTHREAD_RECYCLER_DELAY_MS + int "Delay for reclaiming dynamic pthread stacks (ms)" + default 100 + help + Prior to a POSIX thread terminating via k_thread_abort(), scheduled + work is added to the system workqueue (SWQ) so that any resources + allocated by the thread (e.g. thread stack from a pool or the heap) + can be released back to the system. Because resources are also freed + on calls to pthread_create() there is no need to worry about resource + starvation. + + This option sets the number of milliseconds by which to defer + scheduled work. + + Note: this option should be considered temporary and will likely be + removed once a more synchronous solution is available. + +endif diff --git a/lib/posix/posix_internal.h b/lib/posix/posix_internal.h index e3c2b2817125..39c64c53331c 100644 --- a/lib/posix/posix_internal.h +++ b/lib/posix/posix_internal.h @@ -30,6 +30,9 @@ struct posix_thread { /* List of keys that thread has called pthread_setspecific() on */ sys_slist_t key_list; + /* Dynamic stack */ + k_thread_stack_t *dynamic_stack; + /* Exit status */ void *retval; diff --git a/lib/posix/pthread.c b/lib/posix/pthread.c index 519d68091248..a6505d4f9d33 100644 --- a/lib/posix/pthread.c +++ b/lib/posix/pthread.c @@ -16,6 +16,12 @@ #include #include +#ifdef CONFIG_DYNAMIC_THREAD_STACK_SIZE +#define DYNAMIC_STACK_SIZE CONFIG_DYNAMIC_THREAD_STACK_SIZE +#else +#define DYNAMIC_STACK_SIZE 0 +#endif + #define PTHREAD_INIT_FLAGS PTHREAD_CANCEL_ENABLE #define PTHREAD_CANCELED ((void *) -1) @@ -34,6 +40,7 @@ BUILD_ASSERT((PTHREAD_CREATE_DETACHED == 0 || PTHREAD_CREATE_JOINABLE == 0) && BUILD_ASSERT((PTHREAD_CANCEL_ENABLE == 0 || PTHREAD_CANCEL_DISABLE == 0) && (PTHREAD_CANCEL_ENABLE == 1 || PTHREAD_CANCEL_DISABLE == 1)); +static void posix_thread_recycle(void); static sys_dlist_t ready_q = SYS_DLIST_STATIC_INIT(&ready_q); static sys_dlist_t run_q = SYS_DLIST_STATIC_INIT(&run_q); static sys_dlist_t done_q = SYS_DLIST_STATIC_INIT(&done_q); @@ -205,13 +212,13 @@ int pthread_attr_setstack(pthread_attr_t *_attr, void *stackaddr, size_t stacksi static bool pthread_attr_is_valid(const struct pthread_attr *attr) { - /* - * FIXME: Pthread attribute must be non-null and it provides stack - * pointer and stack size. So even though POSIX 1003.1 spec accepts - * attrib as NULL but zephyr needs it initialized with valid stack. - */ - if (attr == NULL || attr->initialized == 0U || attr->stack == NULL || - attr->stacksize == 0) { + /* auto-alloc thread stack */ + if (attr == NULL) { + return true; + } + + /* caller-provided thread stack */ + if (attr->initialized == 0U || attr->stack == NULL || attr->stacksize == 0) { return false; } @@ -234,6 +241,13 @@ static bool pthread_attr_is_valid(const struct pthread_attr *attr) return true; } +static void posix_thread_recycle_work_handler(struct k_work *work) +{ + ARG_UNUSED(work); + posix_thread_recycle(); +} +static K_WORK_DELAYABLE_DEFINE(posix_thread_recycle_work, posix_thread_recycle_work_handler); + static void posix_thread_finalize(struct posix_thread *t, void *retval) { sys_snode_t *node_l; @@ -259,6 +273,9 @@ static void posix_thread_finalize(struct posix_thread *t, void *retval) t->retval = retval; k_spin_unlock(&pthread_pool_lock, key); + /* trigger recycle work */ + (void)k_work_schedule(&posix_thread_recycle_work, K_MSEC(CONFIG_PTHREAD_RECYCLER_DELAY_MS)); + /* abort the underlying k_thread */ k_thread_abort(&t->thread); } @@ -283,6 +300,45 @@ static void zephyr_thread_wrapper(void *arg1, void *arg2, void *arg3) CODE_UNREACHABLE; } +static void posix_thread_recycle(void) +{ + k_spinlock_key_t key; + struct posix_thread *t; + struct posix_thread *safe_t; + sys_dlist_t recyclables = SYS_DLIST_STATIC_INIT(&recyclables); + + key = k_spin_lock(&pthread_pool_lock); + SYS_DLIST_FOR_EACH_CONTAINER_SAFE(&done_q, t, safe_t, q_node) { + if (t->detachstate == PTHREAD_CREATE_JOINABLE) { + /* thread has not been joined yet */ + continue; + } + + sys_dlist_remove(&t->q_node); + sys_dlist_append(&recyclables, &t->q_node); + } + k_spin_unlock(&pthread_pool_lock, key); + + if (sys_dlist_is_empty(&recyclables)) { + return; + } + + if (IS_ENABLED(CONFIG_DYNAMIC_THREAD)) { + SYS_DLIST_FOR_EACH_CONTAINER(&recyclables, t, q_node) { + if (t->dynamic_stack != NULL) { + (void)k_thread_stack_free(t->dynamic_stack); + t->dynamic_stack = NULL; + } + } + } + + key = k_spin_lock(&pthread_pool_lock); + while (!sys_dlist_is_empty(&recyclables)) { + sys_dlist_append(&ready_q, sys_dlist_get(&recyclables)); + } + k_spin_unlock(&pthread_pool_lock, key); +} + /** * @brief Create a new thread. * @@ -297,32 +353,33 @@ int pthread_create(pthread_t *th, const pthread_attr_t *_attr, void *(*threadrou int err; k_spinlock_key_t key; pthread_barrier_t barrier; - struct posix_thread *safe_t; struct posix_thread *t = NULL; - const struct pthread_attr *attr = (const struct pthread_attr *)_attr; + struct pthread_attr attr_storage = init_pthread_attrs; + struct pthread_attr *attr = (struct pthread_attr *)_attr; if (!pthread_attr_is_valid(attr)) { return EINVAL; } + if (attr == NULL) { + attr = &attr_storage; + attr->stacksize = DYNAMIC_STACK_SIZE; + attr->stack = + k_thread_stack_alloc(attr->stacksize, k_is_user_context() ? K_USER : 0); + if (attr->stack == NULL) { + return EAGAIN; + } + } else { + __ASSERT_NO_MSG(attr != &attr_storage); + } + + /* reclaim resources greedily */ + posix_thread_recycle(); + key = k_spin_lock(&pthread_pool_lock); if (!sys_dlist_is_empty(&ready_q)) { - /* spawn thread 't' directly from ready_q */ t = CONTAINER_OF(sys_dlist_get(&ready_q), struct posix_thread, q_node); - } else { - SYS_DLIST_FOR_EACH_CONTAINER_SAFE(&done_q, t, safe_t, q_node) { - if (t->detachstate == PTHREAD_CREATE_JOINABLE) { - /* thread has not been joined yet */ - continue; - } - /* spawn thread 't' from done_q */ - sys_dlist_remove(&t->q_node); - break; - } - } - - if (t != NULL) { /* initialize thread state */ sys_dlist_append(&run_q, &t->q_node); t->qid = POSIX_THREAD_RUN_Q; @@ -332,12 +389,22 @@ int pthread_create(pthread_t *th, const pthread_attr_t *_attr, void *(*threadrou } t->cancel_pending = false; sys_slist_init(&t->key_list); + t->dynamic_stack = _attr == NULL ? attr->stack : NULL; } k_spin_unlock(&pthread_pool_lock, key); + if (t == NULL) { + /* no threads are ready */ + return EAGAIN; + } + if (IS_ENABLED(CONFIG_PTHREAD_CREATE_BARRIER)) { err = pthread_barrier_init(&barrier, NULL, 2); if (err != 0) { + if (t->dynamic_stack != NULL) { + (void)k_thread_stack_free(attr->stack); + } + /* cannot allocate barrier. move thread back to ready_q */ key = k_spin_lock(&pthread_pool_lock); sys_dlist_remove(&t->q_node); @@ -348,11 +415,6 @@ int pthread_create(pthread_t *th, const pthread_attr_t *_attr, void *(*threadrou } } - if (t == NULL) { - /* no threads are ready */ - return EAGAIN; - } - /* spawn the thread */ k_thread_create(&t->thread, attr->stack, attr->stacksize, zephyr_thread_wrapper, (void *)arg, threadroutine, From 4a6911351539f12037bc4a298758ba5036d157ef Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Wed, 10 Feb 2021 09:17:59 -0500 Subject: [PATCH 4/4] pthread: test: facilitate dynamically allocated thread stacks Tests for dynamically allocated POSIX thread stacks. Signed-off-by: Christopher Friedt --- tests/posix/common/prj.conf | 2 +- tests/posix/common/src/pthread.c | 47 ++++++++++++++++++++++++++++---- tests/posix/common/testcase.yaml | 9 ++++++ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/tests/posix/common/prj.conf b/tests/posix/common/prj.conf index f72676270156..1eb3e47658d4 100644 --- a/tests/posix/common/prj.conf +++ b/tests/posix/common/prj.conf @@ -1,6 +1,6 @@ CONFIG_PTHREAD_IPC=y CONFIG_POSIX_API=y -CONFIG_MAX_PTHREAD_COUNT=20 +CONFIG_MAX_PTHREAD_COUNT=10 CONFIG_ZTEST=y CONFIG_ZTEST_NEW_API=y CONFIG_SEM_VALUE_MAX=32767 diff --git a/tests/posix/common/src/pthread.c b/tests/posix/common/src/pthread.c index 3cd284f62dc4..e88703bae404 100644 --- a/tests/posix/common/src/pthread.c +++ b/tests/posix/common/src/pthread.c @@ -216,6 +216,9 @@ void *thread_top_term(void *p1) } if (id >= 2) { + if (IS_ENABLED(CONFIG_DYNAMIC_THREAD)) { + zassert_false(pthread_detach(self), "failed to set detach state"); + } ret = pthread_detach(self); if (id == 2) { zassert_equal(ret, EINVAL, "re-detached thread!"); @@ -345,8 +348,13 @@ ZTEST(posix_apis, test_pthread_execution) getschedparam.sched_priority, "scheduling priorities do not match!"); - ret = pthread_create(&newthread[i], &attr[i], thread_top_exec, - INT_TO_POINTER(i)); + if (IS_ENABLED(CONFIG_DYNAMIC_THREAD)) { + ret = pthread_create(&newthread[i], NULL, thread_top_exec, + INT_TO_POINTER(i)); + } else { + ret = pthread_create(&newthread[i], &attr[i], thread_top_exec, + INT_TO_POINTER(i)); + } /* TESTPOINT: Check if thread is created successfully */ zassert_false(ret, "Number of threads exceed max limit"); @@ -500,8 +508,13 @@ ZTEST(posix_apis, test_pthread_termination) schedparam.sched_priority = 2; pthread_attr_setschedparam(&attr[i], &schedparam); pthread_attr_setstack(&attr[i], &stack_t[i][0], STACKS); - ret = pthread_create(&newthread[i], &attr[i], thread_top_term, - INT_TO_POINTER(i)); + if (IS_ENABLED(CONFIG_DYNAMIC_THREAD)) { + ret = pthread_create(&newthread[i], NULL, thread_top_term, + INT_TO_POINTER(i)); + } else { + ret = pthread_create(&newthread[i], &attr[i], thread_top_term, + INT_TO_POINTER(i)); + } zassert_false(ret, "Not enough space to create new thread"); } @@ -571,8 +584,10 @@ ZTEST(posix_apis, test_pthread_create_negative) pthread_attr_t attr1; /* create pthread without attr initialized */ - ret = pthread_create(&pthread1, NULL, create_thread1, (void *)1); - zassert_equal(ret, EINVAL, "create thread with NULL successful"); + if (!IS_ENABLED(CONFIG_DYNAMIC_THREAD)) { + ret = pthread_create(&pthread1, NULL, create_thread1, (void *)1); + zassert_equal(ret, EAGAIN, "create thread with NULL successful"); + } /* initialized attr without set stack to create thread */ ret = pthread_attr_init(&attr1); @@ -777,3 +792,23 @@ ZTEST(posix_apis, test_pthread_equal) zassert_true(pthread_equal(pthread_self(), pthread_self())); zassert_false(pthread_equal(pthread_self(), (pthread_t)4242)); } + +static void *fun(void *arg) +{ + *((uint32_t *)arg) = 0xB105F00D; + return NULL; +} + +ZTEST(posix_apis, test_pthread_dynamic_stacks) +{ + pthread_t th; + uint32_t x = 0; + + if (!IS_ENABLED(CONFIG_DYNAMIC_THREAD)) { + ztest_test_skip(); + } + + zassert_ok(pthread_create(&th, NULL, fun, &x)); + zassert_ok(pthread_join(th, NULL)); + zassert_equal(0xB105F00D, x); +} diff --git a/tests/posix/common/testcase.yaml b/tests/posix/common/testcase.yaml index a3086233b6c3..19f858f0b50f 100644 --- a/tests/posix/common/testcase.yaml +++ b/tests/posix/common/testcase.yaml @@ -86,3 +86,12 @@ tests: portability.posix.common.signal.big_nsig: extra_configs: - CONFIG_POSIX_RTSIG_MAX=1024 + portability.posix.common.dynamic_stack: + integration_platforms: + - qemu_x86 + - qemu_riscv64 + extra_configs: + - CONFIG_DYNAMIC_THREAD=y + - CONFIG_THREAD_STACK_INFO=y + - CONFIG_DYNAMIC_THREAD_POOL_SIZE=5 + - CONFIG_HEAP_MEM_POOL_SIZE=16384