kernel: Improve the mutex system

kernel/CMakeLists.txt

@@ -20,6 +20,7 @@ set(SOURCES
     src/arch/Serial.cpp
     src/arch/Timer.cpp
     src/arch/PCI.cpp
+    src/lib/Mutex.cpp
     src/thread/Thread.cpp
     src/thread/ThreadImage.cpp
     src/thread/Scheduler.cpp

kernel/src/arch/x86_64/disk/ATA.cpp

@@ -1,10 +1,12 @@
 #include "arch/x86_64/disk/ATA.h"
 #include "Log.h"
+#include "arch/CPU.h"
 #include "arch/Serial.h"
 #include "arch/Timer.h"
 #include "arch/x86_64/IO.h"
 #include "fs/MBR.h"
 #include "memory/MemoryManager.h"
+#include "thread/Scheduler.h"
 #include <luna/Alignment.h>
 #include <luna/Buffer.h>
 #include <luna/CType.h>
@@ -296,7 +298,7 @@ namespace ATA
 
         for (u8 drive = 0; drive < 2; drive++)
         {
-            ScopedKMutexLock<100> lock(m_lock);
+            ScopedMutexLock lock(m_lock);
 
             select(drive);
 
@@ -757,7 +759,7 @@ ATADevice::ATADevice(ATA::Drive* drive) : BlockDevice(drive->block_size(), drive
 
 Result<void> ATADevice::read_block(Buffer& buf, u64 block) const
 {
-    ScopedKMutexLock<100> lock(m_drive->channel()->lock());
+    ScopedMutexLock lock(m_drive->channel()->lock());
 
     if (buf.size() != m_drive->block_size())
     {

kernel/src/arch/x86_64/disk/ATA.h

@@ -2,7 +2,7 @@
 #include "arch/PCI.h"
 #include "fs/devices/BlockDevice.h"
 #include "fs/devices/DeviceRegistry.h"
-#include "lib/KMutex.h"
+#include "lib/Mutex.h"
 #include <luna/Atomic.h>
 #include <luna/SharedPtr.h>
 #include <luna/StaticString.h>
@@ -244,7 +244,7 @@ namespace ATA
             return m_interrupt_line;
         }
 
-        KMutex<100>& lock()
+        Mutex& lock()
         {
             return m_lock;
         }
@@ -259,7 +259,7 @@ namespace ATA
         bool m_is_pci_native_mode;
         u8 m_interrupt_line;
 
-        KMutex<100> m_lock {};
+        Mutex m_lock {};
 
         Thread* m_thread { nullptr };
 

kernel/src/fs/StorageCache.h

@@ -1,5 +1,5 @@
 #pragma once
-#include "lib/KMutex.h"
+#include "lib/Mutex.h"
 #include <luna/Buffer.h>
 #include <luna/HashMap.h>
 #include <luna/LinkedList.h>
@@ -33,5 +33,5 @@ class StorageCache : public LinkedListNode<StorageCache>
 
   private:
     HashMap<u64, CacheEntry> m_cache_entries;
-    KMutex<100> m_mutex;
+    Mutex m_mutex;
 };

kernel/src/lib/KMutex.h

@@ -1,77 +0,0 @@
-#pragma once
-#include "Log.h"
-#include "arch/CPU.h"
-#include "thread/Scheduler.h"
-#include "thread/Thread.h"
-#include <luna/CircularQueue.h>
-
-template <usize ConcurrentThreads> class KMutex
-{
-  public:
-    void lock()
-    {
-        int expected = 0;
-        while (!m_lock.compare_exchange_strong(expected, 1))
-        {
-            expected = 0;
-            auto* current = Scheduler::current();
-
-            // We cannot be interrupted between these functions, otherwise we might never exit the loop
-            CPU::disable_interrupts();
-            bool ok = m_blocked_threads.try_push(current);
-            if (!ok) kernel_sleep(10);
-            else
-                kernel_wait_for_event();
-            CPU::enable_interrupts();
-        }
-    };
-
-    void unlock()
-    {
-        int expected = 1;
-        if (!m_lock.compare_exchange_strong(expected, 0))
-        {
-            kwarnln("KMutex::unlock() called on an unlocked lock with value %d", expected);
-        }
-
-        Thread* blocked;
-        if (m_blocked_threads.try_pop(blocked)) blocked->wake_up();
-    }
-
-    bool try_lock()
-    {
-        int expected = 0;
-        return m_lock.compare_exchange_strong(expected, 1);
-    }
-
-  private:
-    CircularQueue<Thread*, ConcurrentThreads> m_blocked_threads;
-    Atomic<int> m_lock;
-};
-
-template <usize ConcurrentThreads> class ScopedKMutexLock
-{
-  public:
-    ScopedKMutexLock(KMutex<ConcurrentThreads>& lock) : m_lock(lock)
-    {
-        m_lock.lock();
-    }
-
-    ~ScopedKMutexLock()
-    {
-        if (!m_taken_over) m_lock.unlock();
-    }
-
-    ScopedKMutexLock(const ScopedKMutexLock&) = delete;
-    ScopedKMutexLock(ScopedKMutexLock&&) = delete;
-
-    KMutex<ConcurrentThreads>& take_over()
-    {
-        m_taken_over = true;
-        return m_lock;
-    }
-
-  private:
-    KMutex<ConcurrentThreads>& m_lock;
-    bool m_taken_over { false };
-};

kernel/src/lib/Mutex.cpp

@@ -0,0 +1,93 @@
+#include "lib/Mutex.h"
+#include "Log.h"
+#include "arch/CPU.h"
+#include "thread/Scheduler.h"
+
+void Mutex::lock()
+{
+    auto* current = Scheduler::current();
+    const pid_t desired = current->id;
+    check(desired > 0); // Why the hell would the idle thread be touching a mutex?
+
+    while (true)
+    {
+        // Make sure only one thread is touching the mutex at the same time.
+        m_spinlock.lock();
+
+        pid_t expected = 0;
+        if (!m_thread.compare_exchange_strong(expected, desired))
+        {
+            if (expected == desired)
+            {
+                kerrorln("DEADLOCK! KMutex::lock() recursively called by the same thread (%d)", current->id);
+                fail("Mutex deadlock detected");
+            }
+
+            bool ok = m_blocked_threads.try_push(current);
+            m_spinlock.unlock();
+            if (!ok) kernel_sleep(10);
+            else
+                kernel_wait_for_event();
+        }
+        else
+        {
+            m_spinlock.unlock();
+            break;
+        }
+    }
+};
+
+void Mutex::unlock()
+{
+    auto* current = Scheduler::current();
+    pid_t expected = current->id;
+    check(expected > 0); // Why the hell would the idle thread be touching a mutex?
+
+    m_spinlock.lock();
+
+    if (!m_thread.compare_exchange_strong(expected, 0))
+    {
+        kerrorln("KMutex::unlock() called on a lock already locked by another thread (%d, current is %d)", expected,
+                 current->id);
+        fail("Mutex unlock by different thread");
+    }
+
+    Thread* blocked;
+    if (m_blocked_threads.try_pop(blocked))
+    {
+        while (blocked->state == ThreadState::Runnable)
+        {
+            // Rarely, we could switch to here between m_spinlock.unlock() and kernel_wait_for_event() above.
+            // Let the thread go to sleep before waking it up again.
+            kernel_yield();
+        }
+        blocked->wake_up();
+    }
+
+    m_spinlock.unlock();
+}
+
+bool Mutex::try_lock()
+{
+    auto* current = Scheduler::current();
+    const pid_t desired = current->id;
+    check(desired > 0); // Why the hell would the idle thread be touching a mutex?
+
+    // Make sure only one thread is touching the mutex at the same time.
+    m_spinlock.lock();
+
+    pid_t expected = 0;
+    bool ok = m_thread.compare_exchange_strong(expected, desired);
+
+    if (expected == desired)
+    {
+        kwarnln("Deadlock avoided! KMutex::try_lock() failed because it was already locked by the same thread "
+                "(%d), this is not supposed to happen",
+                current->id);
+        CPU::print_stack_trace();
+    }
+
+    m_spinlock.unlock();
+
+    return ok;
+}

kernel/src/lib/Mutex.h

@@ -0,0 +1,44 @@
+#pragma once
+#include "thread/Thread.h"
+#include <luna/CircularQueue.h>
+#include <luna/Spinlock.h>
+
+class Mutex
+{
+  public:
+    void lock();
+    void unlock();
+    bool try_lock();
+
+  private:
+    CircularQueue<Thread*, 32> m_blocked_threads;
+    Spinlock m_spinlock;
+    Atomic<pid_t> m_thread;
+};
+
+class ScopedMutexLock
+{
+  public:
+    ScopedMutexLock(Mutex& lock) : m_lock(lock)
+    {
+        m_lock.lock();
+    }
+
+    ~ScopedMutexLock()
+    {
+        if (!m_taken_over) m_lock.unlock();
+    }
+
+    ScopedMutexLock(const ScopedMutexLock&) = delete;
+    ScopedMutexLock(ScopedMutexLock&&) = delete;
+
+    Mutex& take_over()
+    {
+        m_taken_over = true;
+        return m_lock;
+    }
+
+  private:
+    Mutex& m_lock;
+    bool m_taken_over { false };
+};

kernel/src/memory/MemoryManager.cpp

@@ -4,6 +4,7 @@
 #include "fs/StorageCache.h"
 #include "memory/KernelVM.h"
 #include "memory/MemoryMap.h"
+#include "thread/Scheduler.h"
 #include <luna/Alignment.h>
 #include <luna/Bitmap.h>
 #include <luna/Common.h>