From 7197c7bb5b85235f61d87d1410277d6d344e0490 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Thu, 10 Nov 2022 10:11:57 -0500 Subject: [PATCH 1/3] posix: pthread: take care with pthread mutex resources Previously, `pthread_mutex_init()` could not actually fail, and destroying mutexes was a no-op, so it was missing in a couple of places. However, with the change of `pthread_mutex_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 mutex resources when a thread completes. Signed-off-by: Chris Friedt --- lib/posix/pthread.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/posix/pthread.c b/lib/posix/pthread.c index 213812d0d85cc..42961bd2337d5 100644 --- a/lib/posix/pthread.c +++ b/lib/posix/pthread.c @@ -149,6 +149,7 @@ static void zephyr_thread_wrapper(void *arg1, void *arg2, void *arg3) int pthread_create(pthread_t *newthread, const pthread_attr_t *attr, void *(*threadroutine)(void *), void *arg) { + int rv; int32_t prio; k_spinlock_key_t key; uint32_t pthread_num; @@ -181,13 +182,15 @@ int pthread_create(pthread_t *newthread, const pthread_attr_t *attr, return EAGAIN; } - prio = posix_to_zephyr_priority(attr->priority, attr->schedpolicy); + rv = pthread_mutex_init(&thread->state_lock, NULL); + if (rv != 0) { + key = k_spin_lock(&pthread_pool_lock); + thread->state = PTHREAD_EXITED; + k_spin_unlock(&pthread_pool_lock, key); + return rv; + } - /* - * Ignore return value, as we know that Zephyr implementation - * cannot fail. - */ - (void)pthread_mutex_init(&thread->state_lock, NULL); + prio = posix_to_zephyr_priority(attr->priority, attr->schedpolicy); cancel_key = k_spin_lock(&thread->cancel_lock); thread->cancel_state = (1 << _PTHREAD_CANCEL_POS) & attr->flags; @@ -402,6 +405,8 @@ void pthread_exit(void *retval) } pthread_mutex_unlock(&self->state_lock); + pthread_mutex_destroy(&self->state_lock); + k_thread_abort((k_tid_t)self); } @@ -440,6 +445,10 @@ int pthread_join(pthread_t thread, void **status) } pthread_mutex_unlock(&pthread->state_lock); + if (pthread->state == PTHREAD_EXITED) { + pthread_mutex_destroy(&pthread->state_lock); + } + return ret; } From 3965dac8a50823ca87842a8e75f80acfa9409be1 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Tue, 8 Nov 2022 10:47:43 -0500 Subject: [PATCH 2/3] posix: mutex: abstract pthread_mutex_t as uint32_t Consistent with the change of `pthread_t` from `struct posix_thread` to `uint32_t`, we can now also abstract `pthread_mutex_t` as `uint32_t` and separate `struct posix_mutex` as an implementation detail, hidden from POSIX API consumers. This change deprecates `PTHREAD_MUTEX_DEFINE()` in favour of the (standardized) `PTHREAD_MUTEX_INITIALIZER`. This change introduces `CONFIG_MAX_PTHREAD_MUTEX_COUNT`. Signed-off-by: Chris Friedt --- include/zephyr/posix/posix_types.h | 7 +- include/zephyr/posix/pthread.h | 16 ++-- lib/posix/Kconfig | 7 ++ lib/posix/posix_internal.h | 34 +++++++ lib/posix/pthread_cond.c | 25 +++-- lib/posix/pthread_mutex.c | 144 ++++++++++++++++++++++++++--- 6 files changed, 198 insertions(+), 35 deletions(-) diff --git a/include/zephyr/posix/posix_types.h b/include/zephyr/posix/posix_types.h index b294f6acd4bee..9377268310b70 100644 --- a/include/zephyr/posix/posix_types.h +++ b/include/zephyr/posix/posix_types.h @@ -50,12 +50,7 @@ typedef uint32_t pthread_t; typedef struct k_sem sem_t; /* Mutex */ -typedef struct pthread_mutex { - k_tid_t owner; - uint16_t lock_count; - int type; - _wait_q_t wait_q; -} pthread_mutex_t; +typedef uint32_t pthread_mutex_t; typedef struct pthread_mutexattr { int type; diff --git a/include/zephyr/posix/pthread.h b/include/zephyr/posix/pthread.h index 9db6bd0b7b606..f0f602a8a8c55 100644 --- a/include/zephyr/posix/pthread.h +++ b/include/zephyr/posix/pthread.h @@ -129,6 +129,13 @@ static inline int pthread_condattr_destroy(pthread_condattr_t *att) return 0; } +/** + * @brief Declare a mutex as initialized + * + * Initialize a mutex with the default mutex attributes. + */ +#define PTHREAD_MUTEX_INITIALIZER (-1) + /** * @brief Declare a pthread mutex * @@ -137,14 +144,9 @@ static inline int pthread_condattr_destroy(pthread_condattr_t *att) * kernel objects. * * @param name Symbol name of the mutex + * @deprecated Use @c PTHREAD_MUTEX_INITIALIZER instead. */ -#define PTHREAD_MUTEX_DEFINE(name) \ - struct pthread_mutex name = \ - { \ - .lock_count = 0, \ - .wait_q = Z_WAIT_Q_INIT(&name.wait_q), \ - .owner = NULL, \ - } +#define PTHREAD_MUTEX_DEFINE(name) pthread_mutex_t name = PTHREAD_MUTEX_INITIALIZER /* * Mutex attributes - type diff --git a/lib/posix/Kconfig b/lib/posix/Kconfig index 8bb56649e3993..661c2bbeb0e2d 100644 --- a/lib/posix/Kconfig +++ b/lib/posix/Kconfig @@ -35,6 +35,13 @@ config MAX_PTHREAD_COUNT help Maximum number of simultaneously active threads in a POSIX application. +config MAX_PTHREAD_MUTEX_COUNT + int "Maximum simultaneously active mutex count in POSIX application" + default 5 + range 0 255 + help + Maximum number of simultaneously active mutexes 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 6da0c8be0ad59..c0bfba72938af 100644 --- a/lib/posix/posix_internal.h +++ b/lib/posix/posix_internal.h @@ -7,6 +7,19 @@ #ifndef ZEPHYR_LIB_POSIX_POSIX_INTERNAL_H_ #define ZEPHYR_LIB_POSIX_POSIX_INTERNAL_H_ +/* + * Bit used to mark a pthread_mutex_t as initialized. Initialization status is + * verified (against internal status) in lock / unlock / destroy functions. + */ +#define PTHREAD_MUTEX_MASK_INIT 0x80000000 + +struct posix_mutex { + k_tid_t owner; + uint16_t lock_count; + int type; + _wait_q_t wait_q; +}; + enum pthread_state { /* The thread structure is unallocated and available for reuse. */ PTHREAD_TERMINATED = 0, @@ -40,4 +53,25 @@ struct posix_thread { 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_mutex_initialized(pthread_mutex_t mut) +{ + return (mut & PTHREAD_MUTEX_MASK_INIT) != 0; +} + +static inline pthread_mutex_t mark_pthread_mutex_initialized(pthread_mutex_t mut) +{ + return mut | PTHREAD_MUTEX_MASK_INIT; +} + +static inline pthread_mutex_t mark_pthread_mutex_uninitialized(pthread_mutex_t mut) +{ + return mut & ~PTHREAD_MUTEX_MASK_INIT; +} + #endif diff --git a/lib/posix/pthread_cond.c b/lib/posix/pthread_cond.c index 03f59305e3f51..ea9cfe24163c8 100644 --- a/lib/posix/pthread_cond.c +++ b/lib/posix/pthread_cond.c @@ -9,21 +9,30 @@ #include #include +#include "posix_internal.h" + 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 *mut, +static int cond_wait(pthread_cond_t *cv, pthread_mutex_t *mu, k_timeout_t timeout) { - __ASSERT(mut->lock_count == 1U, ""); - int ret; - k_spinlock_key_t key = k_spin_lock(&z_pthread_spinlock); + k_spinlock_key_t key; + struct posix_mutex *m; + + key = k_spin_lock(&z_pthread_spinlock); + m = to_posix_mutex(mu); + if (m == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } - mut->lock_count = 0U; - mut->owner = NULL; - _ready_one_thread(&mut->wait_q); + __ASSERT_NO_MSG(m->lock_count == 1U); + m->lock_count = 0U; + m->owner = NULL; + _ready_one_thread(&m->wait_q); ret = z_sched_wait(&z_pthread_spinlock, key, &cv->wait_q, timeout, NULL); /* FIXME: this extra lock (and the potential context switch it @@ -34,7 +43,7 @@ static int cond_wait(pthread_cond_t *cv, pthread_mutex_t *mut, * But that requires putting scheduler intelligence into this * higher level abstraction and is probably not worth it. */ - pthread_mutex_lock(mut); + pthread_mutex_lock(mu); return ret == -EAGAIN ? ETIMEDOUT : ret; } diff --git a/lib/posix/pthread_mutex.c b/lib/posix/pthread_mutex.c index 891bcb4d7db0c..29a878f58bdc8 100644 --- a/lib/posix/pthread_mutex.c +++ b/lib/posix/pthread_mutex.c @@ -8,6 +8,9 @@ #include #include #include +#include + +#include "posix_internal.h" struct k_spinlock z_pthread_spinlock; @@ -22,10 +25,92 @@ static const pthread_mutexattr_t def_attr = { .type = PTHREAD_MUTEX_DEFAULT, }; -static int acquire_mutex(pthread_mutex_t *m, k_timeout_t timeout) +static struct posix_mutex posix_mutex_pool[CONFIG_MAX_PTHREAD_MUTEX_COUNT]; +SYS_BITARRAY_DEFINE_STATIC(posix_mutex_bitarray, CONFIG_MAX_PTHREAD_MUTEX_COUNT); + +/* + * We reserve the MSB to mark a pthread_mutex_t as initialized (from the + * 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, + "CONFIG_MAX_PTHREAD_MUTEX_COUNT is too high"); + +static inline size_t posix_mutex_to_offset(struct posix_mutex *m) +{ + return m - posix_mutex_pool; +} + +static inline size_t to_posix_mutex_idx(pthread_mutex_t mut) +{ + return mark_pthread_mutex_uninitialized(mut); +} + +struct posix_mutex *get_posix_mutex(pthread_mutex_t mu) +{ + int actually_initialized; + 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)) { + return NULL; + } + + /* Mask off the MSB to get the actual bit index */ + if (sys_bitarray_test_bit(&posix_mutex_bitarray, bit, &actually_initialized) < 0) { + return NULL; + } + + if (actually_initialized == 0) { + /* The mutex claims to be initialized but is actually not */ + return NULL; + } + + return &posix_mutex_pool[bit]; +} + +struct posix_mutex *to_posix_mutex(pthread_mutex_t *mu) +{ + size_t bit; + struct posix_mutex *m; + + if (*mu != PTHREAD_MUTEX_INITIALIZER) { + return get_posix_mutex(*mu); + } + + /* Try and automatically associate a posix_mutex */ + if (sys_bitarray_alloc(&posix_mutex_bitarray, 1, &bit) < 0) { + /* No mutexes left to allocate */ + return NULL; + } + + /* Record the associated posix_mutex in mu and mark as initialized */ + *mu = mark_pthread_mutex_initialized(bit); + + /* Initialize the posix_mutex */ + m = &posix_mutex_pool[bit]; + + m->owner = NULL; + m->lock_count = 0U; + + z_waitq_init(&m->wait_q); + + return m; +} + +static int acquire_mutex(pthread_mutex_t *mu, k_timeout_t timeout) { int rc = 0; - k_spinlock_key_t key = k_spin_lock(&z_pthread_spinlock); + k_spinlock_key_t key; + struct posix_mutex *m; + + key = k_spin_lock(&z_pthread_spinlock); + + m = to_posix_mutex(mu); + if (m == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } if (m->lock_count == 0U && m->owner == NULL) { m->lock_count++; @@ -89,19 +174,24 @@ int pthread_mutex_timedlock(pthread_mutex_t *m, * * See IEEE 1003.1 */ -int pthread_mutex_init(pthread_mutex_t *m, +int pthread_mutex_init(pthread_mutex_t *mu, const pthread_mutexattr_t *attr) { - const pthread_mutexattr_t *mattr; + k_spinlock_key_t key; + struct posix_mutex *m; - m->owner = NULL; - m->lock_count = 0U; + *mu = PTHREAD_MUTEX_INITIALIZER; + key = k_spin_lock(&z_pthread_spinlock); - mattr = (attr == NULL) ? &def_attr : attr; + m = to_posix_mutex(mu); + if (m == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return ENOMEM; + } - m->type = mattr->type; + m->type = (attr == NULL) ? def_attr.type : attr->type; - z_waitq_init(&m->wait_q); + k_spin_unlock(&z_pthread_spinlock, key); return 0; } @@ -122,11 +212,20 @@ int pthread_mutex_lock(pthread_mutex_t *m) * * See IEEE 1003.1 */ -int pthread_mutex_unlock(pthread_mutex_t *m) +int pthread_mutex_unlock(pthread_mutex_t *mu) { - k_spinlock_key_t key = k_spin_lock(&z_pthread_spinlock); - k_tid_t thread; + k_spinlock_key_t key; + struct posix_mutex *m; + pthread_mutex_t mut = *mu; + + key = k_spin_lock(&z_pthread_spinlock); + + m = get_posix_mutex(mut); + if (m == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } if (m->owner != k_current_get()) { k_spin_unlock(&z_pthread_spinlock, key); @@ -162,9 +261,26 @@ int pthread_mutex_unlock(pthread_mutex_t *m) * * See IEEE 1003.1 */ -int pthread_mutex_destroy(pthread_mutex_t *m) +int pthread_mutex_destroy(pthread_mutex_t *mu) { - ARG_UNUSED(m); + __unused int rc; + k_spinlock_key_t key; + struct posix_mutex *m; + pthread_mutex_t mut = *mu; + size_t bit = to_posix_mutex_idx(mut); + + key = k_spin_lock(&z_pthread_spinlock); + m = get_posix_mutex(mut); + if (m == NULL) { + k_spin_unlock(&z_pthread_spinlock, key); + return EINVAL; + } + + rc = sys_bitarray_free(&posix_mutex_bitarray, 1, bit); + __ASSERT(rc == 0, "failed to free bit %zu", bit); + + k_spin_unlock(&z_pthread_spinlock, key); + return 0; } From 2477815ecc0f4b99a81f5b9ae385f0787e836b39 Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Thu, 10 Nov 2022 08:56:14 -0500 Subject: [PATCH 3/3] tests: posix: mutex: test to ensure there is no resource leakage Add a test to ensure that `pthread_mutex_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/mutex.c | 73 ++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 21 deletions(-) diff --git a/tests/posix/common/src/mutex.c b/tests/posix/common/src/mutex.c index 280aa023b44a8..aa854476cff35 100644 --- a/tests/posix/common/src/mutex.c +++ b/tests/posix/common/src/mutex.c @@ -4,8 +4,10 @@ * SPDX-License-Identifier: Apache-2.0 */ -#include #include + +#include + #include #define STACK_SIZE (1024 + CONFIG_TEST_EXTRA_STACK_SIZE) @@ -33,21 +35,17 @@ void *normal_mutex_entry(void *p1) zassert_false(rc, "try lock failed"); TC_PRINT("mutex lock is taken\n"); - zassert_false(pthread_mutex_unlock(&mutex1), - "mutex unlock is failed"); + zassert_false(pthread_mutex_unlock(&mutex1), "mutex unlock is failed"); return NULL; } void *recursive_mutex_entry(void *p1) { zassert_false(pthread_mutex_lock(&mutex2), "mutex is not taken"); - zassert_false(pthread_mutex_lock(&mutex2), - "mutex is not taken 2nd time"); + zassert_false(pthread_mutex_lock(&mutex2), "mutex is not taken 2nd time"); TC_PRINT("recursive mutex lock is taken\n"); - zassert_false(pthread_mutex_unlock(&mutex2), - "mutex is not unlocked"); - zassert_false(pthread_mutex_unlock(&mutex2), - "mutex is not unlocked"); + zassert_false(pthread_mutex_unlock(&mutex2), "mutex is not unlocked"); + zassert_false(pthread_mutex_unlock(&mutex2), "mutex is not unlocked"); return NULL; } @@ -72,8 +70,7 @@ ZTEST(posix_apis, test_posix_normal_mutex) if (ret != 0) { zassert_false(pthread_attr_destroy(&attr), "Unable to destroy pthread object attrib"); - zassert_false(pthread_attr_init(&attr), - "Unable to create pthread object attrib"); + zassert_false(pthread_attr_init(&attr), "Unable to create pthread object attrib"); } pthread_attr_setstack(&attr, &stack, STACK_SIZE); @@ -92,11 +89,9 @@ ZTEST(posix_apis, test_posix_normal_mutex) pthread_mutex_lock(&mutex1); - zassert_equal(type, PTHREAD_MUTEX_NORMAL, - "mutex type is not normal"); + zassert_equal(type, PTHREAD_MUTEX_NORMAL, "mutex type is not normal"); - zassert_equal(protocol, PTHREAD_PRIO_NONE, - "mutex protocol is not prio_none"); + zassert_equal(protocol, PTHREAD_PRIO_NONE, "mutex protocol is not prio_none"); ret = pthread_create(&thread_1, &attr, &normal_mutex_entry, NULL); if (ret) { @@ -131,8 +126,7 @@ ZTEST(posix_apis, test_posix_recursive_mutex) if (ret != 0) { zassert_false(pthread_attr_destroy(&attr2), "Unable to destroy pthread object attrib"); - zassert_false(pthread_attr_init(&attr2), - "Unable to create pthread object attrib"); + zassert_false(pthread_attr_init(&attr2), "Unable to create pthread object attrib"); } pthread_attr_setstack(&attr2, &stack, STACK_SIZE); @@ -149,11 +143,9 @@ ZTEST(posix_apis, test_posix_recursive_mutex) temp = pthread_mutexattr_getprotocol(&mut_attr2, &protocol); zassert_false(temp, "reading mutex2 protocol is failed"); - zassert_equal(type, PTHREAD_MUTEX_RECURSIVE, - "mutex2 type is not recursive"); + zassert_equal(type, PTHREAD_MUTEX_RECURSIVE, "mutex2 type is not recursive"); - zassert_equal(protocol, PTHREAD_PRIO_NONE, - "mutex2 protocol is not prio_none"); + zassert_equal(protocol, PTHREAD_PRIO_NONE, "mutex2 protocol is not prio_none"); ret = pthread_create(&thread_2, &attr2, &recursive_mutex_entry, NULL); zassert_false(ret, "Thread2 creation failed"); @@ -162,3 +154,42 @@ ZTEST(posix_apis, test_posix_recursive_mutex) temp = pthread_mutex_destroy(&mutex2); zassert_false(temp, "Destroying mutex2 is failed"); } + +/** + * @brief Test to demonstrate limited mutex resources + * + * @details Exactly CONFIG_MAX_PTHREAD_MUTEX_COUNT can be in use at once. + */ +ZTEST(posix_apis, test_posix_mutex_resource_exhausted) +{ + size_t i; + pthread_mutex_t m[CONFIG_MAX_PTHREAD_MUTEX_COUNT + 1]; + + for (i = 0; i < CONFIG_MAX_PTHREAD_MUTEX_COUNT; ++i) { + zassert_ok(pthread_mutex_init(&m[i], NULL), "failed to init mutex %zu", i); + } + + /* try to initialize one more than CONFIG_MAX_PTHREAD_MUTEX_COUNT */ + zassert_equal(i, CONFIG_MAX_PTHREAD_MUTEX_COUNT); + zassert_not_equal(0, pthread_mutex_init(&m[i], NULL), + "should not have initialized mutex %zu", i); + + for (; i > 0; --i) { + zassert_ok(pthread_mutex_destroy(&m[i - 1]), "failed to destroy mutex %zu", i - 1); + } +} + +/** + * @brief Test to that there are no mutex resource leaks + * + * @details Demonstrate that mutexes may be used over and over again. + */ +ZTEST(posix_apis, test_posix_mutex_resource_leak) +{ + pthread_mutex_t m; + + for (size_t i = 0; i < 2 * CONFIG_MAX_PTHREAD_MUTEX_COUNT; ++i) { + zassert_ok(pthread_mutex_init(&m, NULL), "failed to init mutex %zu", i); + zassert_ok(pthread_mutex_destroy(&m), "failed to destroy mutex %zu", i); + } +}