From 4af337e92d6146b38704c8f6a3bac17181e6d4a5 Mon Sep 17 00:00:00 2001 From: apio Date: Wed, 20 Sep 2023 07:05:33 +0200 Subject: [PATCH] kernel: Improve the mutex system --- kernel/CMakeLists.txt | 1 + kernel/src/arch/x86_64/disk/ATA.cpp | 6 +- kernel/src/arch/x86_64/disk/ATA.h | 6 +- kernel/src/fs/StorageCache.h | 4 +- kernel/src/lib/KMutex.h | 77 ------------------------ kernel/src/lib/Mutex.cpp | 93 +++++++++++++++++++++++++++++ kernel/src/lib/Mutex.h | 44 ++++++++++++++ kernel/src/memory/MemoryManager.cpp | 1 + 8 files changed, 148 insertions(+), 84 deletions(-) delete mode 100644 kernel/src/lib/KMutex.h create mode 100644 kernel/src/lib/Mutex.cpp create mode 100644 kernel/src/lib/Mutex.h diff --git a/kernel/CMakeLists.txt b/kernel/CMakeLists.txt index e602a1c5..438a512e 100644 --- a/kernel/CMakeLists.txt +++ b/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 diff --git a/kernel/src/arch/x86_64/disk/ATA.cpp b/kernel/src/arch/x86_64/disk/ATA.cpp index 17ffc484..a38d1fbe 100644 --- a/kernel/src/arch/x86_64/disk/ATA.cpp +++ b/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 #include #include @@ -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 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()) { diff --git a/kernel/src/arch/x86_64/disk/ATA.h b/kernel/src/arch/x86_64/disk/ATA.h index 9d5e19d7..c344c9ea 100644 --- a/kernel/src/arch/x86_64/disk/ATA.h +++ b/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 #include #include @@ -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 }; diff --git a/kernel/src/fs/StorageCache.h b/kernel/src/fs/StorageCache.h index 9767246d..d46df29e 100644 --- a/kernel/src/fs/StorageCache.h +++ b/kernel/src/fs/StorageCache.h @@ -1,5 +1,5 @@ #pragma once -#include "lib/KMutex.h" +#include "lib/Mutex.h" #include #include #include @@ -33,5 +33,5 @@ class StorageCache : public LinkedListNode private: HashMap m_cache_entries; - KMutex<100> m_mutex; + Mutex m_mutex; }; diff --git a/kernel/src/lib/KMutex.h b/kernel/src/lib/KMutex.h deleted file mode 100644 index 8929de06..00000000 --- a/kernel/src/lib/KMutex.h +++ /dev/null @@ -1,77 +0,0 @@ -#pragma once -#include "Log.h" -#include "arch/CPU.h" -#include "thread/Scheduler.h" -#include "thread/Thread.h" -#include - -template 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 m_blocked_threads; - Atomic m_lock; -}; - -template class ScopedKMutexLock -{ - public: - ScopedKMutexLock(KMutex& lock) : m_lock(lock) - { - m_lock.lock(); - } - - ~ScopedKMutexLock() - { - if (!m_taken_over) m_lock.unlock(); - } - - ScopedKMutexLock(const ScopedKMutexLock&) = delete; - ScopedKMutexLock(ScopedKMutexLock&&) = delete; - - KMutex& take_over() - { - m_taken_over = true; - return m_lock; - } - - private: - KMutex& m_lock; - bool m_taken_over { false }; -}; diff --git a/kernel/src/lib/Mutex.cpp b/kernel/src/lib/Mutex.cpp new file mode 100644 index 00000000..0919471a --- /dev/null +++ b/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; +} diff --git a/kernel/src/lib/Mutex.h b/kernel/src/lib/Mutex.h new file mode 100644 index 00000000..bf36e992 --- /dev/null +++ b/kernel/src/lib/Mutex.h @@ -0,0 +1,44 @@ +#pragma once +#include "thread/Thread.h" +#include +#include + +class Mutex +{ + public: + void lock(); + void unlock(); + bool try_lock(); + + private: + CircularQueue m_blocked_threads; + Spinlock m_spinlock; + Atomic 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 }; +}; diff --git a/kernel/src/memory/MemoryManager.cpp b/kernel/src/memory/MemoryManager.cpp index 4507f9fb..fc530b16 100644 --- a/kernel/src/memory/MemoryManager.cpp +++ b/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 #include #include