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

Scheduler: Allow reentry into block()

With the presence of signal handlers, it is possible that a thread might
be blocked multiple times. Picture for instance a signal handler using
read(), or wait() while the thread is already blocked elsewhere before
the handler is invoked.

To fix this, we turn m_blocker into a chain of handlers. Each block()
call now prepends to the list, and unblocking will only consider the
most recent (first) blocker in the chain.

Fixes #309
This commit is contained in:
Robin Burchell 2019-07-21 12:14:58 +02:00 committed by Andreas Kling
parent a9db382f0e
commit dea7f937bf
3 changed files with 37 additions and 17 deletions

View file

@ -221,8 +221,8 @@ void Thread::consider_unblock(time_t now_sec, long now_usec)
/* don't know, don't care */ /* don't know, don't care */
return; return;
case Thread::Blocked: case Thread::Blocked:
ASSERT(m_blocker); ASSERT(!m_blockers.is_empty());
if (m_blocker->should_unblock(*this, now_sec, now_usec)) if (m_blockers.first()->should_unblock(*this, now_sec, now_usec))
unblock(); unblock();
return; return;
case Thread::Skip1SchedulerPass: case Thread::Skip1SchedulerPass:
@ -307,8 +307,8 @@ bool Scheduler::pick_next()
return IterationDecision::Continue; return IterationDecision::Continue;
if (was_blocked) { if (was_blocked) {
dbgprintf("Unblock %s(%u) due to signal\n", thread.process().name().characters(), thread.pid()); dbgprintf("Unblock %s(%u) due to signal\n", thread.process().name().characters(), thread.pid());
ASSERT(thread.m_blocker); ASSERT(!thread.m_blockers.is_empty());
thread.m_blocker->set_interrupted_by_signal(); thread.m_blockers.first()->set_interrupted_by_signal();
thread.unblock(); thread.unblock();
} }
return IterationDecision::Continue; return IterationDecision::Continue;

View file

@ -148,8 +148,8 @@ const char* Thread::state_string() const
case Thread::Skip0SchedulerPasses: case Thread::Skip0SchedulerPasses:
return "Skip0"; return "Skip0";
case Thread::Blocked: case Thread::Blocked:
ASSERT(m_blocker); ASSERT(!m_blockers.is_empty());
return m_blocker->state_string(); return m_blockers.first()->state_string();
} }
kprintf("to_string(Thread::State): Invalid state: %u\n", state()); kprintf("to_string(Thread::State): Invalid state: %u\n", state());
ASSERT_NOT_REACHED(); ASSERT_NOT_REACHED();
@ -539,8 +539,8 @@ void Thread::set_state(State new_state)
{ {
InterruptDisabler disabler; InterruptDisabler disabler;
if (new_state == Blocked) { if (new_state == Blocked) {
// we should always have an m_blocker while blocked // we should always have a Blocker while blocked
ASSERT(m_blocker != nullptr); ASSERT(!m_blockers.is_empty());
} }
m_state = new_state; m_state = new_state;

View file

@ -72,6 +72,8 @@ public:
bool was_interrupted_by_signal() const { return m_was_interrupted_while_blocked; } bool was_interrupted_by_signal() const { return m_was_interrupted_while_blocked; }
private: private:
bool m_was_interrupted_while_blocked { false }; bool m_was_interrupted_while_blocked { false };
friend class Thread;
IntrusiveListNode m_blocker_list_node;
}; };
class FileDescriptionBlocker : public Blocker { class FileDescriptionBlocker : public Blocker {
@ -217,21 +219,39 @@ public:
template <typename T, class... Args> template <typename T, class... Args>
[[nodiscard]] BlockResult block(Args&& ... args) [[nodiscard]] BlockResult block(Args&& ... args)
{ {
// If this is triggered, state has gotten messed: we should never have a
// blocker already set. That means we're re-entering somehow, which is
// bad.
ASSERT(m_blocker == nullptr);
// We should never be blocking a blocked (or otherwise non-active) thread. // We should never be blocking a blocked (or otherwise non-active) thread.
ASSERT(state() == Thread::Running); ASSERT(state() == Thread::Running);
T t(AK::forward<Args>(args)...); T t(AK::forward<Args>(args)...);
m_blocker = &t; m_blockers.prepend(t);
// Enter blocked state.
set_state(Thread::Blocked); set_state(Thread::Blocked);
block_helper(); // this will unlock, yield, and eventually unblock us to return here.
// Yield to the scheduler, and wait for us to resume unblocked.
block_helper();
// We should no longer be blocked once we woke up
ASSERT(state() != Thread::Blocked); ASSERT(state() != Thread::Blocked);
m_blocker = nullptr;
// We should be the first blocker, otherwise we're waking up in a
// different order than we are waiting: scheduler bug?
ASSERT(m_blockers.first() == &t);
// Remove ourselves...
m_blockers.remove(t);
// If there are still pending blockers that need to be waited for, then
// set state back to Blocked, for the next one to be handled.
// Otherwise, we're good now, and done with blocking state.
if (!m_blockers.is_empty()) {
if (!m_blockers.first()->was_interrupted_by_signal())
set_state(Thread::Blocked);
}
if (t.was_interrupted_by_signal()) if (t.was_interrupted_by_signal())
return BlockResult::InterruptedBySignal; return BlockResult::InterruptedBySignal;
return BlockResult::WokeNormally; return BlockResult::WokeNormally;
}; };
@ -321,7 +341,7 @@ private:
RefPtr<Region> m_kernel_stack_for_signal_handler_region; RefPtr<Region> m_kernel_stack_for_signal_handler_region;
SignalActionData m_signal_action_data[32]; SignalActionData m_signal_action_data[32];
Region* m_signal_stack_user_region { nullptr }; Region* m_signal_stack_user_region { nullptr };
Blocker* m_blocker { nullptr }; IntrusiveList<Blocker, &Blocker::m_blocker_list_node> m_blockers;
FPUState* m_fpu_state { nullptr }; FPUState* m_fpu_state { nullptr };
State m_state { Invalid }; State m_state { Invalid };
bool m_has_used_fpu { false }; bool m_has_used_fpu { false };