From 838d9fa251ed34289cb9c77eb46f889dc9e79416 Mon Sep 17 00:00:00 2001 From: Tom Date: Sun, 27 Sep 2020 08:53:35 -0600 Subject: [PATCH] Kernel: Make Thread refcounted Similar to Process, we need to make Thread refcounted. This will solve problems that will appear once we schedule threads on more than one processor. This allows us to hold onto threads without necessarily holding the scheduler lock for the entire duration. --- Kernel/Net/NetworkTask.cpp | 2 +- Kernel/Process.cpp | 19 +++++++------ Kernel/Process.h | 8 +++--- Kernel/Scheduler.cpp | 8 +++--- Kernel/Scheduler.h | 2 +- Kernel/Syscalls/fork.cpp | 7 +++-- Kernel/Syscalls/sched.cpp | 51 ++++++++++++++++++++-------------- Kernel/Syscalls/thread.cpp | 28 +++++++------------ Kernel/Syscalls/waitid.cpp | 4 ++- Kernel/Tasks/FinalizerTask.cpp | 4 ++- Kernel/Tasks/SyncTask.cpp | 2 +- Kernel/Thread.cpp | 48 ++++++++++++++++++++------------ Kernel/Thread.h | 32 ++++++++++++++++----- Kernel/init.cpp | 11 ++++++-- 14 files changed, 136 insertions(+), 90 deletions(-) diff --git a/Kernel/Net/NetworkTask.cpp b/Kernel/Net/NetworkTask.cpp index ff2816ea12e..913ad877c2f 100644 --- a/Kernel/Net/NetworkTask.cpp +++ b/Kernel/Net/NetworkTask.cpp @@ -61,7 +61,7 @@ static void handle_tcp(const IPv4Packet&, const timeval& packet_timestamp); void NetworkTask::spawn() { - Thread* thread = nullptr; + RefPtr thread; Process::create_kernel_process(thread, "NetworkTask", NetworkTask_main); } diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index f9d6fe4bb00..2efa041f9c9 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -265,7 +265,7 @@ void Process::kill_all_threads() }); } -RefPtr Process::create_user_process(Thread*& first_thread, const String& path, uid_t uid, gid_t gid, ProcessID parent_pid, int& error, Vector&& arguments, Vector&& environment, TTY* tty) +RefPtr Process::create_user_process(RefPtr& first_thread, const String& path, uid_t uid, gid_t gid, ProcessID parent_pid, int& error, Vector&& arguments, Vector&& environment, TTY* tty) { auto parts = path.split('/'); if (arguments.is_empty()) { @@ -298,7 +298,7 @@ RefPtr Process::create_user_process(Thread*& first_thread, const String error = process->exec(path, move(arguments), move(environment)); if (error != 0) { dbg() << "Failed to exec " << path << ": " << error; - delete first_thread; + first_thread = nullptr; return {}; } @@ -311,7 +311,7 @@ RefPtr Process::create_user_process(Thread*& first_thread, const String return process; } -NonnullRefPtr Process::create_kernel_process(Thread*& first_thread, String&& name, void (*e)(), u32 affinity) +NonnullRefPtr Process::create_kernel_process(RefPtr& first_thread, String&& name, void (*e)(), u32 affinity) { auto process = adopt(*new Process(first_thread, move(name), (uid_t)0, (gid_t)0, ProcessID(0), true)); first_thread->tss().eip = (FlatPtr)e; @@ -327,7 +327,7 @@ NonnullRefPtr Process::create_kernel_process(Thread*& first_thread, Str return process; } -Process::Process(Thread*& first_thread, const String& name, uid_t uid, gid_t gid, ProcessID ppid, bool is_kernel_process, RefPtr cwd, RefPtr executable, TTY* tty, Process* fork_parent) +Process::Process(RefPtr& first_thread, const String& name, uid_t uid, gid_t gid, ProcessID ppid, bool is_kernel_process, RefPtr cwd, RefPtr executable, TTY* tty, Process* fork_parent) : m_name(move(name)) , m_pid(allocate_pid()) , m_euid(uid) @@ -356,14 +356,15 @@ Process::Process(Thread*& first_thread, const String& name, uid_t uid, gid_t gid first_thread = Thread::current()->clone(*this); } else { // NOTE: This non-forked code path is only taken when the kernel creates a process "manually" (at boot.) - first_thread = new Thread(*this); + first_thread = adopt(*new Thread(*this)); first_thread->detach(); } } Process::~Process() { - ASSERT(thread_count() == 0); + ASSERT(!m_next && !m_prev); // should have been reaped + ASSERT(thread_count() == 0); // all threads should have been finalized } void Process::dump_regions() @@ -613,7 +614,7 @@ void Process::finalize() { InterruptDisabler disabler; // FIXME: PID/TID BUG - if (auto* parent_thread = Thread::from_tid(m_ppid.value())) { + if (auto parent_thread = Thread::from_tid(m_ppid.value())) { if (parent_thread->m_signal_action_data[SIGCHLD].flags & SA_NOCLDWAIT) { // NOTE: If the parent doesn't care about this process, let it go. m_ppid = 0; @@ -761,13 +762,13 @@ KResult Process::send_signal(u8 signal, Process* sender) return KResult(-ESRCH); } -Thread* Process::create_kernel_thread(void (*entry)(), u32 priority, const String& name, u32 affinity, bool joinable) +RefPtr Process::create_kernel_thread(void (*entry)(), u32 priority, const String& name, u32 affinity, bool joinable) { ASSERT((priority >= THREAD_PRIORITY_MIN) && (priority <= THREAD_PRIORITY_MAX)); // FIXME: Do something with guard pages? - auto* thread = new Thread(*this); + auto thread = adopt(*new Thread(*this)); thread->set_name(name); thread->set_affinity(affinity); diff --git a/Kernel/Process.h b/Kernel/Process.h index d84af27f699..743d654e8bb 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -125,14 +125,14 @@ public: return current_thread ? ¤t_thread->process() : nullptr; } - static NonnullRefPtr create_kernel_process(Thread*& first_thread, String&& name, void (*entry)(), u32 affinity = THREAD_AFFINITY_DEFAULT); - static RefPtr create_user_process(Thread*& first_thread, const String& path, uid_t, gid_t, ProcessID ppid, int& error, Vector&& arguments = Vector(), Vector&& environment = Vector(), TTY* = nullptr); + static NonnullRefPtr create_kernel_process(RefPtr& first_thread, String&& name, void (*entry)(), u32 affinity = THREAD_AFFINITY_DEFAULT); + static RefPtr create_user_process(RefPtr& first_thread, const String& path, uid_t, gid_t, ProcessID ppid, int& error, Vector&& arguments = Vector(), Vector&& environment = Vector(), TTY* = nullptr); ~Process(); static Vector all_pids(); static AK::NonnullRefPtrVector all_processes(); - Thread* create_kernel_thread(void (*entry)(), u32 priority, const String& name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true); + RefPtr create_kernel_thread(void (*entry)(), u32 priority, const String& name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true); bool is_profiling() const { return m_profiling; } void set_profiling(bool profiling) { m_profiling = profiling; } @@ -466,7 +466,7 @@ private: friend class Scheduler; friend class Region; - Process(Thread*& first_thread, const String& name, uid_t, gid_t, ProcessID ppid, bool is_kernel_process, RefPtr cwd = nullptr, RefPtr executable = nullptr, TTY* = nullptr, Process* fork_parent = nullptr); + Process(RefPtr& first_thread, const String& name, uid_t, gid_t, ProcessID ppid, bool is_kernel_process, RefPtr cwd = nullptr, RefPtr executable = nullptr, TTY* = nullptr, Process* fork_parent = nullptr); static ProcessID allocate_pid(); Range allocate_range(VirtualAddress, size_t, size_t alignment = PAGE_SIZE); diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp index 6a4c3236fba..2a48537b598 100644 --- a/Kernel/Scheduler.cpp +++ b/Kernel/Scheduler.cpp @@ -50,7 +50,7 @@ class SchedulerPerProcessorData { public: SchedulerPerProcessorData() = default; - Thread* m_pending_beneficiary { nullptr }; + WeakPtr m_pending_beneficiary; const char* m_pending_donate_reason { nullptr }; bool m_in_scheduler { true }; }; @@ -599,7 +599,7 @@ bool Scheduler::donate_to_and_switch(Thread* beneficiary, const char* reason) return Scheduler::context_switch(beneficiary); } -bool Scheduler::donate_to(Thread* beneficiary, const char* reason) +bool Scheduler::donate_to(RefPtr& beneficiary, const char* reason) { ASSERT(beneficiary); @@ -625,7 +625,7 @@ bool Scheduler::donate_to(Thread* beneficiary, const char* reason) ASSERT(!proc.in_irq()); if (proc.in_critical() > 1) { - scheduler_data.m_pending_beneficiary = beneficiary; // Save the beneficiary + scheduler_data.m_pending_beneficiary = beneficiary->make_weak_ptr(); // Save the beneficiary scheduler_data.m_pending_donate_reason = reason; proc.invoke_scheduler_async(); return false; @@ -740,7 +740,7 @@ void Scheduler::initialize() { ASSERT(&Processor::current() != nullptr); // sanity check - Thread* idle_thread = nullptr; + RefPtr idle_thread; g_scheduler_data = new SchedulerData; g_finalizer_wait_queue = new WaitQueue; diff --git a/Kernel/Scheduler.h b/Kernel/Scheduler.h index e4b2184ce53..cfe27171915 100644 --- a/Kernel/Scheduler.h +++ b/Kernel/Scheduler.h @@ -60,7 +60,7 @@ public: static timeval time_since_boot(); static bool yield(); static bool donate_to_and_switch(Thread*, const char* reason); - static bool donate_to(Thread*, const char* reason); + static bool donate_to(RefPtr&, const char* reason); static bool context_switch(Thread*); static void enter_current(Thread& prev_thread); static void leave_on_first_switch(u32 flags); diff --git a/Kernel/Syscalls/fork.cpp b/Kernel/Syscalls/fork.cpp index d6ca53daf43..c4883911798 100644 --- a/Kernel/Syscalls/fork.cpp +++ b/Kernel/Syscalls/fork.cpp @@ -36,8 +36,10 @@ namespace Kernel { pid_t Process::sys$fork(RegisterState& regs) { REQUIRE_PROMISE(proc); - Thread* child_first_thread = nullptr; - auto* child = new Process(child_first_thread, m_name, m_uid, m_gid, m_pid, m_is_kernel_process, m_cwd, m_executable, m_tty, this); + RefPtr child_first_thread; + auto child = adopt(*new Process(child_first_thread, m_name, m_uid, m_gid, m_pid, m_is_kernel_process, m_cwd, m_executable, m_tty, this)); + if (!child_first_thread) + return -ENOMEM; child->m_root_directory = m_root_directory; child->m_root_directory_relative_to_global_root = m_root_directory_relative_to_global_root; child->m_promises = m_promises; @@ -92,6 +94,7 @@ pid_t Process::sys$fork(RegisterState& regs) { ScopedSpinLock lock(g_processes_lock); g_processes->prepend(child); + child->ref(); // This reference will be dropped by Process::reap } child_first_thread->set_affinity(Thread::current()->affinity()); diff --git a/Kernel/Syscalls/sched.cpp b/Kernel/Syscalls/sched.cpp index 91ca08884eb..8882a0c627d 100644 --- a/Kernel/Syscalls/sched.cpp +++ b/Kernel/Syscalls/sched.cpp @@ -40,8 +40,12 @@ int Process::sys$donate(pid_t tid) REQUIRE_PROMISE(stdio); if (tid < 0) return -EINVAL; - InterruptDisabler disabler; - auto* thread = Thread::from_tid(tid); + + // We don't strictly need to grab the scheduler lock here, but it + // will close a race where we can find the thread but it disappears + // before we call Scheduler::donate_to. + ScopedSpinLock lock(g_scheduler_lock); + auto thread = Thread::from_tid(tid); if (!thread || thread->pid() != pid()) return -ESRCH; Scheduler::donate_to(thread, "sys$donate"); @@ -55,8 +59,11 @@ int Process::sys$sched_setparam(int pid, Userspace us if (!copy_from_user(&desired_param, user_param)) return -EFAULT; - InterruptDisabler disabler; + if (desired_param.sched_priority < THREAD_PRIORITY_MIN || desired_param.sched_priority > THREAD_PRIORITY_MAX) + return -EINVAL; + auto* peer = Thread::current(); + ScopedSpinLock lock(g_scheduler_lock); if (pid != 0) peer = Thread::from_tid(pid); @@ -66,9 +73,6 @@ int Process::sys$sched_setparam(int pid, Userspace us if (!is_superuser() && m_euid != peer->process().m_uid && m_uid != peer->process().m_uid) return -EPERM; - if (desired_param.sched_priority < THREAD_PRIORITY_MIN || desired_param.sched_priority > THREAD_PRIORITY_MAX) - return -EINVAL; - peer->set_priority((u32)desired_param.sched_priority); return 0; } @@ -76,22 +80,27 @@ int Process::sys$sched_setparam(int pid, Userspace us int Process::sys$sched_getparam(pid_t pid, Userspace user_param) { REQUIRE_PROMISE(proc); - InterruptDisabler disabler; - auto* peer = Thread::current(); - if (pid != 0) { - // FIXME: PID/TID BUG - // The entire process is supposed to be affected. - peer = Thread::from_tid(pid); + int priority; + { + auto* peer = Thread::current(); + ScopedSpinLock lock(g_scheduler_lock); + if (pid != 0) { + // FIXME: PID/TID BUG + // The entire process is supposed to be affected. + peer = Thread::from_tid(pid); + } + + if (!peer) + return -ESRCH; + + if (!is_superuser() && m_euid != peer->process().m_uid && m_uid != peer->process().m_uid) + return -EPERM; + + priority = (int)peer->priority(); } - if (!peer) - return -ESRCH; - - if (!is_superuser() && m_euid != peer->process().m_uid && m_uid != peer->process().m_uid) - return -EPERM; - struct sched_param param { - (int)peer->priority() + priority }; if (!copy_to_user(user_param, ¶m)) return -EFAULT; @@ -103,8 +112,8 @@ int Process::sys$set_thread_boost(pid_t tid, int amount) REQUIRE_PROMISE(proc); if (amount < 0 || amount > 20) return -EINVAL; - InterruptDisabler disabler; - auto* thread = Thread::from_tid(tid); + ScopedSpinLock lock(g_scheduler_lock); + auto thread = Thread::from_tid(tid); if (!thread) return -ESRCH; if (thread->state() == Thread::State::Dead || thread->state() == Thread::State::Dying) diff --git a/Kernel/Syscalls/thread.cpp b/Kernel/Syscalls/thread.cpp index bd957da5df2..88b05b6aa5d 100644 --- a/Kernel/Syscalls/thread.cpp +++ b/Kernel/Syscalls/thread.cpp @@ -102,8 +102,7 @@ void Process::sys$exit_thread(Userspace exit_value) int Process::sys$detach_thread(pid_t tid) { REQUIRE_PROMISE(thread); - InterruptDisabler disabler; - auto* thread = Thread::from_tid(tid); + auto thread = Thread::from_tid(tid); if (!thread || thread->pid() != pid()) return -ESRCH; @@ -118,8 +117,7 @@ int Process::sys$join_thread(pid_t tid, Userspace exit_value) { REQUIRE_PROMISE(thread); - InterruptDisabler disabler; - auto* thread = Thread::from_tid(tid); + auto thread = Thread::from_tid(tid); if (!thread || thread->pid() != pid()) return -ESRCH; @@ -134,20 +132,14 @@ int Process::sys$join_thread(pid_t tid, Userspace exit_value) KResult try_join_result(KSuccess); auto result = current_thread->block(nullptr, *thread, try_join_result, joinee_exit_value); if (result == Thread::BlockResult::NotBlocked) { - ASSERT_INTERRUPTS_DISABLED(); if (try_join_result.is_error()) return try_join_result.error(); break; } - if (result == Thread::BlockResult::InterruptedByDeath) { - ASSERT_INTERRUPTS_DISABLED(); - break; - } + if (result == Thread::BlockResult::InterruptedByDeath) + return 0; // we're not going to return back to user mode } - // NOTE: 'thread' is very possibly deleted at this point. Clear it just to be safe. - thread = nullptr; - if (exit_value && !copy_to_user(exit_value, &joinee_exit_value)) return -EFAULT; return 0; @@ -164,8 +156,7 @@ int Process::sys$set_thread_name(pid_t tid, Userspace user_name, si if (name.length() > max_thread_name_size) return -EINVAL; - InterruptDisabler disabler; - auto* thread = Thread::from_tid(tid); + auto thread = Thread::from_tid(tid); if (!thread || thread->pid() != pid()) return -ESRCH; @@ -179,15 +170,16 @@ int Process::sys$get_thread_name(pid_t tid, Userspace buffer, size_t buff if (buffer_size == 0) return -EINVAL; - InterruptDisabler disabler; - auto* thread = Thread::from_tid(tid); + auto thread = Thread::from_tid(tid); if (!thread || thread->pid() != pid()) return -ESRCH; - if (thread->name().length() + 1 > (size_t)buffer_size) + // We must make a temporary copy here to avoid a race with sys$set_thread_name + auto thread_name = thread->name(); + if (thread_name.length() + 1 > (size_t)buffer_size) return -ENAMETOOLONG; - if (!copy_to_user(buffer, thread->name().characters(), thread->name().length() + 1)) + if (!copy_to_user(buffer, thread_name.characters(), thread_name.length() + 1)) return -EFAULT; return 0; } diff --git a/Kernel/Syscalls/waitid.cpp b/Kernel/Syscalls/waitid.cpp index f4d6f9c7ad2..9e75ba376e6 100644 --- a/Kernel/Syscalls/waitid.cpp +++ b/Kernel/Syscalls/waitid.cpp @@ -68,7 +68,9 @@ KResultOr Process::do_waitid(idtype_t idtype, int id, int options) return reap(*waitee_process); } else { // FIXME: PID/TID BUG - auto* waitee_thread = Thread::from_tid(waitee_pid.value()); + // Make sure to hold the scheduler lock so that we operate on a consistent state + ScopedSpinLock scheduler_lock(g_scheduler_lock); + auto waitee_thread = Thread::from_tid(waitee_pid.value()); if (!waitee_thread) return KResult(-ECHILD); ASSERT((options & WNOHANG) || waitee_thread->state() == Thread::State::Stopped); diff --git a/Kernel/Tasks/FinalizerTask.cpp b/Kernel/Tasks/FinalizerTask.cpp index c591a2bc841..fe82f84ab9f 100644 --- a/Kernel/Tasks/FinalizerTask.cpp +++ b/Kernel/Tasks/FinalizerTask.cpp @@ -31,7 +31,8 @@ namespace Kernel { void FinalizerTask::spawn() { - Process::create_kernel_process(g_finalizer, "FinalizerTask", [] { + RefPtr finalizer_thread; + Process::create_kernel_process(finalizer_thread, "FinalizerTask", [] { Thread::current()->set_priority(THREAD_PRIORITY_LOW); for (;;) { Thread::current()->wait_on(*g_finalizer_wait_queue, "FinalizerTask"); @@ -41,6 +42,7 @@ void FinalizerTask::spawn() Thread::finalize_dying_threads(); } }); + g_finalizer = finalizer_thread; } } diff --git a/Kernel/Tasks/SyncTask.cpp b/Kernel/Tasks/SyncTask.cpp index aea99872f1f..58c9e3c9332 100644 --- a/Kernel/Tasks/SyncTask.cpp +++ b/Kernel/Tasks/SyncTask.cpp @@ -33,7 +33,7 @@ namespace Kernel { void SyncTask::spawn() { - Thread* syncd_thread = nullptr; + RefPtr syncd_thread; Process::create_kernel_process(syncd_thread, "SyncTask", [] { dbg() << "SyncTask is running"; for (;;) { diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index caa53001335..43ebc169cea 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -100,17 +100,20 @@ Thread::Thread(NonnullRefPtr process) m_tss.esp0 = m_kernel_stack_top; } + // We need to add another reference if we could successfully create + // all the resources needed for this thread. The reason for this is that + // we don't want to delete this thread after dropping the reference, + // it may still be running or scheduled to be run. + // The finalizer is responsible for dropping this reference once this + // thread is ready to be cleaned up. + ref(); + if (m_process->pid() != 0) Scheduler::init_thread(*this); } Thread::~Thread() { - kfree_aligned(m_fpu_state); - - auto thread_cnt_before = m_process->m_thread_count.fetch_sub(1, AK::MemoryOrder::memory_order_acq_rel); - ASSERT(thread_cnt_before != 0); - ASSERT(!m_joiner); } @@ -269,10 +272,12 @@ const char* Thread::state_string() const return "Stopped"; case Thread::Queued: return "Queued"; - case Thread::Blocked: + case Thread::Blocked: { + ScopedSpinLock lock(m_lock); ASSERT(m_blocker != nullptr); return m_blocker->state_string(); } + } klog() << "Thread::state_string(): Invalid state: " << state(); ASSERT_NOT_REACHED(); return nullptr; @@ -295,6 +300,13 @@ void Thread::finalize() if (m_dump_backtrace_on_finalization) dbg() << backtrace_impl(); + + kfree_aligned(m_fpu_state); + + auto thread_cnt_before = m_process->m_thread_count.fetch_sub(1, AK::MemoryOrder::memory_order_acq_rel); + ASSERT(thread_cnt_before != 0); + if (thread_cnt_before == 1) + process().finalize(); } void Thread::finalize_dying_threads() @@ -310,11 +322,12 @@ void Thread::finalize_dying_threads() }); } for (auto* thread : dying_threads) { - auto& process = thread->process(); thread->finalize(); - delete thread; - if (process.m_thread_count.load(AK::MemoryOrder::memory_order_consume) == 0) - process.finalize(); + + // This thread will never execute again, drop the running reference + // NOTE: This may not necessarily drop the last reference if anything + // else is still holding onto this thread! + thread->unref(); } } @@ -762,9 +775,9 @@ KResultOr Thread::make_userspace_stack_for_main_thread(Vector argum return new_esp; } -Thread* Thread::clone(Process& process) +RefPtr Thread::clone(Process& process) { - auto* clone = new Thread(process); + auto clone = adopt(*new Thread(process)); memcpy(clone->m_signal_action_data, m_signal_action_data, sizeof(m_signal_action_data)); clone->m_signal_mask = m_signal_mask; memcpy(clone->m_fpu_state, m_fpu_state, sizeof(FPUState)); @@ -967,7 +980,7 @@ const LogStream& operator<<(const LogStream& stream, const Thread& value) return stream << value.process().name() << "(" << value.pid().value() << ":" << value.tid().value() << ")"; } -Thread::BlockResult Thread::wait_on(WaitQueue& queue, const char* reason, timeval* timeout, Atomic* lock, Thread* beneficiary) +Thread::BlockResult Thread::wait_on(WaitQueue& queue, const char* reason, timeval* timeout, Atomic* lock, RefPtr beneficiary) { auto* current_thread = Thread::current(); TimerId timer_id {}; @@ -1046,7 +1059,7 @@ Thread::BlockResult Thread::wait_on(WaitQueue& queue, const char* reason, timeva // Our thread was already removed from the queue. The only // way this can happen if someone else is trying to kill us. // In this case, the queue should not contain us anymore. - return BlockResult::InterruptedByDeath; + result = BlockResult::InterruptedByDeath; } // Make sure we cancel the timer if woke normally. @@ -1071,10 +1084,10 @@ void Thread::wake_from_queue() set_state(State::Running); } -Thread* Thread::from_tid(ThreadID tid) +RefPtr Thread::from_tid(ThreadID tid) { - InterruptDisabler disabler; - Thread* found_thread = nullptr; + RefPtr found_thread; + ScopedSpinLock lock(g_scheduler_lock); Thread::for_each([&](auto& thread) { if (thread.tid() == tid) { found_thread = &thread; @@ -1109,6 +1122,7 @@ void Thread::tracer_trap(const RegisterState& regs) const Thread::Blocker& Thread::blocker() const { + ASSERT(m_lock.own_lock()); ASSERT(m_blocker); return *m_blocker; } diff --git a/Kernel/Thread.h b/Kernel/Thread.h index d5cd017614f..57dc4cceb49 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -32,6 +32,8 @@ #include #include #include +#include +#include #include #include #include @@ -66,7 +68,9 @@ struct ThreadSpecificData { #define THREAD_AFFINITY_DEFAULT 0xffffffff -class Thread { +class Thread + : public RefCounted + , public Weakable { AK_MAKE_NONCOPYABLE(Thread); AK_MAKE_NONMOVABLE(Thread); @@ -82,7 +86,7 @@ public: explicit Thread(NonnullRefPtr); ~Thread(); - static Thread* from_tid(ThreadID); + static RefPtr from_tid(ThreadID); static void finalize_dying_threads(); ThreadID tid() const { return m_tid; } @@ -143,9 +147,23 @@ public: String backtrace(); Vector raw_backtrace(FlatPtr ebp, FlatPtr eip) const; - const String& name() const { return m_name; } - void set_name(const StringView& s) { m_name = s; } - void set_name(String&& name) { m_name = move(name); } + String name() const + { + // Because the name can be changed, we can't return a const + // reference here. We must make a copy + ScopedSpinLock lock(m_lock); + return m_name; + } + void set_name(const StringView& s) + { + ScopedSpinLock lock(m_lock); + m_name = s; + } + void set_name(String&& name) + { + ScopedSpinLock lock(m_lock); + m_name = move(name); + } void finalize(); @@ -452,7 +470,7 @@ public: return block(nullptr, state_string, move(condition)); } - BlockResult wait_on(WaitQueue& queue, const char* reason, timeval* timeout = nullptr, Atomic* lock = nullptr, Thread* beneficiary = nullptr); + BlockResult wait_on(WaitQueue& queue, const char* reason, timeval* timeout = nullptr, Atomic* lock = nullptr, RefPtr beneficiary = {}); void wake_from_queue(); void unblock(); @@ -578,7 +596,7 @@ public: return !m_is_joinable; } - Thread* clone(Process&); + RefPtr clone(Process&); template static IterationDecision for_each_in_state(State, Callback); diff --git a/Kernel/init.cpp b/Kernel/init.cpp index c0d317b9159..7a6751a32a2 100644 --- a/Kernel/init.cpp +++ b/Kernel/init.cpp @@ -168,8 +168,13 @@ extern "C" [[noreturn]] void init() Process::initialize(); Scheduler::initialize(); - Thread* init_stage2_thread = nullptr; - Process::create_kernel_process(init_stage2_thread, "init_stage2", init_stage2); + { + RefPtr init_stage2_thread; + Process::create_kernel_process(init_stage2_thread, "init_stage2", init_stage2); + // We need to make sure we drop the reference for init_stage2_thread + // before calling into Scheduler::start, otherwise we will have a + // dangling Thread that never gets cleaned up + } Scheduler::start(); ASSERT_NOT_REACHED(); @@ -351,7 +356,7 @@ void init_stage2() // FIXME: It would be nicer to set the mode from userspace. tty0->set_graphical(!text_mode); - Thread* thread = nullptr; + RefPtr thread; auto userspace_init = kernel_command_line().lookup("init").value_or("/bin/SystemServer"); Process::create_user_process(thread, userspace_init, (uid_t)0, (gid_t)0, ProcessID(0), error, {}, {}, tty0); if (error != 0) {