sys/xtimer: fix xtimer_mutex_lock_timeout corner cases (#2)

sys/include/xtimer.h

@@ -448,17 +448,22 @@ static inline bool xtimer_less(xtimer_ticks32_t a, xtimer_ticks32_t b);
 static inline bool xtimer_less64(xtimer_ticks64_t a, xtimer_ticks64_t b);
 
 /**
- * @brief lock a mutex but with timeout
+ * @brief try to lock a mutex but with timeout
  *
- * @note this requires core_thread_flags to be enabled
+ * This will try to lock a mutex. If the mutex is not available immediately or
+ * until a certain amount of time (@p timeout) the method will return -1
+ * indicating that the locking was not possible within this time.
  *
- * @param[in]    mutex  mutex to lock
- * @param[in]    us     timeout in microseconds relative
+ * @note if the timeout is lower than XTIMER_BACKOFF and the mutex is not
+ * immediately available, this method blocks for the given amount of time
  *
- * @return       0, when returned after mutex was locked
- * @return       -1, when the timeout occcured
+ * @param[in]    mutex    mutex to lock
+ * @param[in]    timeout  timeout in microseconds
+ *
+ * @return       0, mutex was successfully locked
+ * @return       -1, mutex couldn't be locked within the time given
  */
-int xtimer_mutex_lock_timeout(mutex_t *mutex, uint64_t us);
+int xtimer_mutex_lock_timeout(mutex_t *mutex, uint64_t timeout);
 
 /**
  * @brief    Set timeout thread flag after @p timeout

sys/xtimer/xtimer.c

@@ -41,7 +41,11 @@
 typedef struct {
     mutex_t *mutex;
     thread_t *thread;
-    int timeout;
+    enum {
+       NO_TIMEOUT,
+       TIMEDOUT_MUTEX_LOCKED,
+       TIMEDOUT_OTHER,
+    } timeout;
 } mutex_thread_t;
 
 static void _callback_unlock_mutex(void* arg)
@@ -237,30 +241,52 @@ static void _mutex_timeout(void *arg)
 {
     mutex_thread_t *mt = (mutex_thread_t *)arg;
 
-    mt->timeout = 1;
-    list_node_t *node = list_remove(&mt->mutex->queue,
-                                    (list_node_t *)&mt->thread->rq_entry);
-    if ((node != NULL) && (mt->mutex->queue.next == NULL)) {
-        mt->mutex->queue.next = MUTEX_LOCKED;
+    if (mt->mutex->queue.next != MUTEX_LOCKED) {
+        list_node_t *node = list_remove(&mt->mutex->queue,
+                                        (list_node_t *)&mt->thread->rq_entry);
+        if ((node != NULL) && (mt->mutex->queue.next == NULL)) {
+            mt->mutex->queue.next = MUTEX_LOCKED;
+        }
+    }
+
+    /* In case the mutex is unlocked successfully but there are other processes
+     * with higher priority running the timeout can occur while the mutex is
+     * successfully owned. */
+    if (mt->thread->status == STATUS_MUTEX_BLOCKED) {
+        mt->timeout = TIMEDOUT_MUTEX_LOCKED;
+        sched_set_status(mt->thread, STATUS_PENDING);
+        sched_context_switch_request = 1;
+    }
+    else {
+        mt->timeout = TIMEDOUT_OTHER;
     }
-    sched_set_status(mt->thread, STATUS_PENDING);
-    thread_yield_higher();
 }
 
 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, NO_TIMEOUT };
+    int locked = mutex_trylock(mutex);
 
-    if (timeout != 0) {
-        t.callback = _mutex_timeout;
-        t.arg = (void *)((mutex_thread_t *)&mt);
-        xtimer_set64(&t, timeout);
+    if (locked) {
+        return 0;
     }
 
-    mutex_lock(mutex);
+    t.callback = _mutex_timeout;
+    t.arg = (void *)((mutex_thread_t *)&mt);
+    xtimer_set64(&t, timeout);
+
+    /* a timeout lower than XTIMER_BACKOFF causes the xtimer to spin rather
+     * than to set a timer for interrupt. Hence, we shall make the mutex_lock
+     * call blocking only when the interrupt didn't occur yet. */
+    locked = _mutex_lock(mutex, (mt.timeout == NO_TIMEOUT));
     xtimer_remove(&t);
-    return -mt.timeout;
+
+    if (mt.timeout == TIMEDOUT_MUTEX_LOCKED) {
+        return -1;
+    }
+
+    return (locked - 1);
 }
 
 #ifdef MODULE_CORE_THREAD_FLAGS

tests/xtimer_mutex/Makefile

@@ -0,0 +1,9 @@
+APPLICATION = xtimer_mutex
+include ../Makefile.tests_common
+
+USEMODULE += xtimer
+USEMODULE += fmt
+
+BOARD_INSUFFICIENT_MEMORY := nucleo-f030 nucleo-f031 nucleo32-f031
+
+include $(RIOTBASE)/Makefile.include

tests/xtimer_mutex/main.c

