From 31c4e346c910499dfd9c355eeabc7217f68f3941 Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Sat, 20 Jan 2024 08:14:02 -0500 Subject: [PATCH 1/4] posix: signal: reduce padding in sigevent sigval The sigevent struct and sigval union members were previously not ordered in a way that produces optimal alignment / reduces padding on 64-bit systems. Reorder members so that pointers come first. Signed-off-by: Christopher Friedt --- include/zephyr/posix/signal.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/zephyr/posix/signal.h b/include/zephyr/posix/signal.h index d22ac34c48da..3bbdee7317ac 100644 --- a/include/zephyr/posix/signal.h +++ b/include/zephyr/posix/signal.h @@ -80,16 +80,16 @@ typedef struct { typedef int sig_atomic_t; /* Atomic entity type (ANSI) */ union sigval { - int sival_int; void *sival_ptr; + int sival_int; }; struct sigevent { - int sigev_notify; - int sigev_signo; - union sigval sigev_value; void (*sigev_notify_function)(union sigval val); pthread_attr_t *sigev_notify_attributes; + union sigval sigev_value; + int sigev_notify; + int sigev_signo; }; #ifdef CONFIG_POSIX_SIGNAL From d9d592d980d47a28e6f665f9588feeb95b20a010 Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Sat, 20 Jan 2024 08:20:08 -0500 Subject: [PATCH 2/4] posix: timer: support other clocks There is no requirement that says e.g. CLOCK_REALTIME cannot be used for timer_create(). In fact, the spec explicitly requires it. It might not be ideal, but users should still be able to use it. Signed-off-by: Christopher Friedt --- lib/posix/timer.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/lib/posix/timer.c b/lib/posix/timer.c index da4272f3bf1d..7e5224203053 100644 --- a/lib/posix/timer.c +++ b/lib/posix/timer.c @@ -18,9 +18,7 @@ static void zephyr_timer_wrapper(struct k_timer *ztimer); struct timer_obj { struct k_timer ztimer; - void (*sigev_notify_function)(union sigval val); - union sigval val; - int sigev_notify; + struct sigevent sev; struct k_sem sem_cond; pthread_t thread; struct timespec interval; /* Reload value */ @@ -41,7 +39,7 @@ static void zephyr_timer_wrapper(struct k_timer *ztimer) timer->status = NOT_ACTIVE; } - (timer->sigev_notify_function)(timer->val); + (timer->sev.sigev_notify_function)(timer->sev.sigev_value); } static void *zephyr_thread_wrapper(void *arg) @@ -55,7 +53,7 @@ static void *zephyr_thread_wrapper(void *arg) k_sem_take(&timer->sem_cond, K_FOREVER); - (timer->sigev_notify_function)(timer->val); + (timer->sev.sigev_notify_function)(timer->sev.sigev_value); } return NULL; @@ -82,10 +80,8 @@ int timer_create(clockid_t clockid, struct sigevent *evp, timer_t *timerid) struct timer_obj *timer; const k_timeout_t alloc_timeout = K_MSEC(CONFIG_TIMER_CREATE_WAIT); - if (clockid != CLOCK_MONOTONIC || evp == NULL || - (evp->sigev_notify != SIGEV_NONE && - evp->sigev_notify != SIGEV_SIGNAL && - evp->sigev_notify != SIGEV_THREAD)) { + if (evp == NULL || (evp->sigev_notify != SIGEV_NONE && evp->sigev_notify != SIGEV_SIGNAL && + evp->sigev_notify != SIGEV_THREAD)) { errno = EINVAL; return -1; } @@ -97,8 +93,8 @@ int timer_create(clockid_t clockid, struct sigevent *evp, timer_t *timerid) return -1; } - timer->sigev_notify_function = evp->sigev_notify_function; - timer->val = evp->sigev_value; + timer->sev.sigev_notify_function = evp->sigev_notify_function; + timer->sev.sigev_value = evp->sigev_value; timer->interval.tv_sec = 0; timer->interval.tv_nsec = 0; timer->reload = 0U; @@ -109,7 +105,7 @@ int timer_create(clockid_t clockid, struct sigevent *evp, timer_t *timerid) } else if (evp->sigev_notify == SIGEV_THREAD) { int ret; - timer->sigev_notify = SIGEV_THREAD; + timer->sev.sigev_notify = SIGEV_THREAD; k_sem_init(&timer->sem_cond, 0, 1); ret = pthread_create(&timer->thread, evp->sigev_notify_attributes, zephyr_thread_wrapper, timer); @@ -266,8 +262,9 @@ int timer_delete(timer_t timerid) k_timer_stop(&timer->ztimer); } - if (timer->sigev_notify == SIGEV_THREAD) + if (timer->sev.sigev_notify == SIGEV_THREAD) { pthread_cancel(timer->thread); + } k_mem_slab_free(&posix_timer_slab, (void *)timer); From a4900810a6312887d57382ff7886d3d52fb7b8df Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Sat, 20 Jan 2024 09:07:21 -0500 Subject: [PATCH 3/4] posix: timer: use async pthread cancellation Previously, Zephyr's POSIX API did not differentiate between deferred and asynchronous pthread cancellation. In fact all pthread cancellation was asynchronous. According to the spec, all pthreads should be created with deferred cancellation by default. Note: PTHREAD_CANCEL_ASYNCHRONOUS means cancel asynchronously with respect to cancellation points (but synchronously with respect to the thread that callse pthread_cancel(), which is perhaps unintuitive). The POSIX timer relied on this non-standard convention. Oddly, this change prevents what would have otherwise been a regression that would have been caused by fixing pthread behaviour (in a separate commit). We are effectively uncovering bugs which were probably always present in the pthread.c and timer.c implementations going back quite a few years. Signed-off-by: Christopher Friedt --- lib/posix/timer.c | 132 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 101 insertions(+), 31 deletions(-) diff --git a/lib/posix/timer.c b/lib/posix/timer.c index 7e5224203053..7caecc180a12 100644 --- a/lib/posix/timer.c +++ b/lib/posix/timer.c @@ -1,24 +1,27 @@ /* * Copyright (c) 2018 Intel Corporation + * Copyright (c) 2024, Meta * * SPDX-License-Identifier: Apache-2.0 */ -#include #include -#include + +#include +#include #include -#include #include #include #define ACTIVE 1 #define NOT_ACTIVE 0 +LOG_MODULE_REGISTER(posix_timer); + static void zephyr_timer_wrapper(struct k_timer *ztimer); struct timer_obj { struct k_timer ztimer; - struct sigevent sev; + struct sigevent evp; struct k_sem sem_cond; pthread_t thread; struct timespec interval; /* Reload value */ @@ -37,23 +40,53 @@ static void zephyr_timer_wrapper(struct k_timer *ztimer) if (timer->reload == 0U) { timer->status = NOT_ACTIVE; + LOG_DBG("timer %p not active", timer); + return; + } + + if (timer->evp.sigev_notify == SIGEV_NONE) { + LOG_DBG("SIGEV_NONE"); + return; } - (timer->sev.sigev_notify_function)(timer->sev.sigev_value); + if (timer->evp.sigev_notify_function == NULL) { + LOG_DBG("NULL sigev_notify_function"); + return; + } + + LOG_DBG("calling sigev_notify_function %p", timer->evp.sigev_notify_function); + (timer->evp.sigev_notify_function)(timer->evp.sigev_value); } static void *zephyr_thread_wrapper(void *arg) { + int ret; struct timer_obj *timer = (struct timer_obj *)arg; + ret = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); + __ASSERT(ret == 0, "pthread_setcanceltype() failed: %d", ret); + + if (timer->evp.sigev_notify_attributes == NULL) { + ret = pthread_detach(pthread_self()); + __ASSERT(ret == 0, "pthread_detach() failed: %d", ret); + } + while (1) { if (timer->reload == 0U) { timer->status = NOT_ACTIVE; + LOG_DBG("timer %p not active", timer); } - k_sem_take(&timer->sem_cond, K_FOREVER); + ret = k_sem_take(&timer->sem_cond, K_FOREVER); + __ASSERT(ret == 0, "k_sem_take() failed: %d", ret); + + if (timer->evp.sigev_notify_function == NULL) { + LOG_DBG("NULL sigev_notify_function"); + continue; + } - (timer->sev.sigev_notify_function)(timer->sev.sigev_value); + LOG_DBG("calling sigev_notify_function %p", timer->evp.sigev_notify_function); + (timer->evp.sigev_notify_function)(timer->evp.sigev_value); } return NULL; @@ -77,52 +110,89 @@ static void zephyr_timer_interrupt(struct k_timer *ztimer) */ int timer_create(clockid_t clockid, struct sigevent *evp, timer_t *timerid) { + int ret = 0; + int detachstate; struct timer_obj *timer; const k_timeout_t alloc_timeout = K_MSEC(CONFIG_TIMER_CREATE_WAIT); - if (evp == NULL || (evp->sigev_notify != SIGEV_NONE && evp->sigev_notify != SIGEV_SIGNAL && - evp->sigev_notify != SIGEV_THREAD)) { + if (evp == NULL || timerid == NULL) { errno = EINVAL; return -1; } - if (k_mem_slab_alloc(&posix_timer_slab, (void **)&timer, alloc_timeout) == 0) { - (void)memset(timer, 0, sizeof(struct timer_obj)); - } else { + if (k_mem_slab_alloc(&posix_timer_slab, (void **)&timer, alloc_timeout) != 0) { + LOG_DBG("k_mem_slab_alloc() failed: %d", ret); errno = ENOMEM; return -1; } - timer->sev.sigev_notify_function = evp->sigev_notify_function; - timer->sev.sigev_value = evp->sigev_value; - timer->interval.tv_sec = 0; - timer->interval.tv_nsec = 0; - timer->reload = 0U; - timer->status = NOT_ACTIVE; + *timer = (struct timer_obj){0}; + timer->evp = *evp; + evp = &timer->evp; - if (evp->sigev_notify == SIGEV_NONE) { + switch (evp->sigev_notify) { + case SIGEV_NONE: k_timer_init(&timer->ztimer, NULL, NULL); - } else if (evp->sigev_notify == SIGEV_THREAD) { - int ret; + break; + case SIGEV_SIGNAL: + k_timer_init(&timer->ztimer, zephyr_timer_wrapper, NULL); + break; + case SIGEV_THREAD: + if (evp->sigev_notify_attributes != NULL) { + ret = pthread_attr_getdetachstate(evp->sigev_notify_attributes, + &detachstate); + if (ret != 0) { + LOG_DBG("pthread_attr_getdetachstate() failed: %d", ret); + errno = ret; + ret = -1; + goto free_timer; + } + + if (detachstate != PTHREAD_CREATE_DETACHED) { + ret = pthread_attr_setdetachstate(evp->sigev_notify_attributes, + PTHREAD_CREATE_DETACHED); + if (ret != 0) { + LOG_DBG("pthread_attr_setdetachstate() failed: %d", ret); + errno = ret; + ret = -1; + goto free_timer; + } + } + } + + ret = k_sem_init(&timer->sem_cond, 0, 1); + if (ret != 0) { + LOG_DBG("k_sem_init() failed: %d", ret); + errno = -ret; + ret = -1; + goto free_timer; + } - timer->sev.sigev_notify = SIGEV_THREAD; - k_sem_init(&timer->sem_cond, 0, 1); ret = pthread_create(&timer->thread, evp->sigev_notify_attributes, zephyr_thread_wrapper, timer); if (ret != 0) { - k_mem_slab_free(&posix_timer_slab, (void *) &timer); - return ret; + LOG_DBG("pthread_create() failed: %d", ret); + errno = ret; + ret = -1; + goto free_timer; } - pthread_detach(timer->thread); k_timer_init(&timer->ztimer, zephyr_timer_interrupt, NULL); - } else { - k_timer_init(&timer->ztimer, zephyr_timer_wrapper, NULL); + break; + default: + ret = -1; + errno = EINVAL; + goto free_timer; } *timerid = (timer_t)timer; + goto out; - return 0; +free_timer: + k_mem_slab_free(&posix_timer_slab, (void *)&timer); + +out: + return ret; } /** @@ -262,8 +332,8 @@ int timer_delete(timer_t timerid) k_timer_stop(&timer->ztimer); } - if (timer->sev.sigev_notify == SIGEV_THREAD) { - pthread_cancel(timer->thread); + if (timer->evp.sigev_notify == SIGEV_THREAD) { + (void)pthread_cancel(timer->thread); } k_mem_slab_free(&posix_timer_slab, (void *)timer); From 0e283c19dc1c7d61a3c536321aa8c71b6c417b5b Mon Sep 17 00:00:00 2001 From: Christopher Friedt Date: Sat, 20 Jan 2024 11:55:11 -0500 Subject: [PATCH 4/4] tests: posix: timer: run tests for realtime as well Ensure that the realtime clock may also be used with timer_create(). Signed-off-by: Christopher Friedt --- tests/posix/common/src/timer.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/tests/posix/common/src/timer.c b/tests/posix/common/src/timer.c index 9463bc30d781..ffb94e93618b 100644 --- a/tests/posix/common/src/timer.c +++ b/tests/posix/common/src/timer.c @@ -30,7 +30,7 @@ void handler(union sigval val) zassert_equal(val.sival_int, TEST_SIGNAL_VAL); } -void test_timer(int sigev_notify) +void test_timer(clockid_t clock_id, int sigev_notify) { struct sigevent sig = {0}; struct itimerspec value, ovalue; @@ -43,7 +43,7 @@ void test_timer(int sigev_notify) sig.sigev_value.sival_int = TEST_SIGNAL_VAL; /*TESTPOINT: Check if timer is created successfully*/ - zassert_ok(timer_create(CLOCK_MONOTONIC, &sig, &timerid)); + zassert_ok(timer_create(clock_id, &sig, &timerid)); value.it_value.tv_sec = DURATION_SECS; value.it_value.tv_nsec = DURATION_NSECS; @@ -59,9 +59,9 @@ void test_timer(int sigev_notify) LOG_DBG("Time remaining to fire %d secs and %d nsecs", (int)value.it_value.tv_sec, (int)value.it_value.tv_nsec); - clock_gettime(CLOCK_MONOTONIC, &ts); + clock_gettime(clock_id, &ts); sleep(SECS_TO_SLEEP); - clock_gettime(CLOCK_MONOTONIC, &te); + clock_gettime(clock_id, &te); if (te.tv_nsec >= ts.tv_nsec) { secs_elapsed = te.tv_sec - ts.tv_sec; @@ -82,14 +82,24 @@ void test_timer(int sigev_notify) exp_count, expected_signal_count); } -ZTEST(timer, test_SIGEV_SIGNAL) +ZTEST(timer, test_CLOCK_REALTIME__SIGEV_SIGNAL) +{ + test_timer(CLOCK_REALTIME, SIGEV_SIGNAL); +} + +ZTEST(timer, test_CLOCK_REALTIME__SIGEV_THREAD) +{ + test_timer(CLOCK_REALTIME, SIGEV_THREAD); +} + +ZTEST(timer, test_CLOCK_MONOTONIC__SIGEV_SIGNAL) { - test_timer(SIGEV_SIGNAL); + test_timer(CLOCK_MONOTONIC, SIGEV_SIGNAL); } -ZTEST(timer, test_SIGEV_THREAD) +ZTEST(timer, test_CLOCK_MONOTONIC__SIGEV_THREAD) { - test_timer(SIGEV_THREAD); + test_timer(CLOCK_MONOTONIC, SIGEV_THREAD); } ZTEST(timer, test_timer_overrun)