Merge pull request #13199 from JulianHolzwarth/pr/xtimer_mutex_lock_timeout/short_time_fix

xtimer/xtimer.c: xtimer_mutex_lock_timeout fix with short timeout

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,9 +208,36 @@ void xtimer_now_timex(timex_t *out)
     out->microseconds = now - (out->seconds * US_PER_SEC);
 }
 
+/*
+ * 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);
+
+    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;
+            }
+            *unlocked = 1;
+
+            sched_set_status(thread, STATUS_PENDING);
+            irq_restore(irqstate);
+            sched_switch(thread->priority);
+            return;
+        }
+    }
+    *unlocked = 0;
+    irq_restore(irqstate);
+}
+
 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
      *
@@ -212,41 +247,27 @@ static void _mutex_timeout(void *arg)
     unsigned int irqstate = irq_disable();
 
     mutex_thread_t *mt = (mutex_thread_t *)arg;
-
+    mt->blocking = 0;
-    if (mt->mutex->queue.next != MUTEX_LOCKED &&
+    _mutex_remove_thread_from_waiting_queue(mt->mutex, mt->thread, &mt->dequeued);
-        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;
-        }
-    }
     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);
     }
-
+    int ret = _mutex_lock(mutex, mt.blocking);
-    mutex_lock(mutex);
+    if (ret == 0) {
+        return -1;
+    }
     xtimer_remove(&t);
-    return -mt.timeout;
+    return -mt.dequeued;
 }
 
 #ifdef MODULE_CORE_THREAD_FLAGS

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
  *

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__":