From 49a76164c868a0c8d1af50f9193aef0e8b91994a Mon Sep 17 00:00:00 2001 From: Tom Date: Tue, 22 Dec 2020 11:17:56 -0700 Subject: [PATCH] Kernel: Consolidate the various BlockCondition::unblock variants The unblock_all variant used to ASSERT if a blocker didn't unblock, but it wasn't clear from the name that it would do that. Because the BlockCondition already asserts that no blockers are left at destruction time, it would still catch blockers that haven't been unblocked for whatever reason. Fixes #4496 --- Kernel/FileSystem/File.h | 2 +- Kernel/FileSystem/Plan9FileSystem.cpp | 4 +-- Kernel/Net/Routing.cpp | 2 +- Kernel/Thread.h | 51 +++------------------------ Kernel/ThreadBlockers.cpp | 4 +-- Kernel/WaitQueue.cpp | 10 +++--- 6 files changed, 16 insertions(+), 57 deletions(-) diff --git a/Kernel/FileSystem/File.h b/Kernel/FileSystem/File.h index a56c148f59d..a7717db707c 100644 --- a/Kernel/FileSystem/File.h +++ b/Kernel/FileSystem/File.h @@ -58,7 +58,7 @@ public: void unblock() { ScopedSpinLock lock(m_lock); - do_unblock([&](auto& b, void* data) { + do_unblock([&](auto& b, void* data, bool&) { ASSERT(b.blocker_type() == Thread::Blocker::Type::File); auto& blocker = static_cast(b); return blocker.unblock(false, data); diff --git a/Kernel/FileSystem/Plan9FileSystem.cpp b/Kernel/FileSystem/Plan9FileSystem.cpp index 6e868c037dc..ca87b67e0c5 100644 --- a/Kernel/FileSystem/Plan9FileSystem.cpp +++ b/Kernel/FileSystem/Plan9FileSystem.cpp @@ -469,7 +469,7 @@ bool Plan9FS::Plan9FSBlockCondition::should_add_blocker(Thread::Blocker& b, void void Plan9FS::Plan9FSBlockCondition::unblock_completed(u16 tag) { - unblock([&](Thread::Blocker& b, void*) { + unblock([&](Thread::Blocker& b, void*, bool&) { ASSERT(b.blocker_type() == Thread::Blocker::Type::Plan9FS); auto& blocker = static_cast(b); return blocker.unblock(tag); @@ -478,7 +478,7 @@ void Plan9FS::Plan9FSBlockCondition::unblock_completed(u16 tag) void Plan9FS::Plan9FSBlockCondition::unblock_all() { - BlockCondition::unblock_all([&](Thread::Blocker& b, void*) { + unblock([&](Thread::Blocker& b, void*, bool&) { ASSERT(b.blocker_type() == Thread::Blocker::Type::Plan9FS); auto& blocker = static_cast(b); return blocker.unblock(); diff --git a/Kernel/Net/Routing.cpp b/Kernel/Net/Routing.cpp index c974cb5537c..48330b66974 100644 --- a/Kernel/Net/Routing.cpp +++ b/Kernel/Net/Routing.cpp @@ -77,7 +77,7 @@ class ARPTableBlockCondition : public Thread::BlockCondition { public: void unblock(const IPv4Address& ip_addr, const MACAddress& addr) { - unblock_all([&](auto& b, void*) { + BlockCondition::unblock([&](auto& b, void*, bool&) { ASSERT(b.blocker_type() == Thread::Blocker::Type::Routing); auto& blocker = static_cast(b); return blocker.unblock(false, ip_addr, addr); diff --git a/Kernel/Thread.h b/Kernel/Thread.h index cd852f9958a..3ff8fcd96e9 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -398,36 +398,14 @@ public: protected: template - void unblock(UnblockOne unblock_one) + bool unblock(UnblockOne unblock_one) { ScopedSpinLock lock(m_lock); - do_unblock(unblock_one); + return do_unblock(unblock_one); } template - void do_unblock(UnblockOne unblock_one) - { - ASSERT(m_lock.is_locked()); - for (size_t i = 0; i < m_blockers.size();) { - auto& info = m_blockers[i]; - if (unblock_one(*info.blocker, info.data)) { - m_blockers.remove(i); - continue; - } - - i++; - } - } - - template - bool unblock_some(UnblockOne unblock_one) - { - ScopedSpinLock lock(m_lock); - return do_unblock_some(unblock_one); - } - - template - bool do_unblock_some(UnblockOne unblock_one) + bool do_unblock(UnblockOne unblock_one) { ASSERT(m_lock.is_locked()); bool stop_iterating = false; @@ -443,27 +421,6 @@ public: return !stop_iterating; } - template - bool unblock_all(UnblockOne unblock_one) - { - ScopedSpinLock lock(m_lock); - return do_unblock_all(unblock_one); - } - - template - bool do_unblock_all(UnblockOne unblock_one) - { - ASSERT(m_lock.is_locked()); - bool unblocked_any = false; - for (auto& info : m_blockers) { - bool did_unblock = unblock_one(*info.blocker, info.data); - unblocked_any |= did_unblock; - ASSERT(did_unblock); - } - m_blockers.clear(); - return unblocked_any; - } - virtual bool should_add_blocker(Blocker&, void*) { return true; } SpinLock m_lock; @@ -1167,7 +1124,7 @@ private: private: void do_unblock_joiner() { - do_unblock_all([&](Blocker& b, void*) { + do_unblock([&](Blocker& b, void*, bool&) { ASSERT(b.blocker_type() == Blocker::Type::Join); auto& blocker = static_cast(b); return blocker.unblock(exit_value(), false); diff --git a/Kernel/ThreadBlockers.cpp b/Kernel/ThreadBlockers.cpp index 82218e5d342..8ea3617d619 100644 --- a/Kernel/ThreadBlockers.cpp +++ b/Kernel/ThreadBlockers.cpp @@ -440,7 +440,7 @@ void Thread::WaitBlockCondition::disowned_by_waiter(Process& process) for (size_t i = 0; i < m_processes.size();) { auto& info = m_processes[i]; if (info.process == &process) { - do_unblock([&](Blocker& b, void*) { + do_unblock([&](Blocker& b, void*, bool&) { ASSERT(b.blocker_type() == Blocker::Type::Wait); auto& blocker = static_cast(b); bool did_unblock = blocker.unblock(info.process, WaitBlocker::UnblockFlags::Disowned, 0, false); @@ -479,7 +479,7 @@ bool Thread::WaitBlockCondition::unblock(Process& process, WaitBlocker::UnblockF } } - do_unblock([&](Blocker& b, void*) { + do_unblock([&](Blocker& b, void*, bool&) { ASSERT(b.blocker_type() == Blocker::Type::Wait); auto& blocker = static_cast(b); if (was_waited_already && blocker.is_wait()) diff --git a/Kernel/WaitQueue.cpp b/Kernel/WaitQueue.cpp index d9afc20fbef..0da7a9f6763 100644 --- a/Kernel/WaitQueue.cpp +++ b/Kernel/WaitQueue.cpp @@ -55,7 +55,7 @@ void WaitQueue::wake_one() #ifdef WAITQUEUE_DEBUG dbg() << "WaitQueue @ " << this << ": wake_one"; #endif - bool did_unblock_one = do_unblock_some([&](Thread::Blocker& b, void* data, bool& stop_iterating) { + bool did_unblock_one = do_unblock([&](Thread::Blocker& b, void* data, bool& stop_iterating) { ASSERT(data); ASSERT(b.blocker_type() == Thread::Blocker::Type::Queue); auto& blocker = static_cast(b); @@ -79,7 +79,7 @@ void WaitQueue::wake_n(u32 wake_count) #ifdef WAITQUEUE_DEBUG dbg() << "WaitQueue @ " << this << ": wake_n(" << wake_count << ")"; #endif - bool did_unblock_some = do_unblock_some([&](Thread::Blocker& b, void* data, bool& stop_iterating) { + bool did_unblock_some = do_unblock([&](Thread::Blocker& b, void* data, bool& stop_iterating) { ASSERT(data); ASSERT(b.blocker_type() == Thread::Blocker::Type::Queue); auto& blocker = static_cast(b); @@ -103,14 +103,16 @@ void WaitQueue::wake_all() #ifdef WAITQUEUE_DEBUG dbg() << "WaitQueue @ " << this << ": wake_all"; #endif - bool did_unblock_any = do_unblock_all([&](Thread::Blocker& b, void* data) { + bool did_unblock_any = do_unblock([&](Thread::Blocker& b, void* data, bool&) { ASSERT(data); ASSERT(b.blocker_type() == Thread::Blocker::Type::Queue); auto& blocker = static_cast(b); #ifdef WAITQUEUE_DEBUG dbg() << "WaitQueue @ " << this << ": wake_all unblocking " << *static_cast(data); #endif - return blocker.unblock(); + bool did_unblock = blocker.unblock(); + ASSERT(did_unblock); + return true; }); m_wake_requested = !did_unblock_any; }