@@ -0,0 +1,209 @@
+/*
+ * Copyright (C) 2017 TriaGnoSys GmbH
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup tests
+ * @{
+ *
+ * @file
+ * @brief xtimer_mutex test application
+ *
+ * @author      Víctor Ariño <[email protected]>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+#include <time.h>
+
+#include "xtimer.h"
+#include "thread.h"
+#include "msg.h"
+#include "fmt.h"
+
+char thread1_stack[THREAD_STACKSIZE_MAIN];
+char thread2_stack[THREAD_STACKSIZE_MAIN];
+
+static mutex_t mutex = MUTEX_INIT;
+
+static kernel_pid_t mainpid, pid, pid2;
+
+static int tx_lock(int tx, uint64_t timeout)
+{
+    printf("thread%d timeout: ", tx);
+    print_u64_dec(timeout);
+    printf(", mutex: ");
+    if (mutex.queue.next == NULL) {
+        printf("unlocked");
+    }
+    else if (mutex.queue.next == MUTEX_LOCKED) {
+        printf("locked (1)");
+    }
+    else {
+        printf("locked (2+)");
+    }
+    printf(" --> ");
+    return xtimer_mutex_lock_timeout(&mutex, timeout);
+}
+
+static void tx_lock_eq(int tx, uint64_t timeout)
+{
+    if (tx_lock(tx, timeout) == 0) {
+        printf("OK\n");
+    }
+    else {
+        printf("FAIL\n");
+    }
+}
+
+static void tx_lock_lt(int tx, uint64_t timeout)
+{
+    if (tx_lock(tx, timeout) < 0) {
+        printf("OK\n");
+    }
+    else {
+        printf("FAIL\n");
+    }
+}
+
+static void test_timeouts(int tx)
+{
+    uint64_t timeout = 0;
+    tx_lock_lt(tx, timeout);
+
+    timeout = XTIMER_BACKOFF - 1;
+    tx_lock_lt(tx, timeout);
+
+    timeout = XTIMER_BACKOFF;
+    tx_lock_lt(tx, timeout);
+
+    timeout = XTIMER_BACKOFF + 1;
+    tx_lock_lt(tx, timeout);
+
+    timeout = (2 * XTIMER_BACKOFF) - 1;
+    tx_lock_lt(tx, timeout);
+
+    timeout = (2 * XTIMER_BACKOFF);
+    tx_lock_lt(tx, timeout);
+
+    timeout = 1000000;
+    tx_lock_lt(tx, timeout);
+}
+
+void *thread2(void *arg)
+{
+    (void) arg;
+    msg_t m;
+
+    printf("thread2: running\n");
+    test_timeouts(2);
+
+    /* tell thread 1 to unlock the mutex */
+    msg_send(&m, pid);
+    printf("thread2: waiting for mutex\n");
+    if (xtimer_mutex_lock_timeout(&mutex, 2000000) == 0) {
+        printf("thread2: got mutex! --> OK \n");
+    }
+    else {
+        printf("thread2: didn't get the mutex --> FAIL\n");
+    }
+
+    printf("thread2: done\n");
+
+    return 0;
+}
+
+void *thread1(void *arg)
+{
+    (void) arg;
+    msg_t m, r;
+
+    msg_receive(&m);
+    tx_lock_eq(1, 0);
+    tx_lock_lt(1, 0);
+
+    mutex_unlock(&mutex);
+    msg_reply(&m, &r);
+
+    /* let main thread lock the mutex */
+    msg_receive(&m);
+    test_timeouts(1);
+
+    /* mutex will be unlocked while waiting */
+    msg_reply(&m, &r);
+    printf("thread1: waiting for mutex\n");
+    if (xtimer_mutex_lock_timeout(&mutex, 2000000) == 0) {
+        printf("thread1: got mutex! --> OK \n");
+    }
+    else {
+        printf("thread1: didn't get the mutex --> FAIL\n");
+    }
+
+    msg_receive(&m);
+
+    /* got msg from thread 1, we have to unlock the mutex after a delay */
+    xtimer_usleep(500000);
+    printf("thread1: unlocking\n");
+    mutex_unlock(&mutex);
+
+    printf("thread1: done\n");
+
+    return 0;
+}
+
+int main(void)
+{
+    msg_t m, r;
+
+    mainpid = thread_getpid();
+    pid = thread_create(
+        thread1_stack,
+        sizeof(thread1_stack),
+        THREAD_PRIORITY_MAIN + 1,
+        THREAD_CREATE_STACKTEST,
+        thread1,
+        NULL,
+        "thread 1");
+
+    printf("main: tests with unlocked mutex:\n");
+    msg_send_receive(&m, &r, pid);
+
+    printf("main: locking mutex\n");
+    mutex_lock(&mutex);
+
+    printf("main: tests with mutex locked once:\n");
+    msg_send_receive(&m, &r, pid);
+
+    printf("main: test unlocking while waiting\n");
+    xtimer_usleep(500000);
+
+    printf("main: unlocking...\n");
+    mutex_unlock(&mutex);
+    xtimer_usleep(500000);
+
+    /* tests with 2+ nodes locking the mutex */
+    printf("main: starting thread2\n");
+    pid2 = thread_create(
+        thread2_stack,
+        sizeof(thread2_stack),
+        THREAD_PRIORITY_MAIN + 2,
+        THREAD_CREATE_STACKTEST,
+        thread2,
+        NULL,
+        "thread 2");
+
+    printf("main: locking mutex\n");
+    mutex_lock(&mutex);
+
+    printf("main: got mutex, unlocking\n");
+    mutex_unlock(&mutex);
+
+    printf("main: done\n");
+
+    return 0;
+}

tests/xtimer_mutex/tests/01-run.py

@@ -0,0 +1,21 @@
+#!/usr/bin/env python3
+
+# Copyright (C) 2017 TriaGnoSys GmbH
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+
+import os
+import sys
+
+sys.path.append(os.path.join(os.environ['RIOTBASE'], 'dist/tools/testrunner'))
+import testrunner
+
+def testfunc(child):
+    child.expect(u"main: done")
+    child.expect(u"thread1: done")
+    child.expect(u"thread2: done")
+
+if __name__ == "__main__":
+    sys.exit(testrunner.run(testfunc))