From 89b9c87a2ea7ad2be2bddcc1763282498925c328 Mon Sep 17 00:00:00 2001 From: Yong Cong Sin Date: Tue, 26 Dec 2023 18:17:17 +0800 Subject: [PATCH 1/7] MAINTAINERS: add myself as a POSIX collaborator Add myself as a collaborator in the POSIX subsystem. Signed-off-by: Yong Cong Sin --- MAINTAINERS.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS.yml b/MAINTAINERS.yml index 1dffa8e82fc1..ef82c8e3e7cf 100644 --- a/MAINTAINERS.yml +++ b/MAINTAINERS.yml @@ -2730,6 +2730,8 @@ POSIX API layer: status: maintained maintainers: - cfriedt + collaborators: + - ycsin files: - include/zephyr/posix/ - lib/posix/ From f252dda6642deadebe6d8865d30f3ba2cf331a6b Mon Sep 17 00:00:00 2001 From: Yong Cong Sin Date: Tue, 26 Dec 2023 18:15:58 +0800 Subject: [PATCH 2/7] posix: semaphore: implement `sem_open()`, `sem_unlink()` & `sem_close()` Implements `sem_open()`, `sem_unlink()` & `sem_close()` functions and added tests for them. Updated existing tests and POSIX docs. Signed-off-by: Yong Cong Sin --- include/zephyr/posix/semaphore.h | 5 + lib/posix/Kconfig.semaphore | 8 + lib/posix/semaphore.c | 242 ++++++++++++++++++++++++++ tests/posix/common/src/semaphore.c | 191 ++++++++++++++++++++ tests/posix/headers/src/semaphore_h.c | 6 +- 5 files changed, 449 insertions(+), 3 deletions(-) diff --git a/include/zephyr/posix/semaphore.h b/include/zephyr/posix/semaphore.h index 943218ee08e8..3b0f53b07123 100644 --- a/include/zephyr/posix/semaphore.h +++ b/include/zephyr/posix/semaphore.h @@ -13,6 +13,8 @@ extern "C" { #endif +#define SEM_FAILED ((sem_t *) 0) + int sem_destroy(sem_t *semaphore); int sem_getvalue(sem_t *ZRESTRICT semaphore, int *ZRESTRICT value); int sem_init(sem_t *semaphore, int pshared, unsigned int value); @@ -20,6 +22,9 @@ int sem_post(sem_t *semaphore); int sem_timedwait(sem_t *ZRESTRICT semaphore, struct timespec *ZRESTRICT abstime); int sem_trywait(sem_t *semaphore); int sem_wait(sem_t *semaphore); +sem_t *sem_open(const char *name, int oflags, ...); +int sem_unlink(const char *name); +int sem_close(sem_t *sem); #ifdef __cplusplus } diff --git a/lib/posix/Kconfig.semaphore b/lib/posix/Kconfig.semaphore index d9b9b47a5089..aa3468fea763 100644 --- a/lib/posix/Kconfig.semaphore +++ b/lib/posix/Kconfig.semaphore @@ -8,3 +8,11 @@ config SEM_VALUE_MAX range 1 32767 help Maximum semaphore count in POSIX compliant Application. + +config SEM_NAMELEN_MAX + int "Maximum name length" + default 16 + range 2 255 + help + Maximum length of name for a named semaphore. + The max value of 255 corresponds to {NAME_MAX}. diff --git a/lib/posix/semaphore.c b/lib/posix/semaphore.c index 7a1392cd5211..804ca08de1f0 100644 --- a/lib/posix/semaphore.c +++ b/lib/posix/semaphore.c @@ -1,11 +1,75 @@ /* * Copyright (c) 2018 Intel Corporation + * Copyright (c) 2023 Meta * * SPDX-License-Identifier: Apache-2.0 */ #include +#include +#include +#include #include +#include + +struct nsem_obj { + sys_snode_t snode; + sem_t sem; + unsigned int ref_count; + char *name; +}; + +/* Initialize the list */ +static sys_slist_t nsem_list = SYS_SLIST_STATIC_INIT(&nsem_list); + +static K_MUTEX_DEFINE(nsem_mutex); + +static inline void nsem_list_lock(void) +{ + k_mutex_lock(&nsem_mutex, K_FOREVER); +} + +static inline void nsem_list_unlock(void) +{ + k_mutex_unlock(&nsem_mutex); +} + +static struct nsem_obj *nsem_find(const char *name) +{ + struct nsem_obj *nsem; + + SYS_SLIST_FOR_EACH_CONTAINER(&nsem_list, nsem, snode) { + if ((nsem->name != NULL) && (strcmp(nsem->name, name) == 0)) { + return nsem; + } + } + + return NULL; +} + +/* Clean up a named semaphore object completely (incl its `name` buffer) */ +static void nsem_cleanup(struct nsem_obj *nsem) +{ + if (nsem != NULL) { + if (nsem->name != NULL) { + k_free(nsem->name); + nsem->name = NULL; + } + k_free(nsem); + nsem = NULL; + } +} + +/* Remove a named semaphore if it isn't unsed */ +static void nsem_remove_if_unused(struct nsem_obj *nsem) +{ + if (nsem->ref_count == 0) { + sys_slist_find_and_remove(&nsem_list, (sys_snode_t *) nsem); + + /* Free nsem */ + nsem_cleanup(nsem); + } +} /** * @brief Destroy semaphore. @@ -148,3 +212,181 @@ int sem_wait(sem_t *semaphore) (void)k_sem_take(semaphore, K_FOREVER); return 0; } + +sem_t *sem_open(const char *name, int oflags, ...) +{ + va_list va; + mode_t mode; + unsigned int value; + struct nsem_obj *nsem = NULL; + size_t namelen; + + va_start(va, oflags); + BUILD_ASSERT(sizeof(mode_t) <= sizeof(int)); + mode = va_arg(va, int); + value = va_arg(va, unsigned int); + va_end(va); + + if (value > CONFIG_SEM_VALUE_MAX) { + errno = EINVAL; + return (sem_t *)SEM_FAILED; + } + + if (name == NULL) { + errno = EINVAL; + return (sem_t *)SEM_FAILED; + } + + namelen = strlen(name); + if ((namelen + 1) > CONFIG_SEM_NAMELEN_MAX) { + errno = ENAMETOOLONG; + return (sem_t *)SEM_FAILED; + } + + /* Lock before checking to make sure that the call is atomic */ + nsem_list_lock(); + + /* Check if the named semaphore exists */ + nsem = nsem_find(name); + + if (nsem != NULL) { /* Named semaphore exists */ + if (((oflags & O_CREAT) != 0) && ((oflags & O_EXCL) != 0)) { + errno = EEXIST; + goto error_unlock; + } + + __ASSERT_NO_MSG(nsem->ref_count != UINT_MAX); + nsem->ref_count++; + goto unlock; + } + + /* Named sempahore doesn't exist, try to create new one */ + + if ((oflags & O_CREAT) == 0) { + errno = ENOENT; + goto error_unlock; + } + + nsem = k_calloc(1, sizeof(struct nsem_obj)); + if (nsem == NULL) { + errno = ENOSPC; + goto error_unlock; + } + + /* goto `cleanup_error_unlock` past this point to avoid memory leak */ + + nsem->name = k_calloc(namelen + 1, sizeof(uint8_t)); + if (nsem->name == NULL) { + errno = ENOSPC; + goto cleanup_error_unlock; + } + + strcpy(nsem->name, name); + nsem->ref_count = 1; + + (void)k_sem_init(&nsem->sem, value, CONFIG_SEM_VALUE_MAX); + + sys_slist_append(&nsem_list, (sys_snode_t *)&(nsem->snode)); + + goto unlock; + +cleanup_error_unlock: + nsem_cleanup(nsem); + +error_unlock: + nsem = NULL; + +unlock: + nsem_list_unlock(); + return nsem == NULL ? SEM_FAILED : &nsem->sem; +} + +int sem_unlink(const char *name) +{ + int ret = 0; + struct nsem_obj *nsem; + + if (name == NULL) { + errno = EINVAL; + return -1; + } + + if ((strlen(name) + 1) > CONFIG_SEM_NAMELEN_MAX) { + errno = ENAMETOOLONG; + return -1; + } + + nsem_list_lock(); + + /* Check if queue already exists */ + nsem = nsem_find(name); + if (nsem == NULL) { + ret = -1; + errno = ENOENT; + goto unlock; + } + + k_free(nsem->name); + nsem->name = NULL; + + nsem_remove_if_unused(nsem); + +unlock: + nsem_list_unlock(); + return ret; +} + +int sem_close(sem_t *sem) +{ + struct nsem_obj *nsem = CONTAINER_OF(sem, struct nsem_obj, sem); + + if (sem == NULL) { + errno = EINVAL; + return -1; + } + + __ASSERT_NO_MSG(nsem != NULL); + + nsem_list_lock(); + + __ASSERT_NO_MSG(nsem->ref_count != 0); + nsem->ref_count--; + + /* remove sem if marked for unlink */ + if (nsem->name == NULL) { + nsem_remove_if_unused(nsem); + } + + nsem_list_unlock(); + return 0; +} + +#ifdef CONFIG_ZTEST +/* Used by ztest to get the ref count of a named semaphore */ +unsigned int nsem_get_ref_count(sem_t *sem) +{ + struct nsem_obj *nsem = CONTAINER_OF(sem, struct nsem_obj, sem); + unsigned int ref_count; + + __ASSERT_NO_MSG(sem != NULL); + __ASSERT_NO_MSG(nsem != NULL); + + nsem_list_lock(); + ref_count = nsem->ref_count; + nsem_list_unlock(); + + return ref_count; +} + +/* Used by ztest to get the length of the named semaphore */ +size_t nsem_get_list_len(void) +{ + size_t len; + + nsem_list_lock(); + len = sys_slist_len(&nsem_list); + nsem_list_unlock(); + + return len; +} +#endif diff --git a/tests/posix/common/src/semaphore.c b/tests/posix/common/src/semaphore.c index 921ddbb4a416..1e8810683e1b 100644 --- a/tests/posix/common/src/semaphore.c +++ b/tests/posix/common/src/semaphore.c @@ -1,10 +1,12 @@ /* * Copyright (c) 2018 Intel Corporation + * Copyright (c) 2023 Meta * * SPDX-License-Identifier: Apache-2.0 */ #include +#include #include #include @@ -94,3 +96,192 @@ ZTEST(posix_apis, test_semaphore) /* TESTPOINT: Wait and acquire semaphore till thread2 gives */ zassert_equal(sem_wait(&sema), 0, "sem_wait failed"); } + +unsigned int nsem_get_ref_count(sem_t *sem); +size_t nsem_get_list_len(void); +#define N_LOOPS 999 + +static void *nsem_open_func(void *p) +{ + const char *name = (char *)p; + + for (int i = 0; i < N_LOOPS; i++) { + zassert_not_null(sem_open(name, 0, 0, 0), "%s is NULL", name); + k_msleep(1); + } + + /* Unlink after finished opening */ + zassert_ok(sem_unlink(name)); + return NULL; +} + +static void *nsem_close_func(void *p) +{ + sem_t *sem = (sem_t *)p; + + /* Make sure that we have enough ref_count's initially */ + k_msleep(N_LOOPS >> 1); + + for (int i = 0; i < N_LOOPS; i++) { + zassert_ok(sem_close(sem)); + k_msleep(1); + } + + /* Close the last `sem` */ + zassert_ok(sem_close(sem)); + return NULL; +} + +ZTEST(posix_apis, test_named_semaphore) +{ + pthread_t thread1, thread2; + sem_t *sem1, *sem2, *different_sem1; + + /* If `name` is invalid */ + sem1 = sem_open(NULL, 0, 0, 0); + zassert_equal(errno, EINVAL); + zassert_equal_ptr(sem1, SEM_FAILED); + zassert_equal(nsem_get_list_len(), 0); + + /* Attempt to open a named sem that doesn't exist */ + sem1 = sem_open("sem1", 0, 0, 0); + zassert_equal(errno, ENOENT); + zassert_equal_ptr(sem1, SEM_FAILED); + zassert_equal(nsem_get_list_len(), 0); + + /* Name exceeds CONFIG_SEM_NAMELEN_MAX */ + char name_too_long[CONFIG_SEM_NAMELEN_MAX + 2]; + + for (size_t i = 0; i < sizeof(name_too_long) - 1; i++) { + name_too_long[i] = 'a'; + } + name_too_long[sizeof(name_too_long) - 1] = '\0'; + + sem1 = sem_open(name_too_long, 0, 0, 0); + zassert_equal(errno, ENAMETOOLONG, "\"%s\" should be longer than %d", name_too_long, + CONFIG_SEM_NAMELEN_MAX); + zassert_equal_ptr(sem1, SEM_FAILED); + zassert_equal(nsem_get_list_len(), 0); + + /* `value` greater than CONFIG_SEM_VALUE_MAX */ + sem1 = sem_open("sem1", O_CREAT, 0, (CONFIG_SEM_VALUE_MAX + 1)); + zassert_equal(errno, EINVAL); + zassert_equal_ptr(sem1, SEM_FAILED); + zassert_equal(nsem_get_list_len(), 0); + + /* Open named sem */ + sem1 = sem_open("sem1", O_CREAT, 0, 0); + zassert_equal(nsem_get_ref_count(sem1), 1); + zassert_equal(nsem_get_list_len(), 1); + sem2 = sem_open("sem2", O_CREAT, 0, 0); + zassert_equal(nsem_get_ref_count(sem2), 1); + zassert_equal(nsem_get_list_len(), 2); + + /* Open created named sem repeatedly */ + for (size_t i = 1; i < N_LOOPS; i++) { + sem_t *new_sem1, *new_sem2; + + /* oflags are ignored (except when both O_CREAT & O_EXCL are set) */ + new_sem1 = sem_open("sem1", i % 2 == 0 ? O_CREAT : 0, 0, 0); + zassert_not_null(new_sem1); + zassert_equal_ptr(new_sem1, sem1); /* Should point to the same sem */ + new_sem2 = sem_open("sem2", i % 2 == 0 ? O_CREAT : 0, 0, 0); + zassert_not_null(new_sem2); + zassert_equal_ptr(new_sem2, sem2); + + /* ref_count should increment */ + zassert_equal(nsem_get_ref_count(sem1), i + 1); + zassert_equal(nsem_get_ref_count(sem2), i + 1); + + /* Should reuse the same named sem instead of creating another one */ + zassert_equal(nsem_get_list_len(), 2); + } + + /* O_CREAT and O_EXCL are set and the named semaphore already exists */ + zassert_equal_ptr((sem_open("sem1", O_CREAT | O_EXCL, 0, 0)), SEM_FAILED); + zassert_equal(errno, EEXIST); + zassert_equal(nsem_get_list_len(), 2); + + zassert_equal(sem_close(NULL), -1); + zassert_equal(errno, EINVAL); + zassert_equal(nsem_get_list_len(), 2); + + /* Close sem */ + for (size_t i = 0; + /* close until one left, required by the test later */ + i < (N_LOOPS - 1); i++) { + zassert_ok(sem_close(sem1)); + zassert_not_null(sem1); + zassert_equal(nsem_get_ref_count(sem1), N_LOOPS - (i + 1)); + + zassert_ok(sem_close(sem2)); + zassert_not_null(sem2); + zassert_equal(nsem_get_ref_count(sem2), N_LOOPS - (i + 1)); + + zassert_equal(nsem_get_list_len(), 2); + } + + /* If `name` is invalid */ + zassert_equal(sem_unlink(NULL), -1); + zassert_equal(errno, EINVAL); + zassert_equal(nsem_get_list_len(), 2); + + /* Attempt to unlink a named sem that doesn't exist */ + zassert_equal(sem_unlink("sem3"), -1); + zassert_equal(errno, ENOENT); + zassert_equal(nsem_get_list_len(), 2); + + /* Name exceeds CONFIG_SEM_NAMELEN_MAX */ + char long_sem_name[CONFIG_SEM_NAMELEN_MAX + 2]; + + for (int i = 0; i < CONFIG_SEM_NAMELEN_MAX + 1; i++) { + long_sem_name[i] = 'a'; + } + long_sem_name[CONFIG_SEM_NAMELEN_MAX + 1] = '\0'; + + zassert_equal(sem_unlink(long_sem_name), -1); + zassert_equal(errno, ENAMETOOLONG); + zassert_equal(nsem_get_list_len(), 2); + + /* Unlink sem1 when it is still being used */ + zassert_equal(nsem_get_ref_count(sem1), 1); + zassert_ok(sem_unlink("sem1")); + /* sem won't be destroyed */ + zassert_equal(nsem_get_list_len(), 2); + + /* Create another sem with the name of an unlinked sem */ + different_sem1 = sem_open("sem1", O_CREAT, 0, 0); + zassert_not_null(different_sem1); + /* The created sem will be a different instance */ + zassert(different_sem1 != sem1, ""); + zassert_equal(nsem_get_list_len(), 3); + + /* Destruction of sem1 will be postponed until all references to the semaphore have been + * destroyed by calls to sem_close() + */ + zassert_ok(sem_close(sem1)); + zassert_equal(nsem_get_list_len(), 2); + + /* Closing a linked sem won't destroy the sem */ + zassert_ok(sem_close(sem2)); + zassert_equal(nsem_get_ref_count(sem2), 0); + zassert_equal(nsem_get_list_len(), 2); + + /* Instead the sem will be destroyed upon call to sem_unlink() */ + zassert_ok(sem_unlink("sem2")); + zassert_equal(nsem_get_list_len(), 1); + + /* What we have left open here is `different_sem` as "sem1", which has 1 ref_count */ + zassert_equal(nsem_get_ref_count(different_sem1), 1); + + /* Stress test: open & close "sem1" repeatedly */ + zassert_ok(pthread_create(&thread1, NULL, nsem_open_func, "sem1")); + zassert_ok(pthread_create(&thread2, NULL, nsem_close_func, different_sem1)); + + /* Make sure the threads are terminated */ + zassert_ok(pthread_join(thread1, NULL)); + zassert_ok(pthread_join(thread2, NULL)); + + /* All named semaphores should be destroyed here */ + zassert_equal(nsem_get_list_len(), 0); +} diff --git a/tests/posix/headers/src/semaphore_h.c b/tests/posix/headers/src/semaphore_h.c index 0ac1de36f2eb..b4c2e151fa1d 100644 --- a/tests/posix/headers/src/semaphore_h.c +++ b/tests/posix/headers/src/semaphore_h.c @@ -22,15 +22,15 @@ ZTEST(posix_headers, test_semaphore_h) /* zassert_not_equal(SEM_FAILED, (sem_t *)42); */ /* not implemented */ if (IS_ENABLED(CONFIG_POSIX_API)) { - /* zassert_not_null(sem_close); */ /* not implemented */ + zassert_not_null(sem_close); zassert_not_null(sem_destroy); zassert_not_null(sem_getvalue); zassert_not_null(sem_init); - /* zassert_not_null(sem_open); */ /* not implemented */ + zassert_not_null(sem_open); zassert_not_null(sem_post); zassert_not_null(sem_timedwait); zassert_not_null(sem_trywait); - /* zassert_not_null(sem_unlink); */ /* not implemented */ + zassert_not_null(sem_unlink); zassert_not_null(sem_wait); } } From 23788b882e02b0d8e60bd4d52b6bc7342f1b1d22 Mon Sep 17 00:00:00 2001 From: Yong Cong Sin Date: Tue, 2 Jan 2024 11:34:13 +0800 Subject: [PATCH 3/7] doc: posix: mark `sem_open`, `sem_close` & `sem_unlink` as supported The `sem_open()`, `sem_close()` & `sem_unlink()` functions are now implemented, so mark them as supported. Signed-off-by: Yong Cong Sin --- doc/services/portability/posix/option_groups/index.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/services/portability/posix/option_groups/index.rst b/doc/services/portability/posix/option_groups/index.rst index 893754f647d1..edbd6899841b 100644 --- a/doc/services/portability/posix/option_groups/index.rst +++ b/doc/services/portability/posix/option_groups/index.rst @@ -367,14 +367,14 @@ POSIX_SEMAPHORES :header: API, Supported :widths: 50,10 - sem_close(), + sem_close(),yes sem_destroy(),yes sem_getvalue(),yes sem_init(),yes - sem_open(), + sem_open(),yes sem_post(),yes sem_trywait(),yes - sem_unlink(), + sem_unlink(),yes sem_wait(),yes .. _posix_option_group_spin_locks: From eb1f8c8fbe651bd282cb6854ffde0926b6673ca6 Mon Sep 17 00:00:00 2001 From: Yong Cong Sin Date: Tue, 9 Jan 2024 22:57:44 +0800 Subject: [PATCH 4/7] tests: posix: semaphore: refactor `test_semaphore` Localize a few public variables and refactor the test and functions so that they are reusable. Signed-off-by: Yong Cong Sin --- tests/posix/common/src/semaphore.c | 68 +++++++++++++++++------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/tests/posix/common/src/semaphore.c b/tests/posix/common/src/semaphore.c index 1e8810683e1b..e17300d91e8e 100644 --- a/tests/posix/common/src/semaphore.c +++ b/tests/posix/common/src/semaphore.c @@ -13,16 +13,15 @@ #include #include -static sem_t sema; -static void *dummy_sem; - static void *child_func(void *p1) { - zassert_equal(sem_post(&sema), 0, "sem_post failed"); + sem_t *sem = (sem_t *)p1; + + zassert_equal(sem_post(sem), 0, "sem_post failed"); return NULL; } -ZTEST(posix_apis, test_semaphore) +static void semaphore_test(sem_t *sem) { pthread_t thread1, thread2; int val, ret; @@ -31,28 +30,23 @@ ZTEST(posix_apis, test_semaphore) /* TESTPOINT: Check if sema value is less than * CONFIG_SEM_VALUE_MAX */ - zassert_equal(sem_init(&sema, 0, (CONFIG_SEM_VALUE_MAX + 1)), -1, + zassert_equal(sem_init(sem, 0, (CONFIG_SEM_VALUE_MAX + 1)), -1, "value larger than %d\n", CONFIG_SEM_VALUE_MAX); zassert_equal(errno, EINVAL); - zassert_equal(sem_init(&sema, 0, 0), 0, "sem_init failed"); - - /* TESTPOINT: Call sem_post with invalid kobject */ - zassert_equal(sem_post(dummy_sem), -1, "sem_post of" - " invalid semaphore object didn't fail"); - zassert_equal(errno, EINVAL); + zassert_equal(sem_init(sem, 0, 0), 0, "sem_init failed"); /* TESTPOINT: Check if semaphore value is as set */ - zassert_equal(sem_getvalue(&sema, &val), 0); + zassert_equal(sem_getvalue(sem, &val), 0); zassert_equal(val, 0); /* TESTPOINT: Check if sema is acquired when it * is not available */ - zassert_equal(sem_trywait(&sema), -1); + zassert_equal(sem_trywait(sem), -1); zassert_equal(errno, EAGAIN); - ret = pthread_create(&thread1, NULL, child_func, NULL); + ret = pthread_create(&thread1, NULL, child_func, sem); zassert_equal(ret, 0, "Thread creation failed"); zassert_equal(clock_gettime(CLOCK_REALTIME, &abstime), 0, @@ -63,38 +57,54 @@ ZTEST(posix_apis, test_semaphore) /* TESPOINT: Wait for 5 seconds and acquire sema given * by thread1 */ - zassert_equal(sem_timedwait(&sema, &abstime), 0); + zassert_equal(sem_timedwait(sem, &abstime), 0); /* TESTPOINT: Semaphore is already acquired, check if * no semaphore is available */ - zassert_equal(sem_timedwait(&sema, &abstime), -1); + zassert_equal(sem_timedwait(sem, &abstime), -1); zassert_equal(errno, ETIMEDOUT); - /* TESTPOINT: sem_destroy with invalid kobject */ - zassert_equal(sem_destroy(dummy_sem), -1, "invalid" - " semaphore is destroyed"); - zassert_equal(errno, EINVAL); - - zassert_equal(sem_destroy(&sema), 0, "semaphore is not destroyed"); + zassert_equal(sem_destroy(sem), 0, "semaphore is not destroyed"); /* TESTPOINT: Initialize sema with 1 */ - zassert_equal(sem_init(&sema, 0, 1), 0, "sem_init failed"); - zassert_equal(sem_getvalue(&sema, &val), 0); + zassert_equal(sem_init(sem, 0, 1), 0, "sem_init failed"); + zassert_equal(sem_getvalue(sem, &val), 0); zassert_equal(val, 1); - zassert_equal(sem_destroy(&sema), -1, "acquired semaphore" + zassert_equal(sem_destroy(sem), -1, "acquired semaphore" " is destroyed"); zassert_equal(errno, EBUSY); /* TESTPOINT: take semaphore which is initialized with 1 */ - zassert_equal(sem_trywait(&sema), 0); + zassert_equal(sem_trywait(sem), 0); - zassert_equal(pthread_create(&thread2, NULL, child_func, NULL), 0, + zassert_equal(pthread_create(&thread2, NULL, child_func, sem), 0, "Thread creation failed"); /* TESTPOINT: Wait and acquire semaphore till thread2 gives */ - zassert_equal(sem_wait(&sema), 0, "sem_wait failed"); + zassert_equal(sem_wait(sem), 0, "sem_wait failed"); + + /* Make sure the threads are terminated */ + zassert_ok(pthread_join(thread1, NULL)); + zassert_ok(pthread_join(thread2, NULL)); +} + +ZTEST(posix_apis, test_semaphore) +{ + sem_t sema; + + /* TESTPOINT: Call sem_post with invalid kobject */ + zassert_equal(sem_post(NULL), -1, "sem_post of" + " invalid semaphore object didn't fail"); + zassert_equal(errno, EINVAL); + + /* TESTPOINT: sem_destroy with invalid kobject */ + zassert_equal(sem_destroy(NULL), -1, "invalid" + " semaphore is destroyed"); + zassert_equal(errno, EINVAL); + + semaphore_test(&sema); } unsigned int nsem_get_ref_count(sem_t *sem); From 9396cd154f74efacb83bb0002e6aff6445cc8c27 Mon Sep 17 00:00:00 2001 From: Yong Cong Sin Date: Tue, 9 Jan 2024 22:59:45 +0800 Subject: [PATCH 5/7] tests: posix: semaphore: run normal semaphore test with named semaphore Run the normal semaphore test with a named semaphore. Signed-off-by: Yong Cong Sin --- tests/posix/common/src/semaphore.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/posix/common/src/semaphore.c b/tests/posix/common/src/semaphore.c index e17300d91e8e..fdaba371964f 100644 --- a/tests/posix/common/src/semaphore.c +++ b/tests/posix/common/src/semaphore.c @@ -294,4 +294,21 @@ ZTEST(posix_apis, test_named_semaphore) /* All named semaphores should be destroyed here */ zassert_equal(nsem_get_list_len(), 0); + + /* Create a new named sem to be used in the normal semaphore test */ + sem1 = sem_open("nsem", O_CREAT, 0, 0); + zassert_equal(nsem_get_list_len(), 1); + zassert_equal(nsem_get_ref_count(sem1), 1); + + /* Run the semaphore test with the created named semaphore */ + semaphore_test(sem1); + + /* List length and ref_count shouldn't change after the test */ + zassert_equal(nsem_get_list_len(), 1); + zassert_equal(nsem_get_ref_count(sem1), 1); + + /* Unless it is unlinked and closed */ + sem_unlink("nsem"); + sem_close(sem1); + zassert_equal(nsem_get_list_len(), 0); } From 62e737841bb9c2d6c79da5ef944749b8c816cc07 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Tue, 9 Jan 2024 12:42:19 -0500 Subject: [PATCH 6/7] posix: semaphore: optimize named semaphore implementation a bit - Regroup refcount decrement and semaphore destruction by making the linked state into a counted reference for it. This allows for simplifying the code and cleanly adding a few assertions in a common location. - Remove redundant initialization to NULL on memory about to be freed and local pointer in nsem_cleanup(). Signed-off-by: Nicolas Pitre --- lib/posix/semaphore.c | 36 +++++++++++++----------------- tests/posix/common/src/semaphore.c | 2 +- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/lib/posix/semaphore.c b/lib/posix/semaphore.c index 804ca08de1f0..07ec11add4e5 100644 --- a/lib/posix/semaphore.c +++ b/lib/posix/semaphore.c @@ -15,7 +15,7 @@ struct nsem_obj { sys_snode_t snode; sem_t sem; - unsigned int ref_count; + int ref_count; char *name; }; @@ -53,17 +53,20 @@ static void nsem_cleanup(struct nsem_obj *nsem) if (nsem != NULL) { if (nsem->name != NULL) { k_free(nsem->name); - nsem->name = NULL; } k_free(nsem); - nsem = NULL; } } /* Remove a named semaphore if it isn't unsed */ -static void nsem_remove_if_unused(struct nsem_obj *nsem) +static void nsem_unref(struct nsem_obj *nsem) { + nsem->ref_count -= 1; + __ASSERT(nsem->ref_count >= 0, "ref_count may not be negative"); + if (nsem->ref_count == 0) { + __ASSERT(nsem->name == NULL, "ref_count is 0 but sem is not unlinked"); + sys_slist_find_and_remove(&nsem_list, (sys_snode_t *) nsem); /* Free nsem */ @@ -255,7 +258,7 @@ sem_t *sem_open(const char *name, int oflags, ...) goto error_unlock; } - __ASSERT_NO_MSG(nsem->ref_count != UINT_MAX); + __ASSERT_NO_MSG(nsem->ref_count != INT_MAX); nsem->ref_count++; goto unlock; } @@ -282,7 +285,9 @@ sem_t *sem_open(const char *name, int oflags, ...) } strcpy(nsem->name, name); - nsem->ref_count = 1; + + /* 1 for this open instance, +1 for the linked name */ + nsem->ref_count = 2; (void)k_sem_init(&nsem->sem, value, CONFIG_SEM_VALUE_MAX); @@ -328,8 +333,7 @@ int sem_unlink(const char *name) k_free(nsem->name); nsem->name = NULL; - - nsem_remove_if_unused(nsem); + nsem_unref(nsem); unlock: nsem_list_unlock(); @@ -345,28 +349,18 @@ int sem_close(sem_t *sem) return -1; } - __ASSERT_NO_MSG(nsem != NULL); - nsem_list_lock(); - - __ASSERT_NO_MSG(nsem->ref_count != 0); - nsem->ref_count--; - - /* remove sem if marked for unlink */ - if (nsem->name == NULL) { - nsem_remove_if_unused(nsem); - } - + nsem_unref(nsem); nsem_list_unlock(); return 0; } #ifdef CONFIG_ZTEST /* Used by ztest to get the ref count of a named semaphore */ -unsigned int nsem_get_ref_count(sem_t *sem) +int nsem_get_ref_count(sem_t *sem) { struct nsem_obj *nsem = CONTAINER_OF(sem, struct nsem_obj, sem); - unsigned int ref_count; + int ref_count; __ASSERT_NO_MSG(sem != NULL); __ASSERT_NO_MSG(nsem != NULL); diff --git a/tests/posix/common/src/semaphore.c b/tests/posix/common/src/semaphore.c index fdaba371964f..14eb54915ac6 100644 --- a/tests/posix/common/src/semaphore.c +++ b/tests/posix/common/src/semaphore.c @@ -107,7 +107,7 @@ ZTEST(posix_apis, test_semaphore) semaphore_test(&sema); } -unsigned int nsem_get_ref_count(sem_t *sem); +int nsem_get_ref_count(sem_t *sem); size_t nsem_get_list_len(void); #define N_LOOPS 999 From b316cca633a91705a1e2c1afb234fdb045cd582e Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Tue, 9 Jan 2024 12:55:48 -0500 Subject: [PATCH 7/7] tests: posix: semaphore: assorted adjustments - Adjust refcount checks with regards to previous commit. - Remove redundant zassert_not_null() on local pointers after a sem_close(). There is no implicit reference passing in C. Signed-off-by: Nicolas Pitre --- tests/posix/common/src/semaphore.c | 33 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/tests/posix/common/src/semaphore.c b/tests/posix/common/src/semaphore.c index 14eb54915ac6..fda34cfc23d5 100644 --- a/tests/posix/common/src/semaphore.c +++ b/tests/posix/common/src/semaphore.c @@ -181,14 +181,14 @@ ZTEST(posix_apis, test_named_semaphore) /* Open named sem */ sem1 = sem_open("sem1", O_CREAT, 0, 0); - zassert_equal(nsem_get_ref_count(sem1), 1); + zassert_equal(nsem_get_ref_count(sem1), 2); zassert_equal(nsem_get_list_len(), 1); sem2 = sem_open("sem2", O_CREAT, 0, 0); - zassert_equal(nsem_get_ref_count(sem2), 1); + zassert_equal(nsem_get_ref_count(sem2), 2); zassert_equal(nsem_get_list_len(), 2); /* Open created named sem repeatedly */ - for (size_t i = 1; i < N_LOOPS; i++) { + for (size_t i = 1; i <= N_LOOPS; i++) { sem_t *new_sem1, *new_sem2; /* oflags are ignored (except when both O_CREAT & O_EXCL are set) */ @@ -200,8 +200,8 @@ ZTEST(posix_apis, test_named_semaphore) zassert_equal_ptr(new_sem2, sem2); /* ref_count should increment */ - zassert_equal(nsem_get_ref_count(sem1), i + 1); - zassert_equal(nsem_get_ref_count(sem2), i + 1); + zassert_equal(nsem_get_ref_count(sem1), 2 + i); + zassert_equal(nsem_get_ref_count(sem2), 2 + i); /* Should reuse the same named sem instead of creating another one */ zassert_equal(nsem_get_list_len(), 2); @@ -217,16 +217,14 @@ ZTEST(posix_apis, test_named_semaphore) zassert_equal(nsem_get_list_len(), 2); /* Close sem */ - for (size_t i = 0; + for (size_t i = N_LOOPS; /* close until one left, required by the test later */ - i < (N_LOOPS - 1); i++) { + i >= 1; i--) { zassert_ok(sem_close(sem1)); - zassert_not_null(sem1); - zassert_equal(nsem_get_ref_count(sem1), N_LOOPS - (i + 1)); + zassert_equal(nsem_get_ref_count(sem1), 2 + i - 1); zassert_ok(sem_close(sem2)); - zassert_not_null(sem2); - zassert_equal(nsem_get_ref_count(sem2), N_LOOPS - (i + 1)); + zassert_equal(nsem_get_ref_count(sem2), 2 + i - 1); zassert_equal(nsem_get_list_len(), 2); } @@ -254,9 +252,10 @@ ZTEST(posix_apis, test_named_semaphore) zassert_equal(nsem_get_list_len(), 2); /* Unlink sem1 when it is still being used */ - zassert_equal(nsem_get_ref_count(sem1), 1); + zassert_equal(nsem_get_ref_count(sem1), 2); zassert_ok(sem_unlink("sem1")); /* sem won't be destroyed */ + zassert_equal(nsem_get_ref_count(sem1), 1); zassert_equal(nsem_get_list_len(), 2); /* Create another sem with the name of an unlinked sem */ @@ -274,15 +273,15 @@ ZTEST(posix_apis, test_named_semaphore) /* Closing a linked sem won't destroy the sem */ zassert_ok(sem_close(sem2)); - zassert_equal(nsem_get_ref_count(sem2), 0); + zassert_equal(nsem_get_ref_count(sem2), 1); zassert_equal(nsem_get_list_len(), 2); /* Instead the sem will be destroyed upon call to sem_unlink() */ zassert_ok(sem_unlink("sem2")); zassert_equal(nsem_get_list_len(), 1); - /* What we have left open here is `different_sem` as "sem1", which has 1 ref_count */ - zassert_equal(nsem_get_ref_count(different_sem1), 1); + /* What we have left open here is `different_sem` as "sem1", which has a ref_count of 2 */ + zassert_equal(nsem_get_ref_count(different_sem1), 2); /* Stress test: open & close "sem1" repeatedly */ zassert_ok(pthread_create(&thread1, NULL, nsem_open_func, "sem1")); @@ -298,14 +297,14 @@ ZTEST(posix_apis, test_named_semaphore) /* Create a new named sem to be used in the normal semaphore test */ sem1 = sem_open("nsem", O_CREAT, 0, 0); zassert_equal(nsem_get_list_len(), 1); - zassert_equal(nsem_get_ref_count(sem1), 1); + zassert_equal(nsem_get_ref_count(sem1), 2); /* Run the semaphore test with the created named semaphore */ semaphore_test(sem1); /* List length and ref_count shouldn't change after the test */ zassert_equal(nsem_get_list_len(), 1); - zassert_equal(nsem_get_ref_count(sem1), 1); + zassert_equal(nsem_get_ref_count(sem1), 2); /* Unless it is unlinked and closed */ sem_unlink("nsem");