Changeset 9f2f5ee in mainline

Timestamp:

2025-04-10T17:48:56Z (4 days ago)

Author:

Jiří Zárevúcky <zarevucky.jiri@…>

Branches:

master

Children:

c55ab66

Parents:

c626117

git-author:

Jiří Zárevúcky <zarevucky.jiri@…> (2025-04-10 10:55:54)

git-committer:

Jiří Zárevúcky <zarevucky.jiri@…> (2025-04-10 17:48:56)

Message:

Rewrite kernel mutex implementation a little

Removes MUTEX_ACTIVE, the use of which has been removed in favor of
irq_spinlock_t, and fixes some issues with the old implementation.

• A race in mtx→owner access is unavoidable, so make it explicitly atomic.
• The THREAD==NULL case happens when there are no other threads yet, so we factor it out as a special case. Also ensures recursive mutex works before threads are initialized, just as normal mutex does.
• More and better asserts.

Location:

kernel/generic

Files:

• include/synch/mutex.h (modified) (2 diffs)
• src/synch/mutex.c (modified) (8 diffs)

kernel/generic/include/synch/mutex.h

@@ -44 +44 @@
         MUTEX_PASSIVE,
         MUTEX_RECURSIVE,
-        MUTEX_ACTIVE
 } mutex_type_t;
 
@@ -51 +50 @@
 typedef struct {
         mutex_type_t type;
+        int nesting;
         semaphore_t sem;
-        struct thread *owner;
-        unsigned nesting;
+        _Atomic(struct thread *) owner;
 } mutex_t;
 
 #define MUTEX_INITIALIZER(name, mtype) (mutex_t) { \
         .type = (mtype), \
+        .nesting = 0, \
         .sem = SEMAPHORE_INITIALIZER((name).sem, 1), \
         .owner = NULL, \
-        .nesting = 0, \
 }
 

kernel/generic/src/synch/mutex.c

@@ -1 +1 @@
 /*
  * Copyright (c) 2001-2004 Jakub Jermar
- * Copyright (c) 2023 Jiří Zárevúcky
+ * Copyright (c) 2025 Jiří Zárevúcky
  * All rights reserved.
  *
@@ -39 +39 @@
 #include <assert.h>
 #include <errno.h>
+#include <proc/thread.h>
+#include <stdatomic.h>
 #include <synch/mutex.h>
 #include <synch/semaphore.h>
-#include <arch.h>
-#include <stacktrace.h>
-#include <cpu.h>
-#include <proc/thread.h>
 
 /** Initialize mutex.
@@ -56 +54 @@
 }
 
+/** A race in mtx->owner access is unavoidable, so we have to make
+ * access to it formally atomic. These are convenience functions to
+ * read/write the variable without memory barriers, since we don't need
+ * them and C11 atomics default to the strongest possible memory ordering
+ * by default, which is utterly ridiculous.
+ */
+static inline thread_t *_get_owner(mutex_t *mtx)
+{
+        return atomic_load_explicit(&mtx->owner, memory_order_relaxed);
+}
+
+/** Counterpart to _get_owner(). */
+static inline void _set_owner(mutex_t *mtx, thread_t *owner)
+{
+        atomic_store_explicit(&mtx->owner, owner, memory_order_relaxed);
+}
+
 /** Find out whether the mutex is currently locked.
  *
@@ -64 +79 @@
 bool mutex_locked(mutex_t *mtx)
 {
-        errno_t rc = semaphore_trydown(&mtx->sem);
-        if (rc == EOK) {
-                semaphore_up(&mtx->sem);
-        }
-        return rc != EOK;
-}
+        if (!THREAD)
+                return mtx->nesting > 0;
 
-static void mutex_lock_active(mutex_t *mtx)
-{
-        assert((mtx->type == MUTEX_ACTIVE) || !THREAD);
-
-        const unsigned deadlock_treshold = 100000000;
-        unsigned int cnt = 0;
-        bool deadlock_reported = false;
-
-        while (semaphore_trydown(&mtx->sem) != EOK) {
-                if (cnt++ > deadlock_treshold) {
-                        printf("cpu%u: looping on active mutex %p\n", CPU->id, mtx);
-                        stack_trace();
-                        cnt = 0;
-                        deadlock_reported = true;
-                }
-        }
-
-        if (deadlock_reported)
-                printf("cpu%u: not deadlocked\n", CPU->id);
+        return _get_owner(mtx) == THREAD;
 }
 
@@ -98 +91 @@
 void mutex_lock(mutex_t *mtx)
 {
-        if (mtx->type == MUTEX_RECURSIVE && mtx->owner == THREAD) {
-                assert(THREAD);
+        if (!THREAD) {
+                assert(mtx->type == MUTEX_RECURSIVE || mtx->nesting == 0);
                 mtx->nesting++;
                 return;
         }
 
-        if (mtx->type == MUTEX_ACTIVE || !THREAD) {
-                mutex_lock_active(mtx);
+        if (_get_owner(mtx) == THREAD) {
+                /* This will also detect nested locks on a non-recursive mutex. */
+                assert(mtx->type == MUTEX_RECURSIVE);
+                assert(mtx->nesting > 0);
+                mtx->nesting++;
                 return;
         }
 
         semaphore_down(&mtx->sem);
-        mtx->owner = THREAD;
+
+        _set_owner(mtx, THREAD);
+        assert(mtx->nesting == 0);
         mtx->nesting = 1;
 }
@@ -123 +121 @@
 errno_t mutex_lock_timeout(mutex_t *mtx, uint32_t usec)
 {
-        if (usec != 0) {
-                assert(mtx->type != MUTEX_ACTIVE);
-                assert(THREAD);
+        if (!THREAD) {
+                assert(mtx->type == MUTEX_RECURSIVE || mtx->nesting == 0);
+                mtx->nesting++;
+                return EOK;
         }
 
-        if (mtx->type == MUTEX_RECURSIVE && mtx->owner == THREAD) {
-                assert(THREAD);
+        if (_get_owner(mtx) == THREAD) {
+                assert(mtx->type == MUTEX_RECURSIVE);
+                assert(mtx->nesting > 0);
                 mtx->nesting++;
                 return EOK;
@@ -135 +135 @@
 
         errno_t rc = semaphore_down_timeout(&mtx->sem, usec);
-        if (rc == EOK) {
-                mtx->owner = THREAD;
-                mtx->nesting = 1;
-        }
-        return rc;
+        if (rc != EOK)
+                return rc;
+
+        _set_owner(mtx, THREAD);
+        assert(mtx->nesting == 0);
+        mtx->nesting = 1;
+        return EOK;
 }
 
@@ -157 +159 @@
 void mutex_unlock(mutex_t *mtx)
 {
-        if (mtx->type == MUTEX_RECURSIVE) {
-                assert(mtx->owner == THREAD);
-                if (--mtx->nesting > 0)
-                        return;
-                mtx->owner = NULL;
+        if (--mtx->nesting > 0) {
+                assert(mtx->type == MUTEX_RECURSIVE);
+                return;
         }
+
+        assert(mtx->nesting == 0);
+
+        if (!THREAD)
+                return;
+
+        assert(_get_owner(mtx) == THREAD);
+        _set_owner(mtx, NULL);
+
         semaphore_up(&mtx->sem);
 }