Kernel: Use a dedicated thread state for wait-queued threads

Instead of using the generic block mechanism, wait-queued threads now
go into the special Queued state.

This fixes an issue where signal dispatch would unblock a wait-queued
thread (because signal dispatch unblocks blocked threads) and cause
confusion since the thread only expected to be awoken by the queue.

5 files changed, 41 insertions, 58 deletions

diff --git a/Kernel/Lock.cpp b/Kernel/Lock.cpp
index 7ff87359c1..f1d0888dee 100644
--- a/Kernel/Lock.cpp
+++ b/Kernel/Lock.cpp
@@ -19,7 +19,7 @@ void Lock::lock()
                 return;
             }
             m_lock.store(false, AK::memory_order_release);
-            (void)current->donate_remaining_timeslice_and_block<Thread::WaitQueueBlocker>(m_holder, m_name, m_queue);
+            current->wait_on(m_queue, m_holder, m_name);
         }
     }
 }
diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp
index f64c0e3647..c7fbae992b 100644
--- a/Kernel/Scheduler.cpp
+++ b/Kernel/Scheduler.cpp
@@ -82,18 +82,6 @@ bool Thread::JoinBlocker::should_unblock(Thread& joiner, time_t, long)
     return !joiner.m_joinee;
 }
 
-Thread::WaitQueueBlocker::WaitQueueBlocker(WaitQueue& queue)
-    : m_queue(queue)
-{
-    m_queue.enqueue(*current);
-}
-
-bool Thread::WaitQueueBlocker::should_unblock(Thread&, time_t, long)
-{
-    // Someone else will have to unblock us by calling wake_one() or wake_all() on the queue.
-    return false;
-}
-
 Thread::FileDescriptionBlocker::FileDescriptionBlocker(const FileDescription& description)
     : m_blocked_description(description)
 {
@@ -268,6 +256,7 @@ void Thread::consider_unblock(time_t now_sec, long now_usec)
     case Thread::Running:
     case Thread::Dead:
     case Thread::Stopped:
+    case Thread::Queued:
         /* don't know, don't care */
         return;
     case Thread::Blocked:
diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp
index eacc56d951..888fb02ce8 100644
--- a/Kernel/Thread.cpp
+++ b/Kernel/Thread.cpp
@@ -180,12 +180,14 @@ void Thread::yield_without_holding_big_lock()
         process().big_lock().lock();
 }
 
-void Thread::donate_and_yield_without_holding_big_lock(Thread* beneficiary, const char* reason)
+bool Thread::unlock_process_if_locked()
 {
-    bool did_unlock = process().big_lock().unlock_if_locked();
-    Scheduler::donate_to(beneficiary, reason);
-    if (did_unlock)
-        process().big_lock().lock();
+    return process().big_lock().unlock_if_locked();
+}
+
+void Thread::relock_process()
+{
+    process().big_lock().lock();
 }
 
 u64 Thread::sleep(u32 ticks)
@@ -227,6 +229,8 @@ const char* Thread::state_string() const
         return "Skip1";
     case Thread::Skip0SchedulerPasses:
         return "Skip0";
+    case Thread::Queued:
+        return "Queued";
     case Thread::Blocked:
         ASSERT(m_blocker != nullptr);
         return m_blocker->state_string();
@@ -645,6 +649,9 @@ bool Thread::is_thread(void* ptr)
 void Thread::set_state(State new_state)
 {
     InterruptDisabler disabler;
+    if (new_state == m_state)
+        return;
+
     if (new_state == Blocked) {
         // we should always have a Blocker while blocked
         ASSERT(m_blocker != nullptr);
diff --git a/Kernel/Thread.h b/Kernel/Thread.h
index ce1bfa0aa6..54a46e4e2d 100644
--- a/Kernel/Thread.h
+++ b/Kernel/Thread.h
@@ -11,6 +11,7 @@
 #include <Kernel/Scheduler.h>
 #include <Kernel/UnixTypes.h>
 #include <Kernel/VM/Region.h>
+#include <Kernel/WaitQueue.h>
 #include <LibC/fd_set.h>
 
 class Alarm;
@@ -84,6 +85,7 @@ public:
         Dead,
         Stopped,
         Blocked,
+        Queued,
     };
 
     class Blocker {
@@ -111,16 +113,6 @@ public:
         void*& m_joinee_exit_value;
     };
 
-    class WaitQueueBlocker final : public Blocker {
-    public:
-        explicit WaitQueueBlocker(WaitQueue&);
-        virtual bool should_unblock(Thread&, time_t now_s, long us) override;
-        virtual const char* state_string() const override { return "WaitQueued"; }
-
-    private:
-        WaitQueue& m_queue;
-    };
-
     class FileDescriptionBlocker : public Blocker {
     public:
         const FileDescription& blocked_description() const;
@@ -268,30 +260,18 @@ public:
     };
 
     template<typename T, class... Args>
-    [[nodiscard]] BlockResult block_impl(Thread* beneficiary, const char* reason, Args&&... args)
+    [[nodiscard]] BlockResult block(Args&&... args)
     {
         // We should never be blocking a blocked (or otherwise non-active) thread.
         ASSERT(state() == Thread::Running);
         ASSERT(m_blocker == nullptr);
 
-        // NOTE: We disable interrupts here to avoid the situation where a WaitQueueBlocker
-        //       adds the current thread to a WaitQueue, and then someone wakes up before
-        //       we set the state to Blocked decides to wake the queue. They would find
-        //       unblocked threads in a wait queue, which would not be good. We can't go
-        //       into Blocked state earlier, since that would prevent this thread from
-        //       getting scheduled.
-        auto saved_if = cli_and_save_interrupt_flag();
         T t(forward<Args>(args)...);
         m_blocker = &t;
         set_state(Thread::Blocked);
-        restore_interrupt_flag(saved_if);
 
         // Yield to the scheduler, and wait for us to resume unblocked.
-        if (beneficiary) {
-            donate_and_yield_without_holding_big_lock(beneficiary, reason);
-        } else {
-            yield_without_holding_big_lock();
-        }
+        yield_without_holding_big_lock();
 
         // We should no longer be blocked once we woke up
         ASSERT(state() != Thread::Blocked);
@@ -305,26 +285,31 @@ public:
         return BlockResult::WokeNormally;
     }
 
-    template<typename T, class... Args>
-    [[nodiscard]] BlockResult block(Args&&... args)
-    {
-        return block_impl<T>(nullptr, nullptr, forward<Args>(args)...);
-    }
-
-    template<typename T, class... Args>
-    [[nodiscard]] BlockResult donate_remaining_timeslice_and_block(Thread* beneficiary, const char* reason, Args&&... args)
+    [[nodiscard]] BlockResult block_until(const char* state_string, Function<bool()>&& condition)
     {
-        return block_impl<T>(beneficiary, reason, forward<Args>(args)...);
+        return block<ConditionBlocker>(state_string, move(condition));
     }
 
-    [[nodiscard]] BlockResult block_until(const char* state_string, Function<bool()>&& condition)
+    void wait_on(WaitQueue& queue, Thread* beneficiary = nullptr, const char* reason = nullptr)
     {
-        return block<ConditionBlocker>(state_string, move(condition));
+        bool did_unlock = unlock_process_if_locked();
+        cli();
+        set_state(State::Queued);
+        queue.enqueue(*current);
+        // Yield and wait for the queue to wake us up again.
+        if (beneficiary)
+            Scheduler::donate_to(beneficiary, reason);
+        else
+            Scheduler::yield();
+        // We've unblocked, relock the process if needed and carry on.
+        if (did_unlock)
+            relock_process();
     }
 
-    void wait_on(WaitQueue& queue)
+    void wake_from_queue()
     {
-        (void)block<WaitQueueBlocker>(queue);
+        ASSERT(state() == State::Queued);
+        set_state(State::Runnable);
     }
 
     void unblock();
@@ -400,6 +385,9 @@ private:
 
 private:
     friend class SchedulerData;
+    bool unlock_process_if_locked();
+    void relock_process();
+
     String backtrace_impl() const;
     Process& m_process;
     int m_tid { -1 };
@@ -436,7 +424,6 @@ private:
     bool m_should_die { false };
 
     void yield_without_holding_big_lock();
-    void donate_and_yield_without_holding_big_lock(Thread* beneficiary, const char* reason);
 };
 
 HashTable<Thread*>& thread_table();
diff --git a/Kernel/WaitQueue.cpp b/Kernel/WaitQueue.cpp
index c0935f05a1..502027367a 100644
--- a/Kernel/WaitQueue.cpp
+++ b/Kernel/WaitQueue.cpp
@@ -21,7 +21,7 @@ void WaitQueue::wake_one()
     if (m_threads.is_empty())
         return;
     if (auto* thread = m_threads.take_first())
-        thread->unblock();
+        thread->wake_from_queue();
 }
 
 void WaitQueue::wake_all()
@@ -30,5 +30,5 @@ void WaitQueue::wake_all()
     if (m_threads.is_empty())
         return;
     while (!m_threads.is_empty())
-        m_threads.take_first()->unblock();
+        m_threads.take_first()->wake_from_queue();
 }