From dfeaea3c3653b518633f125cb02cd9f826336793 Mon Sep 17 00:00:00 2001 From: JulianHolzwarth Date: Fri, 24 Jan 2020 15:59:12 +0100 Subject: [PATCH 1/4] sys/xtimer/xtimer.c: comment change --- sys/xtimer/xtimer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/xtimer/xtimer.c b/sys/xtimer/xtimer.c index d51e03f5b3..ea09c6f5de 100644 --- a/sys/xtimer/xtimer.c +++ b/sys/xtimer/xtimer.c @@ -202,7 +202,7 @@ void xtimer_now_timex(timex_t *out) static void _mutex_timeout(void *arg) { - /* interrupts a disabled because xtimer can spin + /* interrupts are disabled because xtimer can spin * if xtimer_set spins the callback is executed * in the thread context * From 110c725321ff6aeab08fb546768c874ba2f987f9 Mon Sep 17 00:00:00 2001 From: JulianHolzwarth Date: Wed, 22 May 2019 15:20:08 +0200 Subject: [PATCH 2/4] tests/xtimer_mutex_lock_timeout: minimal xtimer_mutex_lock_timeout test This test checks if the function works when the timeout is smaller than XTIMER_BACKOFF and the mutex is already locked. This means the timer will spin and the timer will shoot before the mutex lock was called. Then the mutex lock gets called and the timer will not remove the thread from the mutex. Checking if this case is handled correctly. --- tests/xtimer_mutex_lock_timeout/main.c | 92 +++++++++++++++++++ .../xtimer_mutex_lock_timeout/tests/01-run.py | 8 ++ 2 files changed, 100 insertions(+) diff --git a/tests/xtimer_mutex_lock_timeout/main.c b/tests/xtimer_mutex_lock_timeout/main.c index c96f32ccbd..f06cf8e25c 100644 --- a/tests/xtimer_mutex_lock_timeout/main.c +++ b/tests/xtimer_mutex_lock_timeout/main.c @@ -28,6 +28,9 @@ /* timeout at one millisecond (1000 us) to make sure it does not spin. */ #define LONG_MUTEX_TIMEOUT 1000 +/* timeout smaller than XTIMER_BACKOFF to make sure it spins. */ +#define SHORT_MUTEX_TIMEOUT ((1 << XTIMER_SHIFT) + 1) + /* main Thread PID */ static kernel_pid_t main_thread_pid; @@ -40,6 +43,11 @@ static int cmd_test_xtimer_mutex_lock_timeout_long_locked(int argc, char **argv); static int cmd_test_xtimer_mutex_lock_timeout_low_prio_thread(int argc, char **argv); +static int cmd_test_xtimer_mutex_lock_timeout_short_unlocked(int argc, + char **argv); +static int cmd_test_xtimer_mutex_lock_timeout_short_locked(int argc, + char **argv); + /** * @brief List of command for this application. @@ -52,6 +60,10 @@ static const shell_command_t shell_commands[] = { { "mutex_timeout_long_locked_low", "lock low-prio-locked-mutex from high-prio-thread (no-spin timeout)", cmd_test_xtimer_mutex_lock_timeout_low_prio_thread, }, + { "mutex_timeout_short_unlocked", "unlocked mutex (spin timeout)", + cmd_test_xtimer_mutex_lock_timeout_short_unlocked, }, + { "mutex_timeout_short_locked", "locked mutex (spin timeout)", + cmd_test_xtimer_mutex_lock_timeout_short_locked, }, { NULL, NULL, NULL } }; @@ -247,6 +259,86 @@ static int cmd_test_xtimer_mutex_lock_timeout_low_prio_thread(int argc, return 0; } +/** + * @brief shell command to test xtimer_mutex_lock_timeout when spinning + * + * The mutex is locked before the function call and + * the timer short. Meaning the timer will trigger before + * xtimer_mutex_lock_timeout tries to acquire the mutex. + * + * @param[in] argc Number of arguments + * @param[in] argv Array of arguments + * + * @return 0 on success + */ +static int cmd_test_xtimer_mutex_lock_timeout_short_locked(int argc, + char **argv) +{ + (void)argc; + (void)argv; + puts( + "starting test: xtimer mutex lock timeout with short timeout and locked mutex"); + mutex_t test_mutex = MUTEX_INIT; + mutex_lock(&test_mutex); + + if (xtimer_mutex_lock_timeout(&test_mutex, SHORT_MUTEX_TIMEOUT) == 0) { + puts("Error: mutex taken"); + } + else { + /* mutex has to be locked */ + if (mutex_trylock(&test_mutex) == 0) { + puts("OK"); + } + else { + puts("error mutex not locked"); + } + } + /* to make the test easier to read */ + printf("\n"); + + return 0; +} + +/** + * @brief shell command to test xtimer_mutex_lock_timeout when spinning + * + * the mutex is not locked before the function is called and + * the timer is short. Meaning the timer will trigger before + * xtimer_mutex_lock_timeout tries to acquire the mutex. + * + * @param[in] argc Number of arguments + * @param[in] argv Array of arguments + * + * @return 0 on success + */ +static int cmd_test_xtimer_mutex_lock_timeout_short_unlocked(int argc, + char **argv) +{ + (void)argc; + (void)argv; + puts( + "starting test: xtimer mutex lock timeout with short timeout and unlocked mutex"); + mutex_t test_mutex = MUTEX_INIT; + + if (xtimer_mutex_lock_timeout(&test_mutex, SHORT_MUTEX_TIMEOUT) == 0) { + /* mutex has to be locked */ + if (mutex_trylock(&test_mutex) == 0) { + puts("OK"); + } + else { + puts("error mutex not locked"); + } + } + else { + puts("Error: mutex timed out"); + } + /* to make the test easier to read */ + printf("\n"); + + return 0; +} + + /** * @brief main function starting shell * diff --git a/tests/xtimer_mutex_lock_timeout/tests/01-run.py b/tests/xtimer_mutex_lock_timeout/tests/01-run.py index e121d477d0..14170f10dd 100755 --- a/tests/xtimer_mutex_lock_timeout/tests/01-run.py +++ b/tests/xtimer_mutex_lock_timeout/tests/01-run.py @@ -38,6 +38,14 @@ def testfunc(child): child.expect("THREAD low prio: exiting low") child.expect("threads = 2") child.expect_exact("> ") + child.sendline("mutex_timeout_short_locked") + child.expect("starting test: xtimer mutex lock timeout with short timeout and locked mutex") + child.expect("OK") + child.expect_exact("> ") + child.sendline("mutex_timeout_short_unlocked") + child.expect("starting test: xtimer mutex lock timeout with short timeout and unlocked mutex") + child.expect("OK") + child.expect_exact("> ") if __name__ == "__main__": From bce45fd9d81a7c62d4e291a6f46abd5d4eedc4d1 Mon Sep 17 00:00:00 2001 From: JulianHolzwarth Date: Fri, 31 May 2019 15:39:34 +0200 Subject: [PATCH 3/4] xtimer/xtimer.c: xtimer_mutex_lock_timeout fix test Handling timeout smaller than XTIMER_BACKOFF (the timer spins) when the mutex is already locked. This fixes the test tests/xtimer_mutex_lock_timeout/main.c:mutex_timeout_spin_locked. --- sys/xtimer/xtimer.c | 62 ++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/sys/xtimer/xtimer.c b/sys/xtimer/xtimer.c index ea09c6f5de..d5c1c63465 100644 --- a/sys/xtimer/xtimer.c +++ b/sys/xtimer/xtimer.c @@ -45,7 +45,15 @@ typedef struct { mutex_t *mutex; thread_t *thread; - volatile int timeout; + /* + * @brief: one means the thread was removed from the mutex thread waiting list + * by _mutex_timeout() + */ + volatile uint8_t dequeued; + /* + * @brief: mutex_lock() should block because _mutex_timeout() did not shoot + */ + volatile uint8_t blocking; } mutex_thread_t; static void _callback_unlock_mutex(void* arg) @@ -200,6 +208,28 @@ void xtimer_now_timex(timex_t *out) out->microseconds = now - (out->seconds * US_PER_SEC); } +static int _mutex_remove_thread_from_waiting_queue(mutex_t *mutex, thread_t *thread) +{ + unsigned irqstate = irq_disable(); + assert(mutex != NULL && thread != NULL); + + if (mutex->queue.next != MUTEX_LOCKED && mutex->queue.next != NULL) { + list_node_t *node = list_remove(&mutex->queue, (list_node_t *)&thread->rq_entry); + /* if thread was removed from the list */ + if (node != NULL) { + if (mutex->queue.next == NULL) { + mutex->queue.next = MUTEX_LOCKED; + } + sched_set_status(thread, STATUS_PENDING); + irq_restore(irqstate); + sched_switch(thread->priority); + return 1; + } + } + irq_restore(irqstate); + return 0; +} + static void _mutex_timeout(void *arg) { /* interrupts are disabled because xtimer can spin @@ -212,41 +242,27 @@ static void _mutex_timeout(void *arg) unsigned int irqstate = irq_disable(); mutex_thread_t *mt = (mutex_thread_t *)arg; - - if (mt->mutex->queue.next != MUTEX_LOCKED && - mt->mutex->queue.next != NULL) { - mt->timeout = 1; - list_node_t *node = list_remove(&mt->mutex->queue, - (list_node_t *)&mt->thread->rq_entry); - - /* if thread was removed from the list */ - if (node != NULL) { - if (mt->mutex->queue.next == NULL) { - mt->mutex->queue.next = MUTEX_LOCKED; - } - sched_set_status(mt->thread, STATUS_PENDING); - irq_restore(irqstate); - sched_switch(mt->thread->priority); - return; - } - } + mt->blocking = 0; + mt->got_unlocked = _mutex_remove_thread_from_waiting_queue(mt->mutex, mt->thread); irq_restore(irqstate); } int xtimer_mutex_lock_timeout(mutex_t *mutex, uint64_t timeout) { xtimer_t t; - mutex_thread_t mt = { mutex, (thread_t *)sched_active_thread, 0 }; + mutex_thread_t mt = { mutex, (thread_t *)sched_active_thread, .dequeued=0, .blocking=1 }; if (timeout != 0) { t.callback = _mutex_timeout; t.arg = (void *)((mutex_thread_t *)&mt); xtimer_set64(&t, timeout); } - - mutex_lock(mutex); + int ret = _mutex_lock(mutex, mt.blocking); + if (ret == 0) { + return -1; + } xtimer_remove(&t); - return -mt.timeout; + return -mt.dequeued; } #ifdef MODULE_CORE_THREAD_FLAGS From 4d85fa16e0d07e5ecb14e95d306f77469002881c Mon Sep 17 00:00:00 2001 From: JulianHolzwarth Date: Fri, 5 Jul 2019 17:11:13 +0200 Subject: [PATCH 4/4] xtimer/xtimer.c: _mutex_remove_thread_from_waiting_queue This function tries to remove the thread from a mutex waiting queue. The value pointed to by unlocked will be set to 1 if the thread was removed from the waiting queue otherwise 0. --- sys/xtimer/xtimer.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/sys/xtimer/xtimer.c b/sys/xtimer/xtimer.c index d5c1c63465..8576228084 100644 --- a/sys/xtimer/xtimer.c +++ b/sys/xtimer/xtimer.c @@ -208,7 +208,10 @@ void xtimer_now_timex(timex_t *out) out->microseconds = now - (out->seconds * US_PER_SEC); } -static int _mutex_remove_thread_from_waiting_queue(mutex_t *mutex, thread_t *thread) +/* + * The value pointed to by unlocked will be set to 1 if the thread was removed from the waiting queue otherwise 0. + */ +static void _mutex_remove_thread_from_waiting_queue(mutex_t *mutex, thread_t *thread, volatile uint8_t *unlocked) { unsigned irqstate = irq_disable(); assert(mutex != NULL && thread != NULL); @@ -220,14 +223,16 @@ static int _mutex_remove_thread_from_waiting_queue(mutex_t *mutex, thread_t *thr if (mutex->queue.next == NULL) { mutex->queue.next = MUTEX_LOCKED; } + *unlocked = 1; + sched_set_status(thread, STATUS_PENDING); irq_restore(irqstate); sched_switch(thread->priority); - return 1; + return; } } + *unlocked = 0; irq_restore(irqstate); - return 0; } static void _mutex_timeout(void *arg) @@ -243,7 +248,7 @@ static void _mutex_timeout(void *arg) mutex_thread_t *mt = (mutex_thread_t *)arg; mt->blocking = 0; - mt->got_unlocked = _mutex_remove_thread_from_waiting_queue(mt->mutex, mt->thread); + _mutex_remove_thread_from_waiting_queue(mt->mutex, mt->thread, &mt->dequeued); irq_restore(irqstate); }