diff options
author | Andreas Kling <kling@serenityos.org> | 2021-08-24 13:11:58 +0200 |
---|---|---|
committer | Andreas Kling <kling@serenityos.org> | 2021-08-24 16:37:28 +0200 |
commit | 0c1d41cc8a10595e4449f9cc92681ee9e90e57c2 (patch) | |
tree | 4e237364dc4162d8533cc73c8f404806df69b375 /Kernel | |
parent | adbf472ca7182c96545f0e54aef58560e27656f6 (diff) | |
download | serenity-0c1d41cc8a10595e4449f9cc92681ee9e90e57c2.zip |
Kernel: Simplify Blockers so they don't need a "should block" flag
The `m_should_block` member variable that many of the Thread::Blocker
subclasses had was really only used to carry state from the constructor
to the immediate-unblock-without-blocking escape hatch.
This patch refactors the blockers so that we don't need to hold on
to this flag after setup_blocker(), and instead the return value from
setup_blocker() is the authority on whether the unblock conditions
are already met.
Diffstat (limited to 'Kernel')
-rw-r--r-- | Kernel/Net/Routing.cpp | 8 | ||||
-rw-r--r-- | Kernel/Thread.h | 8 | ||||
-rw-r--r-- | Kernel/ThreadBlockers.cpp | 61 |
3 files changed, 26 insertions, 51 deletions
diff --git a/Kernel/Net/Routing.cpp b/Kernel/Net/Routing.cpp index 8d8e2482e7..3f5c335818 100644 --- a/Kernel/Net/Routing.cpp +++ b/Kernel/Net/Routing.cpp @@ -52,7 +52,6 @@ private: const IPv4Address m_ip_addr; Optional<MACAddress>& m_addr; bool m_did_unblock { false }; - bool m_should_block { true }; }; class ARPTableBlockerSet final : public Thread::BlockerSet { @@ -90,14 +89,11 @@ ARPTableBlocker::ARPTableBlocker(IPv4Address ip_addr, Optional<MACAddress>& addr bool ARPTableBlocker::setup_blocker() { - if (!add_to_blocker_set(*s_arp_table_blocker_set)) - m_should_block = false; - return m_should_block; + return add_to_blocker_set(*s_arp_table_blocker_set); } -void ARPTableBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason) +void ARPTableBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason) { - VERIFY(reason == UnblockImmediatelyReason::TimeoutInThePast || !m_should_block); auto addr = arp_table().with_shared([&](const auto& table) -> auto { return table.get(ip_addr()); }); diff --git a/Kernel/Thread.h b/Kernel/Thread.h index bb926a8998..a4026c7176 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -528,9 +528,7 @@ public: NonnullRefPtr<Thread> m_joinee; void*& m_joinee_exit_value; KResult& m_try_join_result; - bool m_join_error { false }; bool m_did_unblock { false }; - bool m_should_block { true }; }; class WaitQueueBlocker final : public Blocker { @@ -548,7 +546,6 @@ public: protected: WaitQueue& m_wait_queue; StringView m_block_reason; - bool m_should_block { true }; bool m_did_unblock { false }; }; @@ -578,7 +575,6 @@ public: FutexQueue& m_futex_queue; u32 m_bitset { 0 }; u32 m_relock_flags { 0 }; - bool m_should_block { true }; bool m_did_unblock { false }; }; @@ -605,9 +601,6 @@ public: virtual Type blocker_type() const override { return Type::File; } virtual bool unblock(bool, void*) = 0; - - protected: - bool m_should_block { true }; }; class FileDescriptionBlocker : public FileBlocker { @@ -734,7 +727,6 @@ public: bool m_did_unblock { false }; bool m_error { false }; bool m_got_sigchild { false }; - bool m_should_block; }; class WaitBlockerSet final : public BlockerSet { diff --git a/Kernel/ThreadBlockers.cpp b/Kernel/ThreadBlockers.cpp index d75f1fcbbe..4d0c26d320 100644 --- a/Kernel/ThreadBlockers.cpp +++ b/Kernel/ThreadBlockers.cpp @@ -80,28 +80,25 @@ bool Thread::JoinBlocker::setup_blocker() // We need to hold our lock to avoid a race where try_join succeeds // but the joinee is joining immediately SpinlockLocker lock(m_lock); + bool should_block = true; m_try_join_result = m_joinee->try_join([&]() { if (!add_to_blocker_set(m_joinee->m_join_blocker_set)) - m_should_block = false; + should_block = false; }); - m_join_error = m_try_join_result.is_error(); - if (m_join_error) - m_should_block = false; - return m_should_block; + if (m_try_join_result.is_error()) + return false; + return should_block; } void Thread::JoinBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason) { - if (!m_should_block) { - VERIFY(reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet); - return; - } // If we should have blocked but got here it must have been that the // timeout was already in the past. So we need to ask the BlockerSet // to supply us the information. We cannot hold the lock as unblock // could be called by the BlockerSet at any time! - VERIFY(reason == UnblockImmediatelyReason::TimeoutInThePast); - m_joinee->m_join_blocker_set.try_unblock(*this); + if (reason == UnblockImmediatelyReason::TimeoutInThePast) { + m_joinee->m_join_blocker_set.try_unblock(*this); + } } bool Thread::JoinBlocker::unblock(void* value, bool from_add_blocker) @@ -128,9 +125,7 @@ Thread::WaitQueueBlocker::WaitQueueBlocker(WaitQueue& wait_queue, StringView blo bool Thread::WaitQueueBlocker::setup_blocker() { - if (!add_to_blocker_set(m_wait_queue)) - m_should_block = false; - return m_should_block; + return add_to_blocker_set(m_wait_queue); } Thread::WaitQueueBlocker::~WaitQueueBlocker() @@ -158,9 +153,7 @@ Thread::FutexBlocker::FutexBlocker(FutexQueue& futex_queue, u32 bitset) bool Thread::FutexBlocker::setup_blocker() { - if (!add_to_blocker_set(m_futex_queue)) - m_should_block = false; - return m_should_block; + return add_to_blocker_set(m_futex_queue); } Thread::FutexBlocker::~FutexBlocker() @@ -212,9 +205,7 @@ Thread::FileDescriptionBlocker::FileDescriptionBlocker(FileDescription& descript bool Thread::FileDescriptionBlocker::setup_blocker() { m_unblocked_flags = BlockFlags::None; - if (!add_to_blocker_set(m_blocked_description->blocker_set())) - m_should_block = false; - return m_should_block; + return add_to_blocker_set(m_blocked_description->blocker_set()); } bool Thread::FileDescriptionBlocker::unblock(bool from_add_blocker, void*) @@ -238,10 +229,9 @@ bool Thread::FileDescriptionBlocker::unblock(bool from_add_blocker, void*) void Thread::FileDescriptionBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason) { - if (!m_should_block) { - VERIFY(reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet); + if (reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet) return; - } + // If we should have blocked but got here it must have been that the // timeout was already in the past. So we need to ask the BlockerSet // to supply us the information. We cannot hold the lock as unblock @@ -366,15 +356,16 @@ Thread::SelectBlocker::SelectBlocker(FDVector& fds) bool Thread::SelectBlocker::setup_blocker() { + bool should_block = true; for (auto& fd_entry : m_fds) { fd_entry.unblocked_flags = FileBlocker::BlockFlags::None; - if (!m_should_block) + if (!should_block) continue; if (!fd_entry.description->blocker_set().add_blocker(*this, &fd_entry)) - m_should_block = false; + should_block = false; } - return m_should_block; + return should_block; } Thread::SelectBlocker::~SelectBlocker() @@ -385,15 +376,13 @@ Thread::SelectBlocker::~SelectBlocker() void Thread::SelectBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason) { - // Either the timeout was in the past or we didn't add all blockers - VERIFY(reason == UnblockImmediatelyReason::TimeoutInThePast || !m_should_block); SpinlockLocker lock(m_lock); - if (!m_should_block || !m_did_unblock) { - m_did_unblock = true; - if (reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet) { - auto count = collect_unblocked_flags(); - VERIFY(count > 0); - } + if (m_did_unblock) + return; + m_did_unblock = true; + if (reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet) { + auto count = collect_unblocked_flags(); + VERIFY(count > 0); } } @@ -617,7 +606,6 @@ Thread::WaitBlocker::WaitBlocker(int wait_options, idtype_t id_type, pid_t id, K , m_id_type(id_type) , m_waitee_id(id) , m_result(result) - , m_should_block(!(m_wait_options & WNOHANG)) { } @@ -658,9 +646,8 @@ bool Thread::WaitBlocker::setup_blocker() return add_to_blocker_set(Process::current().wait_blocker_set()); } -void Thread::WaitBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason) +void Thread::WaitBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason) { - VERIFY(reason == UnblockImmediatelyReason::TimeoutInThePast || !m_should_block); if (!m_error) Process::current().wait_blocker_set().try_unblock(*this); } |