From 5329ec5912e3f0ec20680ee8c39ee21cec7a3a2d Mon Sep 17 00:00:00 2001 From: Bananymous Date: Sat, 11 Jul 2026 06:22:41 +0300 Subject: [PATCH] Kernel: Rework spinlock usage when blocking the current thread The old SpinLockAsMutex was pretty confusing first of all. It also was causing the issue thats been around for maybe a year now which is the only consistently happening kernel panic. I've been pretty confused about this and finally figured out what was causing this. The main issue was that we were accidentally enabling interrupts when blocking a thread that passed SpinLockAsMutex from normally interrupt enabled context. This led to receiving IPI for thread unblock while we were actively blocking the thread. I'm very suprized this had't caused any more serious issues than occasional kernel panics :^) --- .../include/kernel/Lock/BlockableSpinLock.h | 41 ++++++++++++ kernel/include/kernel/Lock/Mutex.h | 20 +++--- kernel/include/kernel/Lock/RWLock.h | 10 +-- kernel/include/kernel/Lock/SpinLockAsMutex.h | 64 ------------------- kernel/kernel/ACPI/ACPI.cpp | 1 - kernel/kernel/Audio/Controller.cpp | 6 +- kernel/kernel/Audio/HDAudio/Controller.cpp | 6 +- kernel/kernel/Epoll.cpp | 6 +- kernel/kernel/FS/DevFS/FileSystem.cpp | 6 +- kernel/kernel/Input/InputDevice.cpp | 18 +++--- kernel/kernel/Networking/ARPTable.cpp | 1 - kernel/kernel/Networking/E1000/E1000.cpp | 6 +- kernel/kernel/Networking/IPv4Layer.cpp | 2 +- kernel/kernel/Networking/Loopback.cpp | 10 +-- kernel/kernel/Networking/RTL8169/RTL8169.cpp | 10 +-- kernel/kernel/Networking/UDPSocket.cpp | 6 +- kernel/kernel/Process.cpp | 10 +-- kernel/kernel/Scheduler.cpp | 2 +- kernel/kernel/Storage/NVMe/Queue.cpp | 6 +- kernel/kernel/Terminal/PseudoTerminal.cpp | 6 +- kernel/kernel/Terminal/TTY.cpp | 1 - 21 files changed, 105 insertions(+), 133 deletions(-) create mode 100644 kernel/include/kernel/Lock/BlockableSpinLock.h delete mode 100644 kernel/include/kernel/Lock/SpinLockAsMutex.h diff --git a/kernel/include/kernel/Lock/BlockableSpinLock.h b/kernel/include/kernel/Lock/BlockableSpinLock.h new file mode 100644 index 00000000..56b0ecac --- /dev/null +++ b/kernel/include/kernel/Lock/BlockableSpinLock.h @@ -0,0 +1,41 @@ +#pragma once + +#include +#include + +namespace Kernel +{ + + // FIXME: These classes are HACKS to allow passing spinlock + // to unblock functions. Write a better API that either + // allows passing spinlocks or do something cleaner that + // whatever shit this is + + template requires requires (Lock& lock) { lock.lock(); lock.unlock(InterruptState::Disabled); lock.current_processor_has_lock(); } + class BlockableSpinLock : public BaseMutex + { + public: + BlockableSpinLock(Lock& lock) + : m_lock(lock) + { + ASSERT(m_lock.current_processor_has_lock()); + } + + void lock() override + { + m_lock.lock(); + } + + void unlock() override + { + m_lock.unlock(InterruptState::Disabled); + } + + uint32_t lock_depth() const override { return m_lock.lock_depth(); } + bool is_locked_by_current_thread() const override { return m_lock.current_processor_has_lock(); } + + private: + Lock& m_lock; + }; + +} diff --git a/kernel/include/kernel/Lock/Mutex.h b/kernel/include/kernel/Lock/Mutex.h index 724647bb..d28689e9 100644 --- a/kernel/include/kernel/Lock/Mutex.h +++ b/kernel/include/kernel/Lock/Mutex.h @@ -13,12 +13,10 @@ namespace Kernel { public: virtual void lock() = 0; - virtual bool try_lock() = 0; virtual void unlock() = 0; - virtual pid_t locker() const = 0; - virtual bool is_locked() const = 0; virtual uint32_t lock_depth() const = 0; + virtual bool is_locked_by_current_thread() const = 0; }; class Mutex final : public BaseMutex @@ -51,7 +49,7 @@ namespace Kernel m_lock_depth++; } - bool try_lock() override + bool try_lock() { const auto tid = Thread::current_tid(); if (tid == m_locker) @@ -82,10 +80,10 @@ namespace Kernel } } - pid_t locker() const override { return m_locker; } - bool is_locked() const override { return m_locker != -1; } + pid_t locker() const { return m_locker; } + bool is_locked() const { return m_locker != -1; } uint32_t lock_depth() const override { return m_lock_depth; } - bool is_locked_by_current_thread() const { return m_locker == Thread::current_tid(); } + bool is_locked_by_current_thread() const override { return m_locker == Thread::current_tid(); } private: BAN::Atomic m_locker { -1 }; @@ -126,7 +124,7 @@ namespace Kernel m_lock_depth++; } - bool try_lock() override + bool try_lock() { const auto tid = Thread::current_tid(); @@ -164,10 +162,10 @@ namespace Kernel } } - pid_t locker() const override { return m_locker; } - bool is_locked() const override { return m_locker != -1; } + pid_t locker() const { return m_locker; } + bool is_locked() const { return m_locker != -1; } uint32_t lock_depth() const override { return m_lock_depth; } - bool is_locked_by_current_thread() const { return m_locker == Thread::current_tid(); } + bool is_locked_by_current_thread() const override { return m_locker == Thread::current_tid(); } private: BAN::Atomic m_locker { -1 }; diff --git a/kernel/include/kernel/Lock/RWLock.h b/kernel/include/kernel/Lock/RWLock.h index b829bf7c..39c57b27 100644 --- a/kernel/include/kernel/Lock/RWLock.h +++ b/kernel/include/kernel/Lock/RWLock.h @@ -1,7 +1,7 @@ #pragma once +#include #include -#include namespace Kernel { @@ -18,8 +18,8 @@ namespace Kernel SpinLockGuard _(m_lock); while (m_writers_waiting > 0 || m_writer != -1) { - SpinLockGuardAsMutex smutex(_); - m_thread_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_lock); + m_thread_blocker.block_indefinite(&block); } m_readers_active++; } @@ -44,8 +44,8 @@ namespace Kernel m_writers_waiting++; while (m_readers_active > 0 || m_writer != -1) { - SpinLockGuardAsMutex smutex(_); - m_thread_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_lock); + m_thread_blocker.block_indefinite(&block); } m_writers_waiting--; diff --git a/kernel/include/kernel/Lock/SpinLockAsMutex.h b/kernel/include/kernel/Lock/SpinLockAsMutex.h deleted file mode 100644 index d93b6882..00000000 --- a/kernel/include/kernel/Lock/SpinLockAsMutex.h +++ /dev/null @@ -1,64 +0,0 @@ -#pragma once - -#include -#include - -namespace Kernel -{ - - // FIXME: These classes are HACKS to allow passing spinlock - // to unblock functions. Write a better API that either - // allows passing spinlocks or do something cleaner that - // whatever shit this is - - template - class SpinLockAsMutex : public BaseMutex - { - public: - SpinLockAsMutex(Lock& lock, InterruptState state) - : m_lock(lock) - , m_lock_depth(lock.lock_depth()) - , m_state(state) - , m_locker(Thread::current_tid()) - { - ASSERT(m_lock.current_processor_has_lock()); - } - - void lock() override - { - m_lock.lock(); - m_lock_depth++; - } - - bool try_lock() override - { - lock(); - return true; - } - - void unlock() override - { - m_lock.unlock(--m_lock_depth ? InterruptState::Disabled : m_state); - } - - pid_t locker() const override { return is_locked() ? m_locker : -1; } - bool is_locked() const override { return m_lock_depth; } - uint32_t lock_depth() const override { return m_lock_depth; } - - private: - Lock& m_lock; - uint32_t m_lock_depth { 0 }; - InterruptState m_state; - const pid_t m_locker; - }; - - template - class SpinLockGuardAsMutex : public SpinLockAsMutex - { - public: - SpinLockGuardAsMutex(SpinLockGuard& guard) - : SpinLockAsMutex(guard.m_lock, guard.m_state) - {} - }; - -} diff --git a/kernel/kernel/ACPI/ACPI.cpp b/kernel/kernel/ACPI/ACPI.cpp index 8088f890..b47da7a0 100644 --- a/kernel/kernel/ACPI/ACPI.cpp +++ b/kernel/kernel/ACPI/ACPI.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include diff --git a/kernel/kernel/Audio/Controller.cpp b/kernel/kernel/Audio/Controller.cpp index cc3a1b04..21fbe940 100644 --- a/kernel/kernel/Audio/Controller.cpp +++ b/kernel/kernel/Audio/Controller.cpp @@ -3,7 +3,7 @@ #include #include #include -#include +#include #include #include @@ -65,8 +65,8 @@ namespace Kernel while (m_sample_data->full()) { - SpinLockGuardAsMutex smutex(lock_guard); - TRY(Thread::current().block_or_eintr_indefinite(m_sample_data_blocker, &smutex)); + BlockableSpinLock block(m_spinlock); + TRY(Thread::current().block_or_eintr_indefinite(m_sample_data_blocker, &block)); } const size_t to_copy = BAN::Math::min(buffer.size(), m_sample_data->free()); diff --git a/kernel/kernel/Audio/HDAudio/Controller.cpp b/kernel/kernel/Audio/HDAudio/Controller.cpp index beab5511..4395d1b0 100644 --- a/kernel/kernel/Audio/HDAudio/Controller.cpp +++ b/kernel/kernel/Audio/HDAudio/Controller.cpp @@ -1,8 +1,8 @@ #include #include #include +#include #include -#include #include #include @@ -369,8 +369,8 @@ namespace Kernel { if (SystemTimer::get().ms_since_boot() > waketime_ms) return BAN::Error::from_errno(ETIMEDOUT); - SpinLockGuardAsMutex smutex(sguard); - m_rb_blocker.block_with_timeout_ms(10, &smutex); + BlockableSpinLock block(m_rb_lock); + m_rb_blocker.block_with_timeout_ms(10, &block); } const size_t offset = 2 * m_rirb.index * sizeof(uint32_t); diff --git a/kernel/kernel/Epoll.cpp b/kernel/kernel/Epoll.cpp index ba0b23c9..ba3bc9d1 100644 --- a/kernel/kernel/Epoll.cpp +++ b/kernel/kernel/Epoll.cpp @@ -1,6 +1,6 @@ #include +#include #include -#include #include namespace Kernel @@ -222,8 +222,8 @@ namespace Kernel if (!m_ready_events.empty()) continue; - SpinLockGuardAsMutex smutex(guard); - TRY(Thread::current().block_or_eintr_or_waketime_ns(m_thread_blocker, waketime_ns, false, &smutex)); + BlockableSpinLock block(m_ready_lock); + TRY(Thread::current().block_or_eintr_or_waketime_ns(m_thread_blocker, waketime_ns, false, &block)); } return event_count; diff --git a/kernel/kernel/FS/DevFS/FileSystem.cpp b/kernel/kernel/FS/DevFS/FileSystem.cpp index 11926ca4..b363b69a 100644 --- a/kernel/kernel/FS/DevFS/FileSystem.cpp +++ b/kernel/kernel/FS/DevFS/FileSystem.cpp @@ -7,8 +7,8 @@ #include #include #include +#include #include -#include #include #include #include @@ -77,8 +77,8 @@ namespace Kernel bool expected = true; if (!devfs->m_should_drop_disk_cache.compare_exchange(expected, false)) { - SpinLockGuardAsMutex smutex(guard); - devfs->m_disk_cache_thread_blocker.block_indefinite(&smutex); + BlockableSpinLock block(devfs->m_disk_cache_lock); + devfs->m_disk_cache_thread_blocker.block_indefinite(&block); continue; } } diff --git a/kernel/kernel/Input/InputDevice.cpp b/kernel/kernel/Input/InputDevice.cpp index 583504e0..0ddba30d 100644 --- a/kernel/kernel/Input/InputDevice.cpp +++ b/kernel/kernel/Input/InputDevice.cpp @@ -3,7 +3,7 @@ #include #include #include -#include +#include #include #include @@ -245,8 +245,8 @@ namespace Kernel while (m_event_count == 0) { // FIXME: should m_mutex be unlocked? - SpinLockGuardAsMutex smutex(guard); - TRY(Thread::current().block_or_eintr_indefinite(m_event_thread_blocker, &smutex)); + BlockableSpinLock block(m_event_lock); + TRY(Thread::current().block_or_eintr_indefinite(m_event_thread_blocker, &block)); } memcpy(buffer.data(), &m_event_buffer[m_event_tail * m_event_size], m_event_size); @@ -289,8 +289,8 @@ namespace Kernel if (s_tty_keyboard_events.empty()) { - SpinLockGuardAsMutex smutex(guard); - s_tty_keyboard_event_blocker.block_indefinite(&smutex); + BlockableSpinLock block(s_tty_keyboard_event_lock); + s_tty_keyboard_event_blocker.block_indefinite(&block); continue; } @@ -350,8 +350,8 @@ namespace Kernel return bytes; } - SpinLockGuardAsMutex smutex(keyboard_guard); - TRY(Thread::current().block_or_eintr_indefinite(m_thread_blocker, &smutex)); + BlockableSpinLock block(s_keyboard_lock); + TRY(Thread::current().block_or_eintr_indefinite(m_thread_blocker, &block)); } } @@ -407,8 +407,8 @@ namespace Kernel return bytes; } - SpinLockGuardAsMutex smutex(mouse_guard); - TRY(Thread::current().block_or_eintr_indefinite(m_thread_blocker, &smutex)); + BlockableSpinLock block(s_mouse_lock); + TRY(Thread::current().block_or_eintr_indefinite(m_thread_blocker, &block)); } } diff --git a/kernel/kernel/Networking/ARPTable.cpp b/kernel/kernel/Networking/ARPTable.cpp index abdf6552..93d11d44 100644 --- a/kernel/kernel/Networking/ARPTable.cpp +++ b/kernel/kernel/Networking/ARPTable.cpp @@ -1,4 +1,3 @@ -#include #include #include #include diff --git a/kernel/kernel/Networking/E1000/E1000.cpp b/kernel/kernel/Networking/E1000/E1000.cpp index 479e8838..e6c5b4e9 100644 --- a/kernel/kernel/Networking/E1000/E1000.cpp +++ b/kernel/kernel/Networking/E1000/E1000.cpp @@ -1,7 +1,7 @@ #include #include #include -#include +#include #include #include #include @@ -380,8 +380,8 @@ namespace Kernel if (rx_current != rx_tail) write32(REG_RDT0, rx_tail); - SpinLockGuardAsMutex smutex(guard); - m_rx_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_rx_lock); + m_rx_blocker.block_indefinite(&block); } m_thread_is_dead = true; diff --git a/kernel/kernel/Networking/IPv4Layer.cpp b/kernel/kernel/Networking/IPv4Layer.cpp index dd5dfb81..4116f72d 100644 --- a/kernel/kernel/Networking/IPv4Layer.cpp +++ b/kernel/kernel/Networking/IPv4Layer.cpp @@ -1,6 +1,6 @@ +#include #include #include -#include #include #include #include diff --git a/kernel/kernel/Networking/Loopback.cpp b/kernel/kernel/Networking/Loopback.cpp index 37c1bce0..3f795500 100644 --- a/kernel/kernel/Networking/Loopback.cpp +++ b/kernel/kernel/Networking/Loopback.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include @@ -72,8 +72,8 @@ namespace Kernel descriptor.state = 1; return descriptor; } - SpinLockGuardAsMutex smutex(guard); - m_thread_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_buffer_lock); + m_thread_blocker.block_indefinite(&block); } }(); @@ -118,8 +118,8 @@ namespace Kernel m_thread_blocker.unblock(); } - SpinLockGuardAsMutex smutex(guard); - m_thread_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_buffer_lock); + m_thread_blocker.block_indefinite(&block); } m_thread_is_dead = true; diff --git a/kernel/kernel/Networking/RTL8169/RTL8169.cpp b/kernel/kernel/Networking/RTL8169/RTL8169.cpp index 41f9f54e..8d9a075f 100644 --- a/kernel/kernel/Networking/RTL8169/RTL8169.cpp +++ b/kernel/kernel/Networking/RTL8169/RTL8169.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include #include @@ -218,8 +218,8 @@ namespace Kernel SpinLockGuard guard(m_tx_lock); while (descriptor.command & RTL8169_DESC_CMD_OWN) { - SpinLockGuardAsMutex smutex(guard); - m_tx_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_tx_lock); + m_tx_blocker.block_indefinite(&block); } } @@ -308,8 +308,8 @@ namespace Kernel m_rx_head = (m_rx_head + 1) % m_rx_descriptor_count; } - SpinLockGuardAsMutex smutex(rx_lock_guard); - m_rx_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_rx_lock); + m_rx_blocker.block_indefinite(&block); } m_rx_thread_is_dead = true; diff --git a/kernel/kernel/Networking/UDPSocket.cpp b/kernel/kernel/Networking/UDPSocket.cpp index beac4d63..030eaf2a 100644 --- a/kernel/kernel/Networking/UDPSocket.cpp +++ b/kernel/kernel/Networking/UDPSocket.cpp @@ -1,5 +1,5 @@ #include -#include +#include #include #include #include @@ -181,8 +181,8 @@ namespace Kernel while (m_packets.empty()) { - SpinLockGuardAsMutex smutex(guard); - TRY(Thread::current().block_or_eintr_indefinite(m_packet_thread_blocker, &smutex)); + BlockableSpinLock block(m_packet_lock); + TRY(Thread::current().block_or_eintr_indefinite(m_packet_thread_blocker, &block)); } auto packet_info = m_packets.front(); diff --git a/kernel/kernel/Process.cpp b/kernel/kernel/Process.cpp index 941bd196..0d323619 100644 --- a/kernel/kernel/Process.cpp +++ b/kernel/kernel/Process.cpp @@ -10,8 +10,8 @@ #include #include #include +#include #include -#include #include #include #include @@ -1197,8 +1197,8 @@ namespace Kernel if (options & WNOHANG) return 0; - SpinLockGuardAsMutex smutex(sguard); - TRY(Thread::current().block_or_eintr_indefinite(m_child_wait_blocker, &smutex)); + BlockableSpinLock block(m_child_wait_lock); + TRY(Thread::current().block_or_eintr_indefinite(m_child_wait_blocker, &block)); } if (user_stat_loc != nullptr) @@ -3171,8 +3171,8 @@ namespace Kernel if (!m_stopped) break; - SpinLockGuardAsMutex smutex(guard); - m_stop_blocker.block_indefinite(&smutex); + BlockableSpinLock block(m_signal_lock); + m_stop_blocker.block_indefinite(&block); } } diff --git a/kernel/kernel/Scheduler.cpp b/kernel/kernel/Scheduler.cpp index 5ea5cab0..4339f4e5 100644 --- a/kernel/kernel/Scheduler.cpp +++ b/kernel/kernel/Scheduler.cpp @@ -689,7 +689,7 @@ namespace Kernel uint32_t lock_depth = 0; if (mutex != nullptr) { - ASSERT(mutex->is_locked() && mutex->locker() == m_current->thread->tid()); + ASSERT(mutex->is_locked_by_current_thread()); lock_depth = mutex->lock_depth(); } diff --git a/kernel/kernel/Storage/NVMe/Queue.cpp b/kernel/kernel/Storage/NVMe/Queue.cpp index e074d03b..f5364a1c 100644 --- a/kernel/kernel/Storage/NVMe/Queue.cpp +++ b/kernel/kernel/Storage/NVMe/Queue.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include #include @@ -91,8 +91,8 @@ namespace Kernel while (~m_used_mask == 0) { - SpinLockGuardAsMutex smutex(guard); - m_thread_blocker.block_with_timeout_ms(s_nvme_command_timeout_ms, &smutex); + BlockableSpinLock block(m_lock); + m_thread_blocker.block_with_timeout_ms(s_nvme_command_timeout_ms, &block); } uint16_t cid = 0; diff --git a/kernel/kernel/Terminal/PseudoTerminal.cpp b/kernel/kernel/Terminal/PseudoTerminal.cpp index 32f12352..56828137 100644 --- a/kernel/kernel/Terminal/PseudoTerminal.cpp +++ b/kernel/kernel/Terminal/PseudoTerminal.cpp @@ -1,6 +1,6 @@ #include #include -#include +#include #include #include @@ -112,8 +112,8 @@ namespace Kernel while (m_buffer_size == 0) { - SpinLockGuardAsMutex smutex(guard); - TRY(Thread::current().block_or_eintr_indefinite(m_buffer_blocker, &smutex)); + BlockableSpinLock block(m_buffer_lock); + TRY(Thread::current().block_or_eintr_indefinite(m_buffer_blocker, &block)); } const size_t to_copy = BAN::Math::min(buffer.size(), m_buffer_size); diff --git a/kernel/kernel/Terminal/TTY.cpp b/kernel/kernel/Terminal/TTY.cpp index 52c4d4a7..6e4cf8f7 100644 --- a/kernel/kernel/Terminal/TTY.cpp +++ b/kernel/kernel/Terminal/TTY.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include