From 649486f5cd0419fba3878ba853ccfe1851d7afcd Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Tue, 4 Aug 2020 12:52:32 +0200 Subject: [PATCH 1/3] sys/event: Remove incorrect comment The value of `queue->waiter` at the time the event was queued (with IRQs disabled) was backed up to the stack-variable `waiter`. Thus, the test later on for `waiter` checks if the queue was already claimed at the time the event was queued. Therefore, there is no race. --- sys/event/event.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/sys/event/event.c b/sys/event/event.c index 2091cf9199..325c9e7ccf 100644 --- a/sys/event/event.c +++ b/sys/event/event.c @@ -63,11 +63,6 @@ void event_post(event_queue_t *queue, event_t *event) thread_t *waiter = queue->waiter; irq_restore(state); - /* WARNING: there is a minimal chance, that a waiter claims a formerly - * detached queue between the end of the critical section above and - * the block below. In that case, the new waiter will not be woken - * up. This should be fixed at some point once it is safe to call - * thread_flags_set() inside a critical section on all platforms. */ if (waiter) { thread_flags_set(waiter, THREAD_FLAG_EVENT); } From 2c03dfca13af6c30d5066fc239c061bff75064e2 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Tue, 4 Aug 2020 12:58:39 +0200 Subject: [PATCH 2/3] sys/event: Made trivial functions static inline When the expected ROM overhead of a function is bigger than the actual function, it is better to provide the function as static inline function in the header. --- sys/event/event.c | 28 ---------------------------- sys/include/event.h | 32 +++++++++++++++++++++++++++----- 2 files changed, 27 insertions(+), 33 deletions(-) diff --git a/sys/event/event.c b/sys/event/event.c index 325c9e7ccf..2337b76859 100644 --- a/sys/event/event.c +++ b/sys/event/event.c @@ -33,25 +33,6 @@ #include "xtimer.h" #endif -void event_queue_init_detached(event_queue_t *queue) -{ - assert(queue); - memset(queue, '\0', sizeof(*queue)); -} - -void event_queue_init(event_queue_t *queue) -{ - assert(queue); - memset(queue, '\0', sizeof(*queue)); - queue->waiter = (thread_t *)sched_active_thread; -} - -void event_queue_claim(event_queue_t *queue) -{ - assert(queue && (queue->waiter == NULL)); - queue->waiter = (thread_t *)sched_active_thread; -} - void event_post(event_queue_t *queue, event_t *event) { assert(queue && event); @@ -152,12 +133,3 @@ event_t *event_wait_timeout64(event_queue_t *queue, uint64_t timeout) return _wait_timeout(queue, &timer); } #endif - -void event_loop_multi(event_queue_t *queues, size_t n_queues) -{ - event_t *event; - - while ((event = event_wait_multi(queues, n_queues))) { - event->handler(event); - } -} diff --git a/sys/include/event.h b/sys/include/event.h index 09ef7ea291..25d87ac846 100644 --- a/sys/include/event.h +++ b/sys/include/event.h @@ -97,10 +97,12 @@ #define EVENT_H #include +#include +#include "assert.h" +#include "clist.h" #include "irq.h" #include "thread_flags.h" -#include "clist.h" #ifdef __cplusplus extern "C" { @@ -156,14 +158,23 @@ typedef struct { * * @param[out] queue event queue object to initialize */ -void event_queue_init(event_queue_t *queue); +static inline void event_queue_init(event_queue_t *queue) +{ + assert(queue); + memset(queue, '\0', sizeof(*queue)); + queue->waiter = (thread_t *)sched_active_thread; +} /** * @brief Initialize an event queue not binding it to a thread * * @param[out] queue event queue object to initialize */ -void event_queue_init_detached(event_queue_t *queue); +static inline void event_queue_init_detached(event_queue_t *queue) +{ + assert(queue); + memset(queue, '\0', sizeof(*queue)); +} /** * @brief Bind an event queue to the calling thread @@ -175,7 +186,11 @@ void event_queue_init_detached(event_queue_t *queue); * * @param[out] queue event queue object to bind to a thread */ -void event_queue_claim(event_queue_t *queue); +static inline void event_queue_claim(event_queue_t *queue) +{ + assert(queue && (queue->waiter == NULL)); + queue->waiter = (thread_t *)sched_active_thread; +} /** * @brief Queue an event @@ -308,7 +323,14 @@ event_t *event_wait_timeout64(event_queue_t *queue, uint64_t timeout); * @param[in] queues Event queues to process * @param[in] n_queues Number of queues passed with @p queues */ -void event_loop_multi(event_queue_t *queues, size_t n_queues); +static inline void event_loop_multi(event_queue_t *queues, size_t n_queues) +{ + event_t *event; + + while ((event = event_wait_multi(queues, n_queues))) { + event->handler(event); + } +} /** * @brief Simple event loop From a5c4692806cf878d599073c3a7b5fe7baf03b4ad Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Wed, 5 Aug 2020 11:59:40 +0200 Subject: [PATCH 3/3] sys/event: Add multi-queue initializers Add a set of helper functions to initialize / claim an array of queues and use this in `tests/events`. --- sys/include/event.h | 66 ++++++++++++++++++++++++++++++++++++++++----- tests/events/main.c | 5 ++-- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/sys/include/event.h b/sys/include/event.h index 25d87ac846..21c9ba4f83 100644 --- a/sys/include/event.h +++ b/sys/include/event.h @@ -151,6 +151,26 @@ typedef struct { thread_t *waiter; /**< thread ownning event queue */ } event_queue_t; + +/** + * @brief Initialize an array of event queues + * + * This will set the calling thread as owner of each queue in @p queues. + * + * @param[out] queues event queue objects to initialize + * @param[in] n_queues number of queues in @p queues + */ +static inline void event_queues_init(event_queue_t *queues, + size_t n_queues) +{ + assert(queues && n_queues); + thread_t *me = (thread_t *)sched_active_thread; + for (size_t i = 0; i < n_queues; i++) { + memset(&queues[i], '\0', sizeof(queues[0])); + queues[i].waiter = me; + } +} + /** * @brief Initialize an event queue * @@ -160,9 +180,22 @@ typedef struct { */ static inline void event_queue_init(event_queue_t *queue) { - assert(queue); - memset(queue, '\0', sizeof(*queue)); - queue->waiter = (thread_t *)sched_active_thread; + event_queues_init(queue, 1); +} + +/** + * @brief Initialize an array of event queues not binding it to a thread + * + * @param[out] queues event queue objects to initialize + * @param[in] n_queues number of queues in @p queues + */ +static inline void event_queues_init_detached(event_queue_t *queues, + size_t n_queues) +{ + assert(queues); + for (size_t i = 0; i < n_queues; i++) { + memset(&queues[i], '\0', sizeof(queues[0])); + } } /** @@ -172,8 +205,28 @@ static inline void event_queue_init(event_queue_t *queue) */ static inline void event_queue_init_detached(event_queue_t *queue) { - assert(queue); - memset(queue, '\0', sizeof(*queue)); + event_queues_init_detached(queue, 1); +} + +/** + * @brief Bind an array of event queues to the calling thread + * + * This function must only be called once and only if the given queue is not + * yet bound to a thread. + * + * @pre (queues[i].waiter == NULL for i in {0, ..., n_queues - 1}) + * + * @param[out] queues event queue objects to bind to a thread + * @param[in] n_queues number of queues in @p queues + */ +static inline void event_queues_claim(event_queue_t *queues, size_t n_queues) +{ + assert(queues); + thread_t *me = (thread_t *)sched_active_thread; + for (size_t i = 0; i < n_queues; i++) { + assert(queues[i].waiter == NULL); + queues[i].waiter = me; + } } /** @@ -188,8 +241,7 @@ static inline void event_queue_init_detached(event_queue_t *queue) */ static inline void event_queue_claim(event_queue_t *queue) { - assert(queue && (queue->waiter == NULL)); - queue->waiter = (thread_t *)sched_active_thread; + event_queues_claim(queue, 1); } /** diff --git a/tests/events/main.c b/tests/events/main.c index 2b1a85c382..44d4575fde 100644 --- a/tests/events/main.c +++ b/tests/events/main.c @@ -128,9 +128,8 @@ static void *claiming_thread(void *arg) event_queue_t *dqs = arg; printf("claiming event queues %p\n", (void *)dqs); - for (size_t i = 0; i < DELAYED_QUEUES_NUMOF; i++) { - event_queue_claim(&dqs[i]); - } + event_queues_claim(dqs, DELAYED_QUEUES_NUMOF); + printf("launching event queue for queues %p\n", (void *)dqs); event_loop_multi(dqs, DELAYED_QUEUES_NUMOF);