From 7a37a3dcbc01f15cb7df7e6e4ae3faf03f2aad53 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Wed, 16 Nov 2022 07:15:14 -0500 Subject: [PATCH 1/4] lib: posix: internal: use a more generic INIT mask and inlines Previously `PTHREAD_MUTEX_MASK_INIT` was used to mark a `pthread_mutex_t` as initialized. The same needs to be done for `pthread_cond_t` and likely others. Rather than copy-pasting that and a number of inlines that duplicate the same functionality, simply make it more generic. Signed-off-by: Chris Friedt --- lib/posix/posix_internal.h | 16 ++++++++-------- lib/posix/pthread_mutex.c | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/posix/posix_internal.h b/lib/posix/posix_internal.h index c0bfba72938a..01055b71f7fe 100644 --- a/lib/posix/posix_internal.h +++ b/lib/posix/posix_internal.h @@ -8,10 +8,10 @@ #define ZEPHYR_LIB_POSIX_POSIX_INTERNAL_H_ /* - * Bit used to mark a pthread_mutex_t as initialized. Initialization status is + * Bit used to mark a pthread object as initialized. Initialization status is * verified (against internal status) in lock / unlock / destroy functions. */ -#define PTHREAD_MUTEX_MASK_INIT 0x80000000 +#define PTHREAD_OBJ_MASK_INIT 0x80000000 struct posix_mutex { k_tid_t owner; @@ -59,19 +59,19 @@ struct posix_mutex *to_posix_mutex(pthread_mutex_t *mu); /* get a previously initialized posix_mutex */ struct posix_mutex *get_posix_mutex(pthread_mutex_t mut); -static inline bool is_pthread_mutex_initialized(pthread_mutex_t mut) +static inline bool is_pthread_obj_initialized(uint32_t obj) { - return (mut & PTHREAD_MUTEX_MASK_INIT) != 0; + return (obj & PTHREAD_OBJ_MASK_INIT) != 0; } -static inline pthread_mutex_t mark_pthread_mutex_initialized(pthread_mutex_t mut) +static inline uint32_t mark_pthread_obj_initialized(uint32_t obj) { - return mut | PTHREAD_MUTEX_MASK_INIT; + return obj | PTHREAD_OBJ_MASK_INIT; } -static inline pthread_mutex_t mark_pthread_mutex_uninitialized(pthread_mutex_t mut) +static inline uint32_t mark_pthread_obj_uninitialized(uint32_t obj) { - return mut & ~PTHREAD_MUTEX_MASK_INIT; + return obj & ~PTHREAD_OBJ_MASK_INIT; } #endif diff --git a/lib/posix/pthread_mutex.c b/lib/posix/pthread_mutex.c index 29a878f58bdc..17abffe61af7 100644 --- a/lib/posix/pthread_mutex.c +++ b/lib/posix/pthread_mutex.c @@ -33,7 +33,7 @@ SYS_BITARRAY_DEFINE_STATIC(posix_mutex_bitarray, CONFIG_MAX_PTHREAD_MUTEX_COUNT) * perspective of the application). With a linear space, this means that * the theoretical pthread_mutex_t range is [0,2147483647]. */ -BUILD_ASSERT(CONFIG_MAX_PTHREAD_MUTEX_COUNT < PTHREAD_MUTEX_MASK_INIT, +BUILD_ASSERT(CONFIG_MAX_PTHREAD_MUTEX_COUNT < PTHREAD_OBJ_MASK_INIT, "CONFIG_MAX_PTHREAD_MUTEX_COUNT is too high"); static inline size_t posix_mutex_to_offset(struct posix_mutex *m) @@ -43,7 +43,7 @@ static inline size_t posix_mutex_to_offset(struct posix_mutex *m) static inline size_t to_posix_mutex_idx(pthread_mutex_t mut) { - return mark_pthread_mutex_uninitialized(mut); + return mark_pthread_obj_uninitialized(mut); } struct posix_mutex *get_posix_mutex(pthread_mutex_t mu) @@ -52,7 +52,7 @@ struct posix_mutex *get_posix_mutex(pthread_mutex_t mu) size_t bit = to_posix_mutex_idx(mu); /* if the provided mutex does not claim to be initialized, its invalid */ - if (!is_pthread_mutex_initialized(mu)) { + if (!is_pthread_obj_initialized(mu)) { return NULL; } @@ -85,7 +85,7 @@ struct posix_mutex *to_posix_mutex(pthread_mutex_t *mu) } /* Record the associated posix_mutex in mu and mark as initialized */ - *mu = mark_pthread_mutex_initialized(bit); + *mu = mark_pthread_obj_initialized(bit); /* Initialize the posix_mutex */ m = &posix_mutex_pool[bit]; From 5bcff83c1597c7ff59b7615c36f4ec1bd035c7a7 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Fri, 11 Nov 2022 06:59:02 -0500 Subject: [PATCH 2/4] posix: pthread: take care with pthread cond resources Previously, `pthread_cond_init()` could not actually fail, and destroying condition variables was a no-op, and it was missing in `pthread_exit()`. However, with the change of `pthread_cond_t` to `uint32_t`, and since those are embedded inside of `struct posix_thread` for the time being, the pthread code needs to keep track that it is relinquishes used condition variables when a thread completes. Signed-off-by: Chris Friedt --- lib/posix/pthread.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/posix/pthread.c b/lib/posix/pthread.c index 42961bd2337d..21ba0ef6887b 100644 --- a/lib/posix/pthread.c +++ b/lib/posix/pthread.c @@ -407,6 +407,8 @@ void pthread_exit(void *retval) pthread_mutex_unlock(&self->state_lock); pthread_mutex_destroy(&self->state_lock); + pthread_cond_destroy(&self->state_cond); + k_thread_abort((k_tid_t)self); } From 30c3c545936e0cf40e7f828b6baa1e47326910e1 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Fri, 11 Nov 2022 06:57:31 -0500 Subject: [PATCH 3/4] posix: cond: abstract pthread_cond_t as uint32_t Consistent with the change of `pthread_t` from `struct posix_thread` to `uint32_t`, we can now also abstract `pthread_cond_t` as `uint32_t` and separate `struct posix_cond` as an implementation detail, hidden from POSIX API consumers. This change deprecates `PTHREAD_COND_DEFINE()` in favour of the (standardized) `PTHREAD_COND_INITIALIZER`. This change introduces `CONFIG_MAX_PTHREAD_COND_COUNT`. Signed-off-by: Chris Friedt --- include/zephyr/posix/posix_types.h | 4 +- include/zephyr/posix/pthread.h | 27 ++--- lib/posix/Kconfig | 7 ++ lib/posix/posix_internal.h | 26 +++-- lib/posix/pthread_cond.c | 159 +++++++++++++++++++++++++++-- 5 files changed, 190 insertions(+), 33 deletions(-) diff --git a/include/zephyr/posix/posix_types.h b/include/zephyr/posix/posix_types.h index 9377268310b7..4820f0f5afc1 100644 --- a/include/zephyr/posix/posix_types.h +++ b/include/zephyr/posix/posix_types.h @@ -57,9 +57,7 @@ typedef struct pthread_mutexattr { } pthread_mutexattr_t; /* Condition variables */ -typedef struct pthread_cond { - _wait_q_t wait_q; -} pthread_cond_t; +typedef uint32_t pthread_cond_t; typedef struct pthread_condattr { } pthread_condattr_t; diff --git a/include/zephyr/posix/pthread.h b/include/zephyr/posix/pthread.h index f0f602a8a8c5..4186aefba555 100644 --- a/include/zephyr/posix/pthread.h +++ b/include/zephyr/posix/pthread.h @@ -36,6 +36,13 @@ extern "C" { /* The minimum allowable stack size */ #define PTHREAD_STACK_MIN Z_KERNEL_STACK_SIZE_ADJUST(0) +/** + * @brief Declare a condition variable as initialized + * + * Initialize a condition variable with the default condition variable attributes. + */ +#define PTHREAD_COND_INITIALIZER (-1) + /** * @brief Declare a pthread condition variable * @@ -44,35 +51,23 @@ extern "C" { * strategies for kernel objects. * * @param name Symbol name of the condition variable + * @deprecated Use @c PTHREAD_COND_INITIALIZER instead. */ -#define PTHREAD_COND_DEFINE(name) \ - struct pthread_cond name = { \ - .wait_q = Z_WAIT_Q_INIT(&name.wait_q), \ - } +#define PTHREAD_COND_DEFINE(name) pthread_cond_t name = PTHREAD_COND_INITIALIZER /** * @brief POSIX threading compatibility API * * See IEEE 1003.1 */ -static inline int pthread_cond_init(pthread_cond_t *cv, - const pthread_condattr_t *att) -{ - ARG_UNUSED(att); - z_waitq_init(&cv->wait_q); - return 0; -} +int pthread_cond_init(pthread_cond_t *cv, const pthread_condattr_t *att); /** * @brief POSIX threading compatibility API * * See IEEE 1003.1 */ -static inline int pthread_cond_destroy(pthread_cond_t *cv) -{ - ARG_UNUSED(cv); - return 0; -} +int pthread_cond_destroy(pthread_cond_t *cv); /** * @brief POSIX threading compatibility API diff --git a/lib/posix/Kconfig b/lib/posix/Kconfig index 661c2bbeb0e2..1221a1c2e697 100644 --- a/lib/posix/Kconfig +++ b/lib/posix/Kconfig @@ -42,6 +42,13 @@ config MAX_PTHREAD_MUTEX_COUNT help Maximum number of simultaneously active mutexes in a POSIX application. +config MAX_PTHREAD_COND_COUNT + int "Maximum simultaneously active condition variables in a POSIX application" + default 5 + range 0 255 + help + Maximum number of simultaneously active condition variables in a POSIX application. + config SEM_VALUE_MAX int "Maximum semaphore limit" default 32767 diff --git a/lib/posix/posix_internal.h b/lib/posix/posix_internal.h index 01055b71f7fe..e6e04ef04237 100644 --- a/lib/posix/posix_internal.h +++ b/lib/posix/posix_internal.h @@ -20,6 +20,10 @@ struct posix_mutex { _wait_q_t wait_q; }; +struct posix_cond { + _wait_q_t wait_q; +}; + enum pthread_state { /* The thread structure is unallocated and available for reuse. */ PTHREAD_TERMINATED = 0, @@ -51,14 +55,6 @@ struct posix_thread { pthread_cond_t state_cond; }; -struct posix_thread *to_posix_thread(pthread_t pthread); - -/* get and possibly initialize a posix_mutex */ -struct posix_mutex *to_posix_mutex(pthread_mutex_t *mu); - -/* get a previously initialized posix_mutex */ -struct posix_mutex *get_posix_mutex(pthread_mutex_t mut); - static inline bool is_pthread_obj_initialized(uint32_t obj) { return (obj & PTHREAD_OBJ_MASK_INIT) != 0; @@ -74,4 +70,18 @@ static inline uint32_t mark_pthread_obj_uninitialized(uint32_t obj) return obj & ~PTHREAD_OBJ_MASK_INIT; } +struct posix_thread *to_posix_thread(pthread_t pthread); + +/* get and possibly initialize a posix_mutex */ +struct posix_mutex *to_posix_mutex(pthread_mutex_t *mu); + +/* get a previously initialized posix_mutex */ +struct posix_mutex *get_posix_mutex(pthread_mutex_t mut); + +/* get and possibly initialize a posix_cond */ +struct posix_cond *to_posix_cond(pthread_cond_t *cvar); + +/* get a previously initialized posix_cond */ +struct posix_cond *get_posix_cond(pthread_cond_t cond); + #endif diff --git a/lib/posix/pthread_cond.c b/lib/posix/pthread_cond.c index ea9cfe24163c..7e21244d1d3c 100644 --- a/lib/posix/pthread_cond.c +++ b/lib/posix/pthread_cond.c @@ -8,6 +8,7 @@ #include #include #include +#include #include "posix_internal.h" @@ -15,11 +16,80 @@ extern struct k_spinlock z_pthread_spinlock; int64_t timespec_to_timeoutms(const struct timespec *abstime); -static int cond_wait(pthread_cond_t *cv, pthread_mutex_t *mu, - k_timeout_t timeout) +static struct posix_cond posix_cond_pool[CONFIG_MAX_PTHREAD_COND_COUNT]; +SYS_BITARRAY_DEFINE_STATIC(posix_cond_bitarray, CONFIG_MAX_PTHREAD_COND_COUNT); + +/* + * We reserve the MSB to mark a pthread_cond_t as initialized (from the + * perspective of the application). With a linear space, this means that + * the theoretical pthread_cond_t range is [0,2147483647]. + */ +BUILD_ASSERT(CONFIG_MAX_PTHREAD_COND_COUNT < PTHREAD_OBJ_MASK_INIT, + "CONFIG_MAX_PTHREAD_COND_COUNT is too high"); + +static inline size_t posix_cond_to_offset(struct posix_cond *cv) +{ + return cv - posix_cond_pool; +} + +static inline size_t to_posix_cond_idx(pthread_cond_t cond) +{ + return mark_pthread_obj_uninitialized(cond); +} + +struct posix_cond *get_posix_cond(pthread_cond_t cond) +{ + int actually_initialized; + size_t bit = to_posix_cond_idx(cond); + + /* if the provided cond does not claim to be initialized, its invalid */ + if (!is_pthread_obj_initialized(cond)) { + return NULL; + } + + /* Mask off the MSB to get the actual bit index */ + if (sys_bitarray_test_bit(&posix_cond_bitarray, bit, &actually_initialized) < 0) { + return NULL; + } + + if (actually_initialized == 0) { + /* The cond claims to be initialized but is actually not */ + return NULL; + } + + return &posix_cond_pool[bit]; +} + +struct posix_cond *to_posix_cond(pthread_cond_t *cvar) +{ + size_t bit; + struct posix_cond *cv; + + if (*cvar != PTHREAD_COND_INITIALIZER) { + return get_posix_cond(*cvar); + } + + /* Try and automatically associate a posix_cond */ + if (sys_bitarray_alloc(&posix_cond_bitarray, 1, &bit) < 0) { + /* No conds left to allocate */ + return NULL; + } + + /* Record the associated posix_cond in mu and mark as initialized */ + *cvar = mark_pthread_obj_initialized(bit); + cv = &posix_cond_pool[bit]; + + /* Initialize the condition variable here */ + z_waitq_init(&cv->wait_q); + + return cv; +} + +static int cond_wait(pthread_cond_t *cond, pthread_mutex_t *mu, k_timeout_t timeout) { int ret; k_spinlock_key_t key; + struct posix_cond *cv; struct posix_mutex *m; key = k_spin_lock(&z_pthread_spinlock); @@ -29,6 +99,12 @@ static int cond_wait(pthread_cond_t *cv, pthread_mutex_t *mu, return EINVAL; } + cv = to_posix_cond(cond); + if (cv == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } + __ASSERT_NO_MSG(m->lock_count == 1U); m->lock_count = 0U; m->owner = NULL; @@ -48,14 +124,41 @@ static int cond_wait(pthread_cond_t *cv, pthread_mutex_t *mu, return ret == -EAGAIN ? ETIMEDOUT : ret; } -int pthread_cond_signal(pthread_cond_t *cv) +int pthread_cond_signal(pthread_cond_t *cvar) { + k_spinlock_key_t key; + struct posix_cond *cv; + + key = k_spin_lock(&z_pthread_spinlock); + + cv = to_posix_cond(cvar); + if (cv == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } + + k_spin_unlock(&z_pthread_spinlock, key); + z_sched_wake(&cv->wait_q, 0, NULL); + return 0; } -int pthread_cond_broadcast(pthread_cond_t *cv) +int pthread_cond_broadcast(pthread_cond_t *cvar) { + k_spinlock_key_t key; + struct posix_cond *cv; + + key = k_spin_lock(&z_pthread_spinlock); + + cv = to_posix_cond(cvar); + if (cv == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } + + k_spin_unlock(&z_pthread_spinlock, key); + z_sched_wake_all(&cv->wait_q, 0, NULL); return 0; } @@ -65,9 +168,53 @@ int pthread_cond_wait(pthread_cond_t *cv, pthread_mutex_t *mut) return cond_wait(cv, mut, K_FOREVER); } -int pthread_cond_timedwait(pthread_cond_t *cv, pthread_mutex_t *mut, - const struct timespec *abstime) +int pthread_cond_timedwait(pthread_cond_t *cv, pthread_mutex_t *mut, const struct timespec *abstime) { int32_t timeout = (int32_t)timespec_to_timeoutms(abstime); return cond_wait(cv, mut, K_MSEC(timeout)); } + +int pthread_cond_init(pthread_cond_t *cvar, const pthread_condattr_t *att) +{ + k_spinlock_key_t key; + struct posix_cond *cv; + + ARG_UNUSED(att); + *cvar = PTHREAD_COND_INITIALIZER; + + key = k_spin_lock(&z_pthread_spinlock); + + cv = to_posix_cond(cvar); + if (cv == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } + + k_spin_unlock(&z_pthread_spinlock, key); + + return 0; +} + +int pthread_cond_destroy(pthread_cond_t *cvar) +{ + __unused int rc; + k_spinlock_key_t key; + struct posix_cond *cv; + pthread_cond_t c = *cvar; + size_t bit = to_posix_cond_idx(c); + + key = k_spin_lock(&z_pthread_spinlock); + + cv = get_posix_cond(c); + if (cv == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } + + rc = sys_bitarray_free(&posix_cond_bitarray, 1, bit); + __ASSERT(rc == 0, "failed to free bit %zu", bit); + + k_spin_unlock(&z_pthread_spinlock, key); + + return 0; +} From f622477dc36095ae1ee6ad28da5429dc9b61f921 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Fri, 11 Nov 2022 07:05:09 -0500 Subject: [PATCH 4/4] tests: posix: cond: test to ensure there is no resource leakage Add a test to ensure that `pthread_cond_t` can be used over and over again and that there is no resource leakage with proper usage. Signed-off-by: Chris Friedt --- tests/posix/common/src/cond.c | 48 +++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 tests/posix/common/src/cond.c diff --git a/tests/posix/common/src/cond.c b/tests/posix/common/src/cond.c new file mode 100644 index 000000000000..63caf9a420be --- /dev/null +++ b/tests/posix/common/src/cond.c @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2022 Meta + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#include + +/** + * @brief Test to demonstrate limited condition variable resources + * + * @details Exactly CONFIG_MAX_PTHREAD_COND_COUNT can be in use at once. + */ +ZTEST(posix_apis, test_posix_cond_resource_exhausted) +{ + size_t i; + pthread_cond_t m[CONFIG_MAX_PTHREAD_COND_COUNT + 1]; + + for (i = 0; i < CONFIG_MAX_PTHREAD_COND_COUNT; ++i) { + zassert_ok(pthread_cond_init(&m[i], NULL), "failed to init cond %zu", i); + } + + /* try to initialize one more than CONFIG_MAX_PTHREAD_COND_COUNT */ + zassert_equal(i, CONFIG_MAX_PTHREAD_COND_COUNT); + zassert_not_equal(0, pthread_cond_init(&m[i], NULL), "should not have initialized cond %zu", + i); + + for (; i > 0; --i) { + zassert_ok(pthread_cond_destroy(&m[i - 1]), "failed to destroy cond %zu", i - 1); + } +} + +/** + * @brief Test to that there are no condition variable resource leaks + * + * @details Demonstrate that condition variables may be used over and over again. + */ +ZTEST(posix_apis, test_posix_cond_resource_leak) +{ + pthread_cond_t m; + + for (size_t i = 0; i < 2 * CONFIG_MAX_PTHREAD_COND_COUNT; ++i) { + zassert_ok(pthread_cond_init(&m, NULL), "failed to init cond %zu", i); + zassert_ok(pthread_cond_destroy(&m), "failed to destroy cond %zu", i); + } +}