Kernel: Rewrite WaitQueue

[implicitfield]

Part of #26108.

The old WaitQueue is still around (it's just been renamed to DeprecatedWaitQueue) since not everything has been ported to the new WaitQueue.
There's now also a ConditionWaiter class (can be renamed if we can think of a better name) which allows blocking on a specific (atomic) boolean in a slightly less verbose way as compared to using a Waiter directly.

Porting to the new WaitQueue shouldn't be too hard (there's two such ports in this PR already), though at least KernelRng will require special attention (not done in this PR) because it currently deadlocks when entropy exhaustion occurs.

[spholz]

Thank you very much for working on this!

[Hendiadyoin1]

@spholz reading through this, I am not sure if this works towards solving the issue we have in the xhci code, whats your opinion on that?

Also quite nice getting this out of the less linear to read blocking code

[implicitfield]

Took a break, and recalled that the original article suggested using linked lists. I've replaced the Vector with a linked list-based approach, so that we're fully allocation-free (whereas previously the Vector's inline capacity would run out at some point). This is actually pretty similar to the IntrusiveList idea, just... without using an IntrusiveList

[spholz]

@spholz reading through this, I am not sure if this works towards solving the issue we have in the xhci code, whats your opinion on that?

Why wouldn't it help for xHCI? We would just use a boolean flag similar to the FinalizerTask code in this PR. Alternatively, we could maybe even check if the event ring itself has events for m_event_queue.

Also quite nice getting this out of the less linear to read blocking code

I'm actually kind of hoping that we can maybe replace some Blockers with WaitQueues instead. But I haven't looked into that yet.

[implicitfield]

I guess this is ready for review again. I still had to add some extra thread functions so that we can tell if a signal dispatch occurred, and return EINTR if it did (since the signal will already be gone when we check this). Internal kernel threads can probably just use MUST if they don't expect to receive signals.

I wrote a patch that allows interacting with WaitQueue in a syscall context, and it does seem to block and handle interrupts properly:

--- a/Kernel/API/Syscall.h
+++ b/Kernel/API/Syscall.h
@@ -52,6 +52,7 @@ enum class NeedsBigProcessLock {
     S(annotate_mapping, NeedsBigProcessLock::No)           \
     S(bind, NeedsBigProcessLock::No)                       \
     S(bindmount, NeedsBigProcessLock::No)                  \
+    S(block, NeedsBigProcessLock::No)                      \
     S(chdir, NeedsBigProcessLock::No)                      \
     S(chmod, NeedsBigProcessLock::No)                      \
     S(chown, NeedsBigProcessLock::No)                      \
--- a/Kernel/CMakeLists.txt
+++ b/Kernel/CMakeLists.txt
@@ -302,6 +302,7 @@ set(KERNEL_SOURCES
     Security/UBSanitizer.cpp
     Syscalls/anon_create.cpp
     Syscalls/alarm.cpp
+    Syscalls/block.cpp
     Syscalls/chdir.cpp
     Syscalls/chmod.cpp
     Syscalls/chown.cpp
--- /dev/null
+++ b/Kernel/Syscalls/block.cpp
@@ -0,0 +1,12 @@
+#include <Kernel/Tasks/Process.h>
+#include <Kernel/Tasks/WaitQueue.h>
+
+ErrorOr<FlatPtr> Kernel::Process::sys$block()
+{
+    WaitQueue queue;
+    Spinlock<LockRank::None> lock;
+    (void)queue.wait_until(lock, []() -> bool {
+        return false;
+    });
+    return 0;
+}
--- a/Kernel/Tasks/Process.h
+++ b/Kernel/Tasks/Process.h
@@ -351,6 +351,7 @@ public:
     ErrorOr<FlatPtr> sys$yield();
     ErrorOr<FlatPtr> sys$sync();
     ErrorOr<FlatPtr> sys$beep(int tone);
+    ErrorOr<FlatPtr> sys$block();
     ErrorOr<FlatPtr> sys$create_inode_watcher(u32 flags);
     ErrorOr<FlatPtr> sys$inode_watcher_add_watch(Userspace<Syscall::SC_inode_watcher_add_watch_params const*> user_params);
     ErrorOr<FlatPtr> sys$inode_watcher_remove_watch(int fd, int wd);
--- a/Userland/Utilities/CMakeLists.txt
+++ b/Userland/Utilities/CMakeLists.txt
@@ -10,6 +10,7 @@ set(CMD_SOURCES_CPP
     base64.cpp
     basename.cpp
     beep.cpp
+    block.cpp
     blockdev.cpp
     bt.cpp
     cal.cpp
@@ -302,6 +303,7 @@ target_link_libraries(aconv PRIVATE LibAudio LibFileSystem)
 target_link_libraries(animation PRIVATE LibGfx)
 target_link_libraries(aplay PRIVATE LibAudio LibFileSystem LibIPC)
 target_link_libraries(asctl PRIVATE LibAudio LibIPC)
+target_link_libraries(block PRIVATE LibSystem)
 target_link_libraries(bt PRIVATE LibSymbolication LibURL)
 target_link_libraries(checksum PRIVATE LibCrypto)
 target_link_libraries(chres PRIVATE LibGUI LibIPC)
--- /dev/null
+++ b/Userland/Utilities/block.cpp
@@ -0,0 +1,8 @@
+#include <LibMain/Main.h>
+#include <syscall.h>
+
+ErrorOr<int> serenity_main(Main::Arguments)
+{
+    syscall(SC_block);
+    return 0;
+}

[spholz]

When setting this variable you use seq_cst (the default memory order) and when reading, consume.

Consume is a weird memory order and I wouldn't recommend you use it, especially since it's scheduled for deprecation in C++26.

Using seq_cst is a bit eh, but I'm also not quite sure what memory order we would need here.

[implicitfield]

I guess we might as well use seq_cst here as well then (though I guess acquire would have the same result). has_unmasked_pending_signals() also uses consume, but I'm not sure why.

One thing that just having a bool for this causes is that a SIGSTOP / SIGCONT sequence (or even just a SIGCONT to a running process) will lead to this being interrupted, but that seems to match how blockers work, so it might be better to fix this globally at some later point.

[spholz]

It'd be nice to get rid of the seq_cst in the future, but for now it's good enough I guess.

[spholz]

Thank you very much for working on this!

(Seriously, I think this is a very nice change!)

[implicitfield]

Thanks for reviewing this (and the thread/signal/scheduler cleanup PR). It's kind of nice that this had the side effect of making all of that slightly easier to follow.