From 934b1d8a9ba9d86f9540cc15f881d67e705f27bd Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sat, 1 Feb 2020 10:27:25 +0100 Subject: [PATCH] Kernel: Finalizer should not go back to sleep if there's more to do Before putting itself back on the wait queue, the finalizer task will now check if there's more work to do, and if so, do it first. :^) This patch also puts a bunch of process/thread debug logging behind PROCESS_DEBUG and THREAD_DEBUG since it was unbearable to debug this stuff with all the spam. --- Kernel/Process.cpp | 10 ++++++++++ Kernel/Scheduler.cpp | 2 ++ Kernel/Scheduler.h | 1 + Kernel/Thread.cpp | 19 ++++++++++++++++--- Kernel/init.cpp | 8 +++++++- 5 files changed, 36 insertions(+), 4 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 644de985008..acac5fb086d 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -73,6 +73,7 @@ #include #include +//#define PROCESS_DEBUG //#define DEBUG_POLL_SELECT //#define DEBUG_IO //#define TASK_DEBUG @@ -1295,7 +1296,9 @@ Process::Process(Thread*& first_thread, const String& name, uid_t uid, gid_t gid , m_tty(tty) , m_ppid(ppid) { +#ifdef PROCESS_DEBUG dbg() << "Created new process " << m_name << "(" << m_pid << ")"; +#endif m_page_directory = PageDirectory::create_for_userspace(*this, fork_parent ? &fork_parent->page_directory().range_allocator() : nullptr); #ifdef MM_DEBUG @@ -2278,7 +2281,9 @@ int Process::reap(Process& process) } } +#ifdef PROCESS_DEBUG dbg() << "Reaping process " << process; +#endif ASSERT(process.is_dead()); g_processes->remove(&process); } @@ -2289,7 +2294,10 @@ int Process::reap(Process& process) pid_t Process::sys$waitpid(pid_t waitee, int* wstatus, int options) { REQUIRE_PROMISE(stdio); + +#ifdef PROCESS_DEBUG dbg() << "sys$waitpid(" << waitee << ", " << wstatus << ", " << options << ")"; +#endif if (!options) { // FIXME: This can't be right.. can it? Figure out how this should actually work. @@ -2928,7 +2936,9 @@ int Process::sys$chown(const Syscall::SC_chown_params* user_params) void Process::finalize() { ASSERT(current == g_finalizer); +#ifdef PROCESS_DEBUG dbg() << "Finalizing process " << *this; +#endif m_fds.clear(); m_tty = nullptr; diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp index d5a6ff9314f..2ef7bf728b2 100644 --- a/Kernel/Scheduler.cpp +++ b/Kernel/Scheduler.cpp @@ -68,6 +68,7 @@ Thread* current; Thread* g_finalizer; Thread* g_colonel; WaitQueue* g_finalizer_wait_queue; +bool g_finalizer_has_work; static Process* s_colonel_process; u64 g_uptime; @@ -578,6 +579,7 @@ void Scheduler::initialize() { g_scheduler_data = new SchedulerData; g_finalizer_wait_queue = new WaitQueue; + g_finalizer_has_work = false; s_redirection.selector = gdt_alloc_entry(); initialize_redirection(); s_colonel_process = Process::create_kernel_process(g_colonel, "colonel", nullptr); diff --git a/Kernel/Scheduler.h b/Kernel/Scheduler.h index 66a7a0909ae..826b31c0173 100644 --- a/Kernel/Scheduler.h +++ b/Kernel/Scheduler.h @@ -41,6 +41,7 @@ extern Thread* current; extern Thread* g_finalizer; extern Thread* g_colonel; extern WaitQueue* g_finalizer_wait_queue; +extern bool g_finalizer_has_work; extern u64 g_uptime; extern SchedulerData* g_scheduler_data; diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 20bb8878a4c..840a65ed35c 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -37,6 +37,7 @@ #include //#define SIGNAL_DEBUG +//#define THREAD_DEBUG static FPUState s_clean_fpu_state; @@ -82,7 +83,9 @@ Thread::Thread(Process& process) m_tid = Process::allocate_pid(); } process.m_thread_count++; +#ifdef THREAD_DEBUG dbg() << "Created new thread " << process.name() << "(" << process.pid() << ":" << m_tid << ")"; +#endif set_default_signal_dispositions(); m_fpu_state = (FPUState*)kmalloc_aligned(sizeof(FPUState), 16); memcpy(m_fpu_state, &s_clean_fpu_state, sizeof(FPUState)); @@ -163,7 +166,9 @@ void Thread::unblock() void Thread::set_should_die() { if (m_should_die) { - dbgprintf("Should already die (%u)\n", m_tid); +#ifdef THREAD_DEBUG + dbg() << *this << " Should already die"; +#endif return; } InterruptDisabler disabler; @@ -275,7 +280,9 @@ void Thread::finalize() { ASSERT(current == g_finalizer); +#ifdef THREAD_DEBUG dbg() << "Finalizing thread " << *this; +#endif set_state(Thread::State::Dead); if (m_joiner) { @@ -320,21 +327,25 @@ bool Thread::tick() return --m_ticks_left; } -void Thread::send_signal(u8 signal, Process* sender) +void Thread::send_signal(u8 signal, [[maybe_unused]] Process* sender) { ASSERT(signal < 32); InterruptDisabler disabler; // FIXME: Figure out what to do for masked signals. Should we also ignore them here? if (should_ignore_signal(signal)) { +#ifdef SIGNAL_DEBUG dbg() << "signal " << signal << " was ignored by " << process(); +#endif return; } +#ifdef SIGNAL_DEBUG if (sender) dbgprintf("signal: %s(%u) sent %d to %s(%u)\n", sender->name().characters(), sender->pid(), signal, process().name().characters(), pid()); else dbgprintf("signal: kernel sent %d to %s(%u)\n", signal, process().name().characters(), pid()); +#endif m_pending_signals |= 1 << (signal - 1); } @@ -707,8 +718,10 @@ void Thread::set_state(State new_state) Scheduler::update_state_for_thread(*this); } - if (new_state == Dying) + if (new_state == Dying) { + g_finalizer_has_work = true; g_finalizer_wait_queue->wake_all(); + } } String Thread::backtrace(ProcessInspectionHandle&) const diff --git a/Kernel/init.cpp b/Kernel/init.cpp index 2f69a6e5768..d4c04d6e8a4 100644 --- a/Kernel/init.cpp +++ b/Kernel/init.cpp @@ -189,7 +189,13 @@ extern "C" [[noreturn]] void init() Process::create_kernel_process(g_finalizer, "Finalizer", [] { current->set_priority(THREAD_PRIORITY_LOW); for (;;) { - current->wait_on(*g_finalizer_wait_queue); + { + InterruptDisabler disabler; + if (!g_finalizer_has_work) + current->wait_on(*g_finalizer_wait_queue); + ASSERT(g_finalizer_has_work); + g_finalizer_has_work = false; + } Thread::finalize_dying_threads(); } });