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 :^)

kernel/include/kernel/Lock/BlockableSpinLock.h

@@ -0,0 +1,41 @@
+#pragma once
+
+#include <kernel/Lock/SpinLock.h>
+#include <kernel/Lock/Mutex.h>
+
+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<typename Lock> 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;
+	};
+
+}

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<pid_t>	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<pid_t>		m_locker		{ -1 };

kernel/include/kernel/Lock/RWLock.h

@@ -1,7 +1,7 @@
 #pragma once
 
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Lock/SpinLock.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 
 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--;
 

kernel/include/kernel/Lock/SpinLockAsMutex.h

@@ -1,64 +0,0 @@
-#pragma once
-
-#include <kernel/Lock/SpinLock.h>
-#include <kernel/Lock/Mutex.h>
-
-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<typename Lock>
-	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<typename Lock>
-	class SpinLockGuardAsMutex : public SpinLockAsMutex<Lock>
-	{
-	public:
-		SpinLockGuardAsMutex(SpinLockGuard<Lock>& guard)
-			: SpinLockAsMutex<Lock>(guard.m_lock, guard.m_state)
-		{}
-	};
-
-}

kernel/kernel/ACPI/ACPI.cpp

@@ -6,7 +6,6 @@
 #include <kernel/BootInfo.h>
 #include <kernel/InterruptController.h>
 #include <kernel/IO.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/Memory/PageTable.h>
 #include <kernel/Process.h>
 #include <kernel/Timer/Timer.h>

kernel/kernel/Audio/Controller.cpp

@@ -3,7 +3,7 @@
 #include <kernel/Audio/HDAudio/Controller.h>
 #include <kernel/Device/DeviceNumbers.h>
 #include <kernel/FS/DevFS/FileSystem.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 
 #include <sys/ioctl.h>
 #include <sys/sysmacros.h>
@@ -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());

kernel/kernel/Audio/HDAudio/Controller.cpp

@@ -1,8 +1,8 @@
 #include <kernel/Audio/HDAudio/AudioFunctionGroup.h>
 #include <kernel/Audio/HDAudio/Controller.h>
 #include <kernel/Audio/HDAudio/Registers.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Lock/LockGuard.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/MMIO.h>
 #include <kernel/Timer/Timer.h>
 
@@ -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);

kernel/kernel/Epoll.cpp

@@ -1,6 +1,6 @@
 #include <kernel/Epoll.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Lock/LockGuard.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/Timer/Timer.h>
 
 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;

kernel/kernel/FS/DevFS/FileSystem.cpp

@@ -7,8 +7,8 @@
 #include <kernel/FS/DevFS/FileSystem.h>
 #include <kernel/FS/TmpFS/Inode.h>
 #include <kernel/Input/InputDevice.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Lock/LockGuard.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/Process.h>
 #include <kernel/Scheduler.h>
 #include <kernel/Storage/StorageDevice.h>
@@ -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;
 						}
 					}

kernel/kernel/Input/InputDevice.cpp

@@ -3,7 +3,7 @@
 #include <kernel/Device/DeviceNumbers.h>
 #include <kernel/FS/DevFS/FileSystem.h>
 #include <kernel/Input/InputDevice.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Terminal/TTY.h>
 
 #include <LibInput/Joystick.h>
@@ -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));
 		}
 	}
 

kernel/kernel/Networking/ARPTable.cpp

@@ -1,4 +1,3 @@
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/Networking/ARPTable.h>
 #include <kernel/Scheduler.h>
 #include <kernel/Timer/Timer.h>

kernel/kernel/Networking/E1000/E1000.cpp

@@ -1,7 +1,7 @@
 #include <kernel/IDT.h>
 #include <kernel/InterruptController.h>
 #include <kernel/IO.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Memory/PageTable.h>
 #include <kernel/MMIO.h>
 #include <kernel/Networking/E1000/E1000.h>
@@ -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;

kernel/kernel/Networking/IPv4Layer.cpp

@@ -1,6 +1,6 @@
+#include <kernel/Lock/SpinLock.h>
 #include <kernel/Memory/Heap.h>
 #include <kernel/Memory/PageTable.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/Networking/ICMP.h>
 #include <kernel/Networking/IPv4Layer.h>
 #include <kernel/Networking/NetworkManager.h>

kernel/kernel/Networking/Loopback.cpp

@@ -1,4 +1,4 @@
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Networking/Loopback.h>
 #include <kernel/Networking/NetworkManager.h>
 
@@ -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;

kernel/kernel/Networking/RTL8169/RTL8169.cpp

@@ -1,4 +1,4 @@
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Networking/NetworkManager.h>
 #include <kernel/Networking/RTL8169/Definitions.h>
 #include <kernel/Networking/RTL8169/RTL8169.h>
@@ -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;

kernel/kernel/Networking/UDPSocket.cpp

@@ -1,5 +1,5 @@
 #include <kernel/Lock/LockGuard.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Memory/Heap.h>
 #include <kernel/Networking/UDPSocket.h>
 #include <kernel/Thread.h>
@@ -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();

kernel/kernel/Process.cpp

@@ -10,8 +10,8 @@
 #include <kernel/FS/VirtualFileSystem.h>
 #include <kernel/IDT.h>
 #include <kernel/InterruptController.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Lock/LockGuard.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/Memory/FileBackedRegion.h>
 #include <kernel/Memory/Heap.h>
 #include <kernel/Memory/MemoryBackedRegion.h>
@@ -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);
 		}
 	}
 

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();
 		}
 

kernel/kernel/Storage/NVMe/Queue.cpp

@@ -1,4 +1,4 @@
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Storage/NVMe/Queue.h>
 #include <kernel/Thread.h>
 #include <kernel/Timer/Timer.h>
@@ -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;

kernel/kernel/Terminal/PseudoTerminal.cpp

@@ -1,6 +1,6 @@
 #include <kernel/Device/DeviceNumbers.h>
 #include <kernel/FS/DevFS/FileSystem.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
+#include <kernel/Lock/BlockableSpinLock.h>
 #include <kernel/Terminal/PseudoTerminal.h>
 
 #include <BAN/ScopeGuard.h>
@@ -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);

kernel/kernel/Terminal/TTY.cpp

@@ -7,7 +7,6 @@
 #include <kernel/FS/VirtualFileSystem.h>
 #include <kernel/Input/InputDevice.h>
 #include <kernel/Lock/LockGuard.h>
-#include <kernel/Lock/SpinLockAsMutex.h>
 #include <kernel/Process.h>
 #include <kernel/Terminal/TTY.h>
 #include <kernel/Timer/Timer.h>