1
Fork 0
mirror of https://github.com/RGBCube/serenity synced 2025-07-24 19:57:41 +00:00

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.
This commit is contained in:
Andreas Kling 2021-08-24 13:11:58 +02:00
parent adbf472ca7
commit 0c1d41cc8a
3 changed files with 26 additions and 51 deletions

View file

@ -52,7 +52,6 @@ private:
const IPv4Address m_ip_addr; const IPv4Address m_ip_addr;
Optional<MACAddress>& m_addr; Optional<MACAddress>& m_addr;
bool m_did_unblock { false }; bool m_did_unblock { false };
bool m_should_block { true };
}; };
class ARPTableBlockerSet final : public Thread::BlockerSet { class ARPTableBlockerSet final : public Thread::BlockerSet {
@ -90,14 +89,11 @@ ARPTableBlocker::ARPTableBlocker(IPv4Address ip_addr, Optional<MACAddress>& addr
bool ARPTableBlocker::setup_blocker() bool ARPTableBlocker::setup_blocker()
{ {
if (!add_to_blocker_set(*s_arp_table_blocker_set)) return add_to_blocker_set(*s_arp_table_blocker_set);
m_should_block = false;
return m_should_block;
} }
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 { auto addr = arp_table().with_shared([&](const auto& table) -> auto {
return table.get(ip_addr()); return table.get(ip_addr());
}); });

View file

@ -528,9 +528,7 @@ public:
NonnullRefPtr<Thread> m_joinee; NonnullRefPtr<Thread> m_joinee;
void*& m_joinee_exit_value; void*& m_joinee_exit_value;
KResult& m_try_join_result; KResult& m_try_join_result;
bool m_join_error { false };
bool m_did_unblock { false }; bool m_did_unblock { false };
bool m_should_block { true };
}; };
class WaitQueueBlocker final : public Blocker { class WaitQueueBlocker final : public Blocker {
@ -548,7 +546,6 @@ public:
protected: protected:
WaitQueue& m_wait_queue; WaitQueue& m_wait_queue;
StringView m_block_reason; StringView m_block_reason;
bool m_should_block { true };
bool m_did_unblock { false }; bool m_did_unblock { false };
}; };
@ -578,7 +575,6 @@ public:
FutexQueue& m_futex_queue; FutexQueue& m_futex_queue;
u32 m_bitset { 0 }; u32 m_bitset { 0 };
u32 m_relock_flags { 0 }; u32 m_relock_flags { 0 };
bool m_should_block { true };
bool m_did_unblock { false }; bool m_did_unblock { false };
}; };
@ -605,9 +601,6 @@ public:
virtual Type blocker_type() const override { return Type::File; } virtual Type blocker_type() const override { return Type::File; }
virtual bool unblock(bool, void*) = 0; virtual bool unblock(bool, void*) = 0;
protected:
bool m_should_block { true };
}; };
class FileDescriptionBlocker : public FileBlocker { class FileDescriptionBlocker : public FileBlocker {
@ -734,7 +727,6 @@ public:
bool m_did_unblock { false }; bool m_did_unblock { false };
bool m_error { false }; bool m_error { false };
bool m_got_sigchild { false }; bool m_got_sigchild { false };
bool m_should_block;
}; };
class WaitBlockerSet final : public BlockerSet { class WaitBlockerSet final : public BlockerSet {

View file

@ -80,28 +80,25 @@ bool Thread::JoinBlocker::setup_blocker()
// We need to hold our lock to avoid a race where try_join succeeds // We need to hold our lock to avoid a race where try_join succeeds
// but the joinee is joining immediately // but the joinee is joining immediately
SpinlockLocker lock(m_lock); SpinlockLocker lock(m_lock);
bool should_block = true;
m_try_join_result = m_joinee->try_join([&]() { m_try_join_result = m_joinee->try_join([&]() {
if (!add_to_blocker_set(m_joinee->m_join_blocker_set)) 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_try_join_result.is_error())
if (m_join_error) return false;
m_should_block = false; return should_block;
return m_should_block;
} }
void Thread::JoinBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason) 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 // 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 // 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 // to supply us the information. We cannot hold the lock as unblock
// could be called by the BlockerSet at any time! // could be called by the BlockerSet at any time!
VERIFY(reason == UnblockImmediatelyReason::TimeoutInThePast); if (reason == UnblockImmediatelyReason::TimeoutInThePast) {
m_joinee->m_join_blocker_set.try_unblock(*this); m_joinee->m_join_blocker_set.try_unblock(*this);
}
} }
bool Thread::JoinBlocker::unblock(void* value, bool from_add_blocker) 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() bool Thread::WaitQueueBlocker::setup_blocker()
{ {
if (!add_to_blocker_set(m_wait_queue)) return add_to_blocker_set(m_wait_queue);
m_should_block = false;
return m_should_block;
} }
Thread::WaitQueueBlocker::~WaitQueueBlocker() Thread::WaitQueueBlocker::~WaitQueueBlocker()
@ -158,9 +153,7 @@ Thread::FutexBlocker::FutexBlocker(FutexQueue& futex_queue, u32 bitset)
bool Thread::FutexBlocker::setup_blocker() bool Thread::FutexBlocker::setup_blocker()
{ {
if (!add_to_blocker_set(m_futex_queue)) return add_to_blocker_set(m_futex_queue);
m_should_block = false;
return m_should_block;
} }
Thread::FutexBlocker::~FutexBlocker() Thread::FutexBlocker::~FutexBlocker()
@ -212,9 +205,7 @@ Thread::FileDescriptionBlocker::FileDescriptionBlocker(FileDescription& descript
bool Thread::FileDescriptionBlocker::setup_blocker() bool Thread::FileDescriptionBlocker::setup_blocker()
{ {
m_unblocked_flags = BlockFlags::None; m_unblocked_flags = BlockFlags::None;
if (!add_to_blocker_set(m_blocked_description->blocker_set())) return add_to_blocker_set(m_blocked_description->blocker_set());
m_should_block = false;
return m_should_block;
} }
bool Thread::FileDescriptionBlocker::unblock(bool from_add_blocker, void*) 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) void Thread::FileDescriptionBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason)
{ {
if (!m_should_block) { if (reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet)
VERIFY(reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet);
return; return;
}
// If we should have blocked but got here it must have been that the // 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 // 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 // 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 Thread::SelectBlocker::setup_blocker()
{ {
bool should_block = true;
for (auto& fd_entry : m_fds) { for (auto& fd_entry : m_fds) {
fd_entry.unblocked_flags = FileBlocker::BlockFlags::None; fd_entry.unblocked_flags = FileBlocker::BlockFlags::None;
if (!m_should_block) if (!should_block)
continue; continue;
if (!fd_entry.description->blocker_set().add_blocker(*this, &fd_entry)) 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() Thread::SelectBlocker::~SelectBlocker()
@ -385,15 +376,13 @@ Thread::SelectBlocker::~SelectBlocker()
void Thread::SelectBlocker::will_unblock_immediately_without_blocking(UnblockImmediatelyReason reason) 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); SpinlockLocker lock(m_lock);
if (!m_should_block || !m_did_unblock) { if (m_did_unblock)
m_did_unblock = true; return;
if (reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet) { m_did_unblock = true;
auto count = collect_unblocked_flags(); if (reason == UnblockImmediatelyReason::UnblockConditionAlreadyMet) {
VERIFY(count > 0); 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_id_type(id_type)
, m_waitee_id(id) , m_waitee_id(id)
, m_result(result) , 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()); 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) if (!m_error)
Process::current().wait_blocker_set().try_unblock(*this); Process::current().wait_blocker_set().try_unblock(*this);
} }