From 38a90882a4230bb797ae3f6857e986d70f3bd265 Mon Sep 17 00:00:00 2001 From: Subv Date: Sun, 1 Jan 2017 11:57:02 -0500 Subject: [PATCH 1/9] Kernel/Synch: Do not attempt a reschedule on every syscall. Not all syscalls should cause reschedules, this commit attempts to remedy that, however, it still does not cover all cases. --- src/core/hle/kernel/mutex.cpp | 1 + src/core/hle/svc.cpp | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/core/hle/kernel/mutex.cpp b/src/core/hle/kernel/mutex.cpp index 736944bae0..8d92a9b8ed 100644 --- a/src/core/hle/kernel/mutex.cpp +++ b/src/core/hle/kernel/mutex.cpp @@ -84,6 +84,7 @@ void Mutex::Release() { if (lock_count == 0) { holding_thread->held_mutexes.erase(this); ResumeWaitingThread(this); + Core::System::GetInstance().PrepareReschedule(); } } } diff --git a/src/core/hle/svc.cpp b/src/core/hle/svc.cpp index 2ca270de3d..5b538be22f 100644 --- a/src/core/hle/svc.cpp +++ b/src/core/hle/svc.cpp @@ -248,6 +248,8 @@ static ResultCode SendSyncRequest(Kernel::Handle handle) { LOG_TRACE(Kernel_SVC, "called handle=0x%08X(%s)", handle, session->GetName().c_str()); + Core::System::GetInstance().PrepareReschedule(); + // TODO(Subv): svcSendSyncRequest should put the caller thread to sleep while the server // responds and cause a reschedule. return session->SendSyncRequest(); @@ -284,6 +286,8 @@ static ResultCode WaitSynchronization1(Kernel::Handle handle, s64 nano_seconds) // Create an event to wake the thread up after the specified nanosecond delay has passed thread->WakeAfterDelay(nano_seconds); + Core::System::GetInstance().PrepareReschedule(); + // Note: The output of this SVC will be set to RESULT_SUCCESS if the thread // resumes due to a signal in its wait objects. // Otherwise we retain the default value of timeout. @@ -366,6 +370,8 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha // Create an event to wake the thread up after the specified nanosecond delay has passed thread->WakeAfterDelay(nano_seconds); + Core::System::GetInstance().PrepareReschedule(); + // This value gets set to -1 by default in this case, it is not modified after this. *out = -1; // Note: The output of this SVC will be set to RESULT_SUCCESS if the thread resumes due to @@ -414,6 +420,8 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha // Create an event to wake the thread up after the specified nanosecond delay has passed thread->WakeAfterDelay(nano_seconds); + Core::System::GetInstance().PrepareReschedule(); + // Note: The output of this SVC will be set to RESULT_SUCCESS if the thread resumes due to a // signal in one of its wait objects. // Otherwise we retain the default value of timeout, and -1 in the out parameter @@ -448,6 +456,9 @@ static ResultCode ArbitrateAddress(Kernel::Handle handle, u32 address, u32 type, auto res = arbiter->ArbitrateAddress(static_cast(type), address, value, nanoseconds); + // TODO(Subv): Identify in which specific cases this call should cause a reschedule. + Core::System::GetInstance().PrepareReschedule(); + return res; } @@ -574,6 +585,8 @@ static ResultCode CreateThread(Kernel::Handle* out_handle, s32 priority, u32 ent CASCADE_RESULT(*out_handle, Kernel::g_handle_table.Create(std::move(thread))); + Core::System::GetInstance().PrepareReschedule(); + LOG_TRACE(Kernel_SVC, "called entrypoint=0x%08X (%s), arg=0x%08X, stacktop=0x%08X, " "threadpriority=0x%08X, processorid=0x%08X : created handle=0x%08X", entry_point, name.c_str(), arg, stack_top, priority, processor_id, *out_handle); @@ -586,6 +599,7 @@ static void ExitThread() { LOG_TRACE(Kernel_SVC, "called, pc=0x%08X", Core::CPU().GetPC()); Kernel::ExitCurrentThread(); + Core::System::GetInstance().PrepareReschedule(); } /// Gets the priority for the specified thread @@ -605,6 +619,7 @@ static ResultCode SetThreadPriority(Kernel::Handle handle, s32 priority) { return ERR_INVALID_HANDLE; thread->SetPriority(priority); + Core::System::GetInstance().PrepareReschedule(); return RESULT_SUCCESS; } @@ -849,6 +864,8 @@ static void SleepThread(s64 nanoseconds) { // Create an event to wake the thread up after the specified nanosecond delay has passed Kernel::GetCurrentThread()->WakeAfterDelay(nanoseconds); + + Core::System::GetInstance().PrepareReschedule(); } /// This returns the total CPU ticks elapsed since the CPU was powered-on @@ -1184,8 +1201,6 @@ void CallSVC(u32 immediate) { if (info) { if (info->func) { info->func(); - // TODO(Subv): Not all service functions should cause a reschedule in all cases. - Core::System::GetInstance().PrepareReschedule(); } else { LOG_ERROR(Kernel_SVC, "unimplemented SVC function %s(..)", info->name); } From e6a7723f2f4b62279cd4f6d4b48eb02a9b60ffb6 Mon Sep 17 00:00:00 2001 From: Subv Date: Sun, 1 Jan 2017 16:53:22 -0500 Subject: [PATCH 2/9] Kernel: Object ShouldWait and Acquire calls now take a thread as a parameter. This will be useful when implementing mutex priority inheritance. --- src/core/hle/kernel/event.cpp | 6 +++--- src/core/hle/kernel/event.h | 4 ++-- src/core/hle/kernel/kernel.cpp | 14 ++++++-------- src/core/hle/kernel/kernel.h | 9 +++++---- src/core/hle/kernel/mutex.cpp | 24 ++++++------------------ src/core/hle/kernel/mutex.h | 5 +++-- src/core/hle/kernel/semaphore.cpp | 6 +++--- src/core/hle/kernel/semaphore.h | 4 ++-- src/core/hle/kernel/server_port.cpp | 6 +++--- src/core/hle/kernel/server_port.h | 4 ++-- src/core/hle/kernel/server_session.cpp | 6 +++--- src/core/hle/kernel/server_session.h | 4 ++-- src/core/hle/kernel/thread.cpp | 6 +++--- src/core/hle/kernel/thread.h | 4 ++-- src/core/hle/kernel/timer.cpp | 6 +++--- src/core/hle/kernel/timer.h | 4 ++-- src/core/hle/svc.cpp | 12 ++++++------ 17 files changed, 56 insertions(+), 68 deletions(-) diff --git a/src/core/hle/kernel/event.cpp b/src/core/hle/kernel/event.cpp index 3e116e3dfc..e1f42af051 100644 --- a/src/core/hle/kernel/event.cpp +++ b/src/core/hle/kernel/event.cpp @@ -30,12 +30,12 @@ SharedPtr Event::Create(ResetType reset_type, std::string name) { return evt; } -bool Event::ShouldWait() { +bool Event::ShouldWait(Thread* thread) const { return !signaled; } -void Event::Acquire() { - ASSERT_MSG(!ShouldWait(), "object unavailable!"); +void Event::Acquire(Thread* thread) { + ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); // Release the event if it's not sticky... if (reset_type != ResetType::Sticky) diff --git a/src/core/hle/kernel/event.h b/src/core/hle/kernel/event.h index 8dcd23edb8..39452bf338 100644 --- a/src/core/hle/kernel/event.h +++ b/src/core/hle/kernel/event.h @@ -35,8 +35,8 @@ public: bool signaled; ///< Whether the event has already been signaled std::string name; ///< Name of event (optional) - bool ShouldWait() override; - void Acquire() override; + bool ShouldWait(Thread* thread) const override; + void Acquire(Thread* thread) override; void Signal(); void Clear(); diff --git a/src/core/hle/kernel/kernel.cpp b/src/core/hle/kernel/kernel.cpp index 1db8e102f0..ef9dbafa56 100644 --- a/src/core/hle/kernel/kernel.cpp +++ b/src/core/hle/kernel/kernel.cpp @@ -39,11 +39,6 @@ SharedPtr WaitObject::GetHighestPriorityReadyThread() { thread->status == THREADSTATUS_DEAD; }); - // TODO(Subv): This call should be performed inside the loop below to check if an object can be - // acquired by a particular thread. This is useful for things like recursive locking of Mutexes. - if (ShouldWait()) - return nullptr; - Thread* candidate = nullptr; s32 candidate_priority = THREADPRIO_LOWEST + 1; @@ -51,9 +46,12 @@ SharedPtr WaitObject::GetHighestPriorityReadyThread() { if (thread->current_priority >= candidate_priority) continue; + if (ShouldWait(thread.get())) + continue; + bool ready_to_run = std::none_of(thread->wait_objects.begin(), thread->wait_objects.end(), - [](const SharedPtr& object) { return object->ShouldWait(); }); + [&thread](const SharedPtr& object) { return object->ShouldWait(thread.get()); }); if (ready_to_run) { candidate = thread.get(); candidate_priority = thread->current_priority; @@ -66,7 +64,7 @@ SharedPtr WaitObject::GetHighestPriorityReadyThread() { void WaitObject::WakeupAllWaitingThreads() { while (auto thread = GetHighestPriorityReadyThread()) { if (!thread->IsSleepingOnWaitAll()) { - Acquire(); + Acquire(thread.get()); // Set the output index of the WaitSynchronizationN call to the index of this object. if (thread->wait_set_output) { thread->SetWaitSynchronizationOutput(thread->GetWaitObjectIndex(this)); @@ -74,7 +72,7 @@ void WaitObject::WakeupAllWaitingThreads() { } } else { for (auto& object : thread->wait_objects) { - object->Acquire(); + object->Acquire(thread.get()); object->RemoveWaitingThread(thread.get()); } // Note: This case doesn't update the output index of WaitSynchronizationN. diff --git a/src/core/hle/kernel/kernel.h b/src/core/hle/kernel/kernel.h index 9503e7d044..67eae93f27 100644 --- a/src/core/hle/kernel/kernel.h +++ b/src/core/hle/kernel/kernel.h @@ -132,13 +132,14 @@ using SharedPtr = boost::intrusive_ptr; class WaitObject : public Object { public: /** - * Check if the current thread should wait until the object is available + * Check if the specified thread should wait until the object is available + * @param thread The thread about which we're deciding. * @return True if the current thread should wait due to this object being unavailable */ - virtual bool ShouldWait() = 0; + virtual bool ShouldWait(Thread* thread) const = 0; - /// Acquire/lock the object if it is available - virtual void Acquire() = 0; + /// Acquire/lock the object for the specified thread if it is available + virtual void Acquire(Thread* thread) = 0; /** * Add a thread to wait on this object diff --git a/src/core/hle/kernel/mutex.cpp b/src/core/hle/kernel/mutex.cpp index 8d92a9b8ed..072e4e7c18 100644 --- a/src/core/hle/kernel/mutex.cpp +++ b/src/core/hle/kernel/mutex.cpp @@ -40,31 +40,19 @@ SharedPtr Mutex::Create(bool initial_locked, std::string name) { mutex->name = std::move(name); mutex->holding_thread = nullptr; - // Acquire mutex with current thread if initialized as locked... + // Acquire mutex with current thread if initialized as locked if (initial_locked) - mutex->Acquire(); + mutex->Acquire(GetCurrentThread()); return mutex; } -bool Mutex::ShouldWait() { - auto thread = GetCurrentThread(); - bool wait = lock_count > 0 && holding_thread != thread; - - // If the holding thread of the mutex is lower priority than this thread, that thread should - // temporarily inherit this thread's priority - if (wait && thread->current_priority < holding_thread->current_priority) - holding_thread->BoostPriority(thread->current_priority); - - return wait; +bool Mutex::ShouldWait(Thread* thread) const { + return lock_count > 0 && thread != holding_thread; } -void Mutex::Acquire() { - Acquire(GetCurrentThread()); -} - -void Mutex::Acquire(SharedPtr thread) { - ASSERT_MSG(!ShouldWait(), "object unavailable!"); +void Mutex::Acquire(Thread* thread) { + ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); // Actually "acquire" the mutex only if we don't already have it... if (lock_count == 0) { diff --git a/src/core/hle/kernel/mutex.h b/src/core/hle/kernel/mutex.h index 53c3dc1f1f..98b3d40b53 100644 --- a/src/core/hle/kernel/mutex.h +++ b/src/core/hle/kernel/mutex.h @@ -38,8 +38,9 @@ public: std::string name; ///< Name of mutex (optional) SharedPtr holding_thread; ///< Thread that has acquired the mutex - bool ShouldWait() override; - void Acquire() override; + bool ShouldWait(Thread* thread) const override; + void Acquire(Thread* thread) override; + /** * Acquires the specified mutex for the specified thread diff --git a/src/core/hle/kernel/semaphore.cpp b/src/core/hle/kernel/semaphore.cpp index bf76007805..5e6139265d 100644 --- a/src/core/hle/kernel/semaphore.cpp +++ b/src/core/hle/kernel/semaphore.cpp @@ -30,12 +30,12 @@ ResultVal> Semaphore::Create(s32 initial_count, s32 max_cou return MakeResult>(std::move(semaphore)); } -bool Semaphore::ShouldWait() { +bool Semaphore::ShouldWait(Thread* thread) const { return available_count <= 0; } -void Semaphore::Acquire() { - ASSERT_MSG(!ShouldWait(), "object unavailable!"); +void Semaphore::Acquire(Thread* thread) { + ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); --available_count; } diff --git a/src/core/hle/kernel/semaphore.h b/src/core/hle/kernel/semaphore.h index e01908a257..cde94f7ccd 100644 --- a/src/core/hle/kernel/semaphore.h +++ b/src/core/hle/kernel/semaphore.h @@ -39,8 +39,8 @@ public: s32 available_count; ///< Number of free slots left in the semaphore std::string name; ///< Name of semaphore (optional) - bool ShouldWait() override; - void Acquire() override; + bool ShouldWait(Thread* thread) const override; + void Acquire(Thread* thread) override; /** * Releases a certain number of slots from a semaphore. diff --git a/src/core/hle/kernel/server_port.cpp b/src/core/hle/kernel/server_port.cpp index 6c19aa7c09..fd3bbbcad3 100644 --- a/src/core/hle/kernel/server_port.cpp +++ b/src/core/hle/kernel/server_port.cpp @@ -14,13 +14,13 @@ namespace Kernel { ServerPort::ServerPort() {} ServerPort::~ServerPort() {} -bool ServerPort::ShouldWait() { +bool ServerPort::ShouldWait(Thread* thread) const { // If there are no pending sessions, we wait until a new one is added. return pending_sessions.size() == 0; } -void ServerPort::Acquire() { - ASSERT_MSG(!ShouldWait(), "object unavailable!"); +void ServerPort::Acquire(Thread* thread) { + ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); } std::tuple, SharedPtr> ServerPort::CreatePortPair( diff --git a/src/core/hle/kernel/server_port.h b/src/core/hle/kernel/server_port.h index b0f8df62c5..6f8bdb6a90 100644 --- a/src/core/hle/kernel/server_port.h +++ b/src/core/hle/kernel/server_port.h @@ -53,8 +53,8 @@ public: /// ServerSessions created from this port inherit a reference to this handler. std::shared_ptr hle_handler; - bool ShouldWait() override; - void Acquire() override; + bool ShouldWait(Thread* thread) const override; + void Acquire(Thread* thread) override; private: ServerPort(); diff --git a/src/core/hle/kernel/server_session.cpp b/src/core/hle/kernel/server_session.cpp index 146458c1ca..9447ff236a 100644 --- a/src/core/hle/kernel/server_session.cpp +++ b/src/core/hle/kernel/server_session.cpp @@ -29,12 +29,12 @@ ResultVal> ServerSession::Create( return MakeResult>(std::move(server_session)); } -bool ServerSession::ShouldWait() { +bool ServerSession::ShouldWait(Thread* thread) const { return !signaled; } -void ServerSession::Acquire() { - ASSERT_MSG(!ShouldWait(), "object unavailable!"); +void ServerSession::Acquire(Thread* thread) { + ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); signaled = false; } diff --git a/src/core/hle/kernel/server_session.h b/src/core/hle/kernel/server_session.h index 458284a5db..c088b9a199 100644 --- a/src/core/hle/kernel/server_session.h +++ b/src/core/hle/kernel/server_session.h @@ -57,9 +57,9 @@ public: */ ResultCode HandleSyncRequest(); - bool ShouldWait() override; + bool ShouldWait(Thread* thread) const override; - void Acquire() override; + void Acquire(Thread* thread) override; std::string name; ///< The name of this session (optional) bool signaled; ///< Whether there's new data available to this ServerSession diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index 5fb95dadaa..7d03a2cf72 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -27,12 +27,12 @@ namespace Kernel { /// Event type for the thread wake up event static int ThreadWakeupEventType; -bool Thread::ShouldWait() { +bool Thread::ShouldWait(Thread* thread) const { return status != THREADSTATUS_DEAD; } -void Thread::Acquire() { - ASSERT_MSG(!ShouldWait(), "object unavailable!"); +void Thread::Acquire(Thread* thread) { + ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); } // TODO(yuriks): This can be removed if Thread objects are explicitly pooled in the future, allowing diff --git a/src/core/hle/kernel/thread.h b/src/core/hle/kernel/thread.h index c77ac644d5..f2bc1ec9c5 100644 --- a/src/core/hle/kernel/thread.h +++ b/src/core/hle/kernel/thread.h @@ -72,8 +72,8 @@ public: return HANDLE_TYPE; } - bool ShouldWait() override; - void Acquire() override; + bool ShouldWait(Thread* thread) const override; + void Acquire(Thread* thread) override; /** * Gets the thread's current priority diff --git a/src/core/hle/kernel/timer.cpp b/src/core/hle/kernel/timer.cpp index b50cf520df..8f2bc4c7f1 100644 --- a/src/core/hle/kernel/timer.cpp +++ b/src/core/hle/kernel/timer.cpp @@ -39,12 +39,12 @@ SharedPtr Timer::Create(ResetType reset_type, std::string name) { return timer; } -bool Timer::ShouldWait() { +bool Timer::ShouldWait(Thread* thread) const { return !signaled; } -void Timer::Acquire() { - ASSERT_MSG(!ShouldWait(), "object unavailable!"); +void Timer::Acquire(Thread* thread) { + ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); if (reset_type == ResetType::OneShot) signaled = false; diff --git a/src/core/hle/kernel/timer.h b/src/core/hle/kernel/timer.h index 18ea0236b7..2e3b31b23d 100644 --- a/src/core/hle/kernel/timer.h +++ b/src/core/hle/kernel/timer.h @@ -39,8 +39,8 @@ public: u64 initial_delay; ///< The delay until the timer fires for the first time u64 interval_delay; ///< The delay until the timer fires after the first time - bool ShouldWait() override; - void Acquire() override; + bool ShouldWait(Thread* thread) const override; + void Acquire(Thread* thread) override; /** * Starts the timer, with the specified initial delay and interval. diff --git a/src/core/hle/svc.cpp b/src/core/hle/svc.cpp index 5b538be22f..159ac0bf63 100644 --- a/src/core/hle/svc.cpp +++ b/src/core/hle/svc.cpp @@ -272,7 +272,7 @@ static ResultCode WaitSynchronization1(Kernel::Handle handle, s64 nano_seconds) LOG_TRACE(Kernel_SVC, "called handle=0x%08X(%s:%s), nanoseconds=%lld", handle, object->GetTypeName().c_str(), object->GetName().c_str(), nano_seconds); - if (object->ShouldWait()) { + if (object->ShouldWait(thread)) { if (nano_seconds == 0) return ERR_SYNC_TIMEOUT; @@ -294,7 +294,7 @@ static ResultCode WaitSynchronization1(Kernel::Handle handle, s64 nano_seconds) return ERR_SYNC_TIMEOUT; } - object->Acquire(); + object->Acquire(thread); return RESULT_SUCCESS; } @@ -336,11 +336,11 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha if (wait_all) { bool all_available = std::all_of(objects.begin(), objects.end(), - [](const ObjectPtr& object) { return !object->ShouldWait(); }); + [thread](const ObjectPtr& object) { return !object->ShouldWait(thread); }); if (all_available) { // We can acquire all objects right now, do so. for (auto& object : objects) - object->Acquire(); + object->Acquire(thread); // Note: In this case, the `out` parameter is not set, // and retains whatever value it had before. return RESULT_SUCCESS; @@ -380,12 +380,12 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha } else { // Find the first object that is acquirable in the provided list of objects auto itr = std::find_if(objects.begin(), objects.end(), - [](const ObjectPtr& object) { return !object->ShouldWait(); }); + [thread](const ObjectPtr& object) { return !object->ShouldWait(thread); }); if (itr != objects.end()) { // We found a ready object, acquire it and set the result value Kernel::WaitObject* object = itr->get(); - object->Acquire(); + object->Acquire(thread); *out = std::distance(objects.begin(), itr); return RESULT_SUCCESS; } From 7abf1853907fe086753df0031262b668a2da88b0 Mon Sep 17 00:00:00 2001 From: Subv Date: Sun, 1 Jan 2017 16:59:30 -0500 Subject: [PATCH 3/9] Kernel/Mutex: Implemented priority inheritance. The implementation is based on reverse engineering of the 3DS's kernel. A mutex holder's priority will be temporarily boosted to the best priority among any threads that want to acquire any of its held mutexes. When the holder releases the mutex, it's priority will be boosted to the best priority among the threads that want to acquire any of its remaining held mutexes. --- src/core/hle/kernel/kernel.h | 2 +- src/core/hle/kernel/mutex.cpp | 58 ++++++++++++++++++++++++++-------- src/core/hle/kernel/mutex.h | 7 ++-- src/core/hle/kernel/thread.cpp | 6 ++-- src/core/hle/svc.cpp | 9 ------ 5 files changed, 51 insertions(+), 31 deletions(-) diff --git a/src/core/hle/kernel/kernel.h b/src/core/hle/kernel/kernel.h index 67eae93f27..2680f89c9f 100644 --- a/src/core/hle/kernel/kernel.h +++ b/src/core/hle/kernel/kernel.h @@ -145,7 +145,7 @@ public: * Add a thread to wait on this object * @param thread Pointer to thread to add */ - void AddWaitingThread(SharedPtr thread); + virtual void AddWaitingThread(SharedPtr thread); /** * Removes a thread from waiting on this object (e.g. if it was resumed already) diff --git a/src/core/hle/kernel/mutex.cpp b/src/core/hle/kernel/mutex.cpp index 072e4e7c18..e83717e80f 100644 --- a/src/core/hle/kernel/mutex.cpp +++ b/src/core/hle/kernel/mutex.cpp @@ -6,6 +6,7 @@ #include #include #include "common/assert.h" +#include "core/core.h" #include "core/hle/kernel/kernel.h" #include "core/hle/kernel/mutex.h" #include "core/hle/kernel/thread.h" @@ -13,19 +14,25 @@ namespace Kernel { /** - * Resumes a thread waiting for the specified mutex - * @param mutex The mutex that some thread is waiting on + * Boost's a thread's priority to the best priority among the thread's held mutexes. + * This prevents priority inversion via priority inheritance. */ -static void ResumeWaitingThread(Mutex* mutex) { - // Reset mutex lock thread handle, nothing is waiting - mutex->lock_count = 0; - mutex->holding_thread = nullptr; - mutex->WakeupAllWaitingThreads(); +static void UpdateThreadPriority(Thread* thread) { + s32 best_priority = THREADPRIO_LOWEST; + for (auto& mutex : thread->held_mutexes) { + if (mutex->priority < best_priority) + best_priority = mutex->priority; + } + + best_priority = std::min(best_priority, thread->nominal_priority); + thread->SetPriority(best_priority); } void ReleaseThreadMutexes(Thread* thread) { for (auto& mtx : thread->held_mutexes) { - ResumeWaitingThread(mtx.get()); + mtx->lock_count = 0; + mtx->holding_thread = nullptr; + mtx->WakeupAllWaitingThreads(); } thread->held_mutexes.clear(); } @@ -54,27 +61,52 @@ bool Mutex::ShouldWait(Thread* thread) const { void Mutex::Acquire(Thread* thread) { ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); - // Actually "acquire" the mutex only if we don't already have it... + // Actually "acquire" the mutex only if we don't already have it if (lock_count == 0) { + priority = thread->current_priority; thread->held_mutexes.insert(this); - holding_thread = std::move(thread); + holding_thread = thread; + + UpdateThreadPriority(thread); + + Core::System::GetInstance().PrepareReschedule(); } lock_count++; } void Mutex::Release() { - // Only release if the mutex is held... + // Only release if the mutex is held if (lock_count > 0) { lock_count--; - // Yield to the next thread only if we've fully released the mutex... + // Yield to the next thread only if we've fully released the mutex if (lock_count == 0) { holding_thread->held_mutexes.erase(this); - ResumeWaitingThread(this); + UpdateThreadPriority(holding_thread.get()); + holding_thread = nullptr; + WakeupAllWaitingThreads(); Core::System::GetInstance().PrepareReschedule(); } } } +void Mutex::AddWaitingThread(SharedPtr thread) { + WaitObject::AddWaitingThread(thread); + + // Elevate the mutex priority to the best priority + // among the priorities of all its waiting threads. + + s32 best_priority = THREADPRIO_LOWEST; + for (auto& waiter : GetWaitingThreads()) { + if (waiter->current_priority < best_priority) + best_priority = waiter->current_priority; + } + + if (best_priority != priority) { + priority = best_priority; + UpdateThreadPriority(holding_thread.get()); + } +} + } // namespace diff --git a/src/core/hle/kernel/mutex.h b/src/core/hle/kernel/mutex.h index 98b3d40b53..3e6adeb179 100644 --- a/src/core/hle/kernel/mutex.h +++ b/src/core/hle/kernel/mutex.h @@ -35,18 +35,15 @@ public: } int lock_count; ///< Number of times the mutex has been acquired + u32 priority; ///< The priority of the mutex, used for priority inheritance. std::string name; ///< Name of mutex (optional) SharedPtr holding_thread; ///< Thread that has acquired the mutex bool ShouldWait(Thread* thread) const override; void Acquire(Thread* thread) override; + void AddWaitingThread(SharedPtr thread) override; - /** - * Acquires the specified mutex for the specified thread - * @param thread Thread that will acquire the mutex - */ - void Acquire(SharedPtr thread); void Release(); private: diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index 7d03a2cf72..d44010824d 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -90,9 +90,6 @@ static bool CheckWait_AddressArbiter(const Thread* thread, VAddr wait_address) { } void Thread::Stop() { - // Release all the mutexes that this thread holds - ReleaseThreadMutexes(this); - // Cancel any outstanding wakeup events for this thread CoreTiming::UnscheduleEvent(ThreadWakeupEventType, callback_handle); wakeup_callback_handle_table.Close(callback_handle); @@ -108,6 +105,9 @@ void Thread::Stop() { WakeupAllWaitingThreads(); + // Release all the mutexes that this thread holds + ReleaseThreadMutexes(this); + // Clean up any dangling references in objects that this thread was waiting for for (auto& wait_object : wait_objects) { wait_object->RemoveWaitingThread(this); diff --git a/src/core/hle/svc.cpp b/src/core/hle/svc.cpp index 159ac0bf63..5d63593446 100644 --- a/src/core/hle/svc.cpp +++ b/src/core/hle/svc.cpp @@ -278,9 +278,6 @@ static ResultCode WaitSynchronization1(Kernel::Handle handle, s64 nano_seconds) return ERR_SYNC_TIMEOUT; object->AddWaitingThread(thread); - // TODO(Subv): Perform things like update the mutex lock owner's priority to - // prevent priority inversion. Currently this is done in Mutex::ShouldWait, - // but it should be moved to a function that is called from here. thread->status = THREADSTATUS_WAIT_SYNCH; // Create an event to wake the thread up after the specified nanosecond delay has passed @@ -359,9 +356,6 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha // Add the thread to each of the objects' waiting threads. for (auto& object : objects) { object->AddWaitingThread(thread); - // TODO(Subv): Perform things like update the mutex lock owner's priority to - // prevent priority inversion. Currently this is done in Mutex::ShouldWait, - // but it should be moved to a function that is called from here. } // Set the thread's waitlist to the list of objects passed to WaitSynchronizationN @@ -409,9 +403,6 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha // Set the index of this object in the mapping of Objects -> index for this thread. thread->wait_objects_index[object->GetObjectId()] = static_cast(i); object->AddWaitingThread(thread); - // TODO(Subv): Perform things like update the mutex lock owner's priority to - // prevent priority inversion. Currently this is done in Mutex::ShouldWait, - // but it should be moved to a function that is called from here. } // Note: If no handles and no timeout were given, then the thread will deadlock, this is From b6a0355568ee327bef8957b9a2498897b96e1278 Mon Sep 17 00:00:00 2001 From: Subv Date: Mon, 2 Jan 2017 13:53:10 -0500 Subject: [PATCH 4/9] Kernel/Mutex: Update a mutex priority when a thread stops waiting on it. --- src/core/hle/kernel/kernel.cpp | 23 ++++++++++++++-------- src/core/hle/kernel/kernel.h | 2 +- src/core/hle/kernel/mutex.cpp | 35 +++++++++++++++++++++------------- src/core/hle/kernel/mutex.h | 1 + src/core/hle/svc.cpp | 5 +++-- 5 files changed, 42 insertions(+), 24 deletions(-) diff --git a/src/core/hle/kernel/kernel.cpp b/src/core/hle/kernel/kernel.cpp index ef9dbafa56..6f61d526ad 100644 --- a/src/core/hle/kernel/kernel.cpp +++ b/src/core/hle/kernel/kernel.cpp @@ -3,7 +3,6 @@ // Refer to the license.txt file included. #include -#include #include "common/assert.h" #include "common/logging/log.h" #include "core/hle/config_mem.h" @@ -34,10 +33,17 @@ void WaitObject::RemoveWaitingThread(Thread* thread) { SharedPtr WaitObject::GetHighestPriorityReadyThread() { // Remove the threads that are ready or already running from our waitlist - boost::range::remove_erase_if(waiting_threads, [](const SharedPtr& thread) { - return thread->status == THREADSTATUS_RUNNING || thread->status == THREADSTATUS_READY || - thread->status == THREADSTATUS_DEAD; - }); + auto to_remove = waiting_threads.end(); + do { + to_remove = std::find_if(waiting_threads.begin(), waiting_threads.end(), + [](const SharedPtr& thread) { + return thread->status == THREADSTATUS_RUNNING || + thread->status == THREADSTATUS_READY || + thread->status == THREADSTATUS_DEAD; + }); + // Call RemoveWaitingThread so that child classes can override the behavior. + RemoveWaitingThread(to_remove->get()); + } while (to_remove != waiting_threads.end()); Thread* candidate = nullptr; s32 candidate_priority = THREADPRIO_LOWEST + 1; @@ -49,9 +55,10 @@ SharedPtr WaitObject::GetHighestPriorityReadyThread() { if (ShouldWait(thread.get())) continue; - bool ready_to_run = - std::none_of(thread->wait_objects.begin(), thread->wait_objects.end(), - [&thread](const SharedPtr& object) { return object->ShouldWait(thread.get()); }); + bool ready_to_run = std::none_of(thread->wait_objects.begin(), thread->wait_objects.end(), + [&thread](const SharedPtr& object) { + return object->ShouldWait(thread.get()); + }); if (ready_to_run) { candidate = thread.get(); candidate_priority = thread->current_priority; diff --git a/src/core/hle/kernel/kernel.h b/src/core/hle/kernel/kernel.h index 2680f89c9f..05097824b5 100644 --- a/src/core/hle/kernel/kernel.h +++ b/src/core/hle/kernel/kernel.h @@ -151,7 +151,7 @@ public: * Removes a thread from waiting on this object (e.g. if it was resumed already) * @param thread Pointer to thread to remove */ - void RemoveWaitingThread(Thread* thread); + virtual void RemoveWaitingThread(Thread* thread); /** * Wake up all threads waiting on this object that can be awoken, in priority order, diff --git a/src/core/hle/kernel/mutex.cpp b/src/core/hle/kernel/mutex.cpp index e83717e80f..84ff651502 100644 --- a/src/core/hle/kernel/mutex.cpp +++ b/src/core/hle/kernel/mutex.cpp @@ -28,6 +28,23 @@ static void UpdateThreadPriority(Thread* thread) { thread->SetPriority(best_priority); } +/** + * Elevate the mutex priority to the best priority + * among the priorities of all its waiting threads. + */ +static void UpdateMutexPriority(Mutex* mutex) { + s32 best_priority = THREADPRIO_LOWEST; + for (auto& waiter : mutex->GetWaitingThreads()) { + if (waiter->current_priority < best_priority) + best_priority = waiter->current_priority; + } + + if (best_priority != mutex->priority) { + mutex->priority = best_priority; + UpdateThreadPriority(mutex->holding_thread.get()); + } +} + void ReleaseThreadMutexes(Thread* thread) { for (auto& mtx : thread->held_mutexes) { mtx->lock_count = 0; @@ -93,20 +110,12 @@ void Mutex::Release() { void Mutex::AddWaitingThread(SharedPtr thread) { WaitObject::AddWaitingThread(thread); + UpdateMutexPriority(this); +} - // Elevate the mutex priority to the best priority - // among the priorities of all its waiting threads. - - s32 best_priority = THREADPRIO_LOWEST; - for (auto& waiter : GetWaitingThreads()) { - if (waiter->current_priority < best_priority) - best_priority = waiter->current_priority; - } - - if (best_priority != priority) { - priority = best_priority; - UpdateThreadPriority(holding_thread.get()); - } +void Mutex::RemoveWaitingThread(Thread* thread) { + WaitObject::RemoveWaitingThread(thread); + UpdateMutexPriority(this); } } // namespace diff --git a/src/core/hle/kernel/mutex.h b/src/core/hle/kernel/mutex.h index 3e6adeb179..31f9205167 100644 --- a/src/core/hle/kernel/mutex.h +++ b/src/core/hle/kernel/mutex.h @@ -43,6 +43,7 @@ public: void Acquire(Thread* thread) override; void AddWaitingThread(SharedPtr thread) override; + void RemoveWaitingThread(Thread* thread) override; void Release(); diff --git a/src/core/hle/svc.cpp b/src/core/hle/svc.cpp index 5d63593446..b6e34a9e9a 100644 --- a/src/core/hle/svc.cpp +++ b/src/core/hle/svc.cpp @@ -373,8 +373,9 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha return ERR_SYNC_TIMEOUT; } else { // Find the first object that is acquirable in the provided list of objects - auto itr = std::find_if(objects.begin(), objects.end(), - [thread](const ObjectPtr& object) { return !object->ShouldWait(thread); }); + auto itr = std::find_if(objects.begin(), objects.end(), [thread](const ObjectPtr& object) { + return !object->ShouldWait(thread); + }); if (itr != objects.end()) { // We found a ready object, acquire it and set the result value From d3ff5b91e14356912589f9bac47fccbe79e07279 Mon Sep 17 00:00:00 2001 From: Subv Date: Mon, 2 Jan 2017 19:38:08 -0500 Subject: [PATCH 5/9] Kernel/Mutex: Propagate thread priority changes to other threads inheriting the priority via mutexes --- src/core/hle/kernel/mutex.cpp | 60 +++++++++++++--------------------- src/core/hle/kernel/mutex.h | 6 ++++ src/core/hle/kernel/thread.cpp | 21 +++++++++--- src/core/hle/kernel/thread.h | 9 +++++ src/core/hle/svc.cpp | 6 ++++ 5 files changed, 60 insertions(+), 42 deletions(-) diff --git a/src/core/hle/kernel/mutex.cpp b/src/core/hle/kernel/mutex.cpp index 84ff651502..cef9612899 100644 --- a/src/core/hle/kernel/mutex.cpp +++ b/src/core/hle/kernel/mutex.cpp @@ -13,38 +13,6 @@ namespace Kernel { -/** - * Boost's a thread's priority to the best priority among the thread's held mutexes. - * This prevents priority inversion via priority inheritance. - */ -static void UpdateThreadPriority(Thread* thread) { - s32 best_priority = THREADPRIO_LOWEST; - for (auto& mutex : thread->held_mutexes) { - if (mutex->priority < best_priority) - best_priority = mutex->priority; - } - - best_priority = std::min(best_priority, thread->nominal_priority); - thread->SetPriority(best_priority); -} - -/** - * Elevate the mutex priority to the best priority - * among the priorities of all its waiting threads. - */ -static void UpdateMutexPriority(Mutex* mutex) { - s32 best_priority = THREADPRIO_LOWEST; - for (auto& waiter : mutex->GetWaitingThreads()) { - if (waiter->current_priority < best_priority) - best_priority = waiter->current_priority; - } - - if (best_priority != mutex->priority) { - mutex->priority = best_priority; - UpdateThreadPriority(mutex->holding_thread.get()); - } -} - void ReleaseThreadMutexes(Thread* thread) { for (auto& mtx : thread->held_mutexes) { mtx->lock_count = 0; @@ -83,9 +51,7 @@ void Mutex::Acquire(Thread* thread) { priority = thread->current_priority; thread->held_mutexes.insert(this); holding_thread = thread; - - UpdateThreadPriority(thread); - + thread->UpdatePriority(); Core::System::GetInstance().PrepareReschedule(); } @@ -100,7 +66,7 @@ void Mutex::Release() { // Yield to the next thread only if we've fully released the mutex if (lock_count == 0) { holding_thread->held_mutexes.erase(this); - UpdateThreadPriority(holding_thread.get()); + holding_thread->UpdatePriority(); holding_thread = nullptr; WakeupAllWaitingThreads(); Core::System::GetInstance().PrepareReschedule(); @@ -110,12 +76,30 @@ void Mutex::Release() { void Mutex::AddWaitingThread(SharedPtr thread) { WaitObject::AddWaitingThread(thread); - UpdateMutexPriority(this); + thread->pending_mutexes.insert(this); + UpdatePriority(); } void Mutex::RemoveWaitingThread(Thread* thread) { WaitObject::RemoveWaitingThread(thread); - UpdateMutexPriority(this); + thread->pending_mutexes.erase(this); + UpdatePriority(); +} + +void Mutex::UpdatePriority() { + if (!holding_thread) + return; + + s32 best_priority = THREADPRIO_LOWEST; + for (auto& waiter : GetWaitingThreads()) { + if (waiter->current_priority < best_priority) + best_priority = waiter->current_priority; + } + + if (best_priority != priority) { + priority = best_priority; + holding_thread->UpdatePriority(); + } } } // namespace diff --git a/src/core/hle/kernel/mutex.h b/src/core/hle/kernel/mutex.h index 31f9205167..c57adf4002 100644 --- a/src/core/hle/kernel/mutex.h +++ b/src/core/hle/kernel/mutex.h @@ -39,6 +39,12 @@ public: std::string name; ///< Name of mutex (optional) SharedPtr holding_thread; ///< Thread that has acquired the mutex + /** + * Elevate the mutex priority to the best priority + * among the priorities of all its waiting threads. + */ + void UpdatePriority(); + bool ShouldWait(Thread* thread) const override; void Acquire(Thread* thread) override; diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index d44010824d..3a5a67450d 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -105,15 +105,15 @@ void Thread::Stop() { WakeupAllWaitingThreads(); - // Release all the mutexes that this thread holds - ReleaseThreadMutexes(this); - // Clean up any dangling references in objects that this thread was waiting for for (auto& wait_object : wait_objects) { wait_object->RemoveWaitingThread(this); } wait_objects.clear(); + // Release all the mutexes that this thread holds + ReleaseThreadMutexes(this); + // Mark the TLS slot in the thread's page as free. u32 tls_page = (tls_address - Memory::TLS_AREA_VADDR) / Memory::PAGE_SIZE; u32 tls_slot = @@ -515,8 +515,21 @@ void Thread::SetPriority(s32 priority) { nominal_priority = current_priority = priority; } +void Thread::UpdatePriority() { + s32 best_priority = nominal_priority; + for (auto& mutex : held_mutexes) { + if (mutex->priority < best_priority) + best_priority = mutex->priority; + } + BoostPriority(best_priority); +} + void Thread::BoostPriority(s32 priority) { - ready_queue.move(this, current_priority, priority); + // If thread was ready, adjust queues + if (status == THREADSTATUS_READY) + ready_queue.move(this, current_priority, priority); + else + ready_queue.prepare(priority); current_priority = priority; } diff --git a/src/core/hle/kernel/thread.h b/src/core/hle/kernel/thread.h index f2bc1ec9c5..e2f0cc831f 100644 --- a/src/core/hle/kernel/thread.h +++ b/src/core/hle/kernel/thread.h @@ -89,6 +89,12 @@ public: */ void SetPriority(s32 priority); + /** + * Boost's a thread's priority to the best priority among the thread's held mutexes. + * This prevents priority inversion via priority inheritance. + */ + void UpdatePriority(); + /** * Temporarily boosts the thread's priority until the next time it is scheduled * @param priority The new priority @@ -178,6 +184,9 @@ public: /// Mutexes currently held by this thread, which will be released when it exits. boost::container::flat_set> held_mutexes; + /// Mutexes that this thread is currently waiting for. + boost::container::flat_set> pending_mutexes; + SharedPtr owner_process; ///< Process that owns this thread /// Objects that the thread is waiting on. diff --git a/src/core/hle/svc.cpp b/src/core/hle/svc.cpp index b6e34a9e9a..160f27c98a 100644 --- a/src/core/hle/svc.cpp +++ b/src/core/hle/svc.cpp @@ -611,6 +611,12 @@ static ResultCode SetThreadPriority(Kernel::Handle handle, s32 priority) { return ERR_INVALID_HANDLE; thread->SetPriority(priority); + thread->UpdatePriority(); + + // Update the mutexes that this thread is waiting for + for (auto& mutex : thread->pending_mutexes) + mutex->UpdatePriority(); + Core::System::GetInstance().PrepareReschedule(); return RESULT_SUCCESS; } From cef5f45de2fd64f0728d4504d0ad7434cb8ac519 Mon Sep 17 00:00:00 2001 From: Subv Date: Wed, 4 Jan 2017 10:44:38 -0500 Subject: [PATCH 6/9] Kernel: Use different thread statuses when a thread calls WaitSynchronization1 and WaitSynchronizationN with wait_all = true. This commit removes the overly general THREADSTATUS_WAIT_SYNCH and replaces it with two more granular statuses: THREADSTATUS_WAIT_SYNCH_ANY when a thread waits on objects via WaitSynchronization1 or WaitSynchronizationN with wait_all = false. THREADSTATUS_WAIT_SYNCH_ALL when a thread waits on objects via WaitSynchronizationN with wait_all = true. --- src/citra_qt/debugger/wait_tree.cpp | 9 ++++++--- src/core/hle/kernel/thread.cpp | 11 +++++++---- src/core/hle/kernel/thread.h | 19 ++++++++++--------- src/core/hle/svc.cpp | 6 +++--- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/src/citra_qt/debugger/wait_tree.cpp b/src/citra_qt/debugger/wait_tree.cpp index 1d2de5185f..b6ecf38194 100644 --- a/src/citra_qt/debugger/wait_tree.cpp +++ b/src/citra_qt/debugger/wait_tree.cpp @@ -153,7 +153,8 @@ QString WaitTreeThread::GetText() const { case THREADSTATUS_WAIT_SLEEP: status = tr("sleeping"); break; - case THREADSTATUS_WAIT_SYNCH: + case THREADSTATUS_WAIT_SYNCH_ALL: + case THREADSTATUS_WAIT_SYNCH_ANY: status = tr("waiting for objects"); break; case THREADSTATUS_DORMANT: @@ -180,7 +181,8 @@ QColor WaitTreeThread::GetColor() const { return QColor(Qt::GlobalColor::darkRed); case THREADSTATUS_WAIT_SLEEP: return QColor(Qt::GlobalColor::darkYellow); - case THREADSTATUS_WAIT_SYNCH: + case THREADSTATUS_WAIT_SYNCH_ALL: + case THREADSTATUS_WAIT_SYNCH_ANY: return QColor(Qt::GlobalColor::red); case THREADSTATUS_DORMANT: return QColor(Qt::GlobalColor::darkCyan); @@ -228,7 +230,8 @@ std::vector> WaitTreeThread::GetChildren() const { } else { list.push_back(std::make_unique(thread.held_mutexes)); } - if (thread.status == THREADSTATUS_WAIT_SYNCH) { + if (thread.status == THREADSTATUS_WAIT_SYNCH_ANY || + thread.status == THREADSTATUS_WAIT_SYNCH_ALL) { list.push_back(std::make_unique(thread.wait_objects, thread.IsSleepingOnWaitAll())); } diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index 3a5a67450d..aa99d18c71 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -72,7 +72,8 @@ Thread* GetCurrentThread() { * @return True if the thread is waiting, false otherwise */ static bool CheckWait_WaitObject(const Thread* thread, WaitObject* wait_object) { - if (thread->status != THREADSTATUS_WAIT_SYNCH) + if (thread->status != THREADSTATUS_WAIT_SYNCH_ALL && + thread->status != THREADSTATUS_WAIT_SYNCH_ANY) return false; auto itr = std::find(thread->wait_objects.begin(), thread->wait_objects.end(), wait_object); @@ -253,7 +254,7 @@ void WaitCurrentThread_WaitSynchronization(std::vector> wa Thread* thread = GetCurrentThread(); thread->wait_set_output = wait_set_output; thread->wait_objects = std::move(wait_objects); - thread->status = THREADSTATUS_WAIT_SYNCH; + thread->status = THREADSTATUS_WAIT_SYNCH_ANY; } void WaitCurrentThread_ArbitrateAddress(VAddr wait_address) { @@ -281,7 +282,8 @@ static void ThreadWakeupCallback(u64 thread_handle, int cycles_late) { return; } - if (thread->status == THREADSTATUS_WAIT_SYNCH || thread->status == THREADSTATUS_WAIT_ARB) { + if (thread->status == THREADSTATUS_WAIT_SYNCH_ANY || + thread->status == THREADSTATUS_WAIT_SYNCH_ALL || thread->status == THREADSTATUS_WAIT_ARB) { thread->wait_set_output = false; // Remove the thread from each of its waiting objects' waitlists for (auto& object : thread->wait_objects) @@ -306,7 +308,8 @@ void Thread::WakeAfterDelay(s64 nanoseconds) { void Thread::ResumeFromWait() { switch (status) { - case THREADSTATUS_WAIT_SYNCH: + case THREADSTATUS_WAIT_SYNCH_ALL: + case THREADSTATUS_WAIT_SYNCH_ANY: case THREADSTATUS_WAIT_ARB: case THREADSTATUS_WAIT_SLEEP: break; diff --git a/src/core/hle/kernel/thread.h b/src/core/hle/kernel/thread.h index e2f0cc831f..6cd8c20e27 100644 --- a/src/core/hle/kernel/thread.h +++ b/src/core/hle/kernel/thread.h @@ -31,13 +31,14 @@ enum ThreadProcessorId : s32 { }; enum ThreadStatus { - THREADSTATUS_RUNNING, ///< Currently running - THREADSTATUS_READY, ///< Ready to run - THREADSTATUS_WAIT_ARB, ///< Waiting on an address arbiter - THREADSTATUS_WAIT_SLEEP, ///< Waiting due to a SleepThread SVC - THREADSTATUS_WAIT_SYNCH, ///< Waiting due to a WaitSynchronization SVC - THREADSTATUS_DORMANT, ///< Created but not yet made ready - THREADSTATUS_DEAD ///< Run to completion, or forcefully terminated + THREADSTATUS_RUNNING, ///< Currently running + THREADSTATUS_READY, ///< Ready to run + THREADSTATUS_WAIT_ARB, ///< Waiting on an address arbiter + THREADSTATUS_WAIT_SLEEP, ///< Waiting due to a SleepThread SVC + THREADSTATUS_WAIT_SYNCH_ANY, ///< Waiting due to WaitSynch1 or WaitSynchN with wait_all = false + THREADSTATUS_WAIT_SYNCH_ALL, ///< Waiting due to WaitSynchronizationN with wait_all = true + THREADSTATUS_DORMANT, ///< Created but not yet made ready + THREADSTATUS_DEAD ///< Run to completion, or forcefully terminated }; namespace Kernel { @@ -158,10 +159,10 @@ public: /** * Returns whether this thread is waiting for all the objects in * its wait list to become ready, as a result of a WaitSynchronizationN call - * with wait_all = true, or a ReplyAndReceive call. + * with wait_all = true. */ bool IsSleepingOnWaitAll() const { - return !wait_objects.empty(); + return status == THREADSTATUS_WAIT_SYNCH_ALL; } ARM_Interface::ThreadContext context; diff --git a/src/core/hle/svc.cpp b/src/core/hle/svc.cpp index 160f27c98a..1e1ca5180c 100644 --- a/src/core/hle/svc.cpp +++ b/src/core/hle/svc.cpp @@ -278,7 +278,7 @@ static ResultCode WaitSynchronization1(Kernel::Handle handle, s64 nano_seconds) return ERR_SYNC_TIMEOUT; object->AddWaitingThread(thread); - thread->status = THREADSTATUS_WAIT_SYNCH; + thread->status = THREADSTATUS_WAIT_SYNCH_ANY; // Create an event to wake the thread up after the specified nanosecond delay has passed thread->WakeAfterDelay(nano_seconds); @@ -351,7 +351,7 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha return ERR_SYNC_TIMEOUT; // Put the thread to sleep - thread->status = THREADSTATUS_WAIT_SYNCH; + thread->status = THREADSTATUS_WAIT_SYNCH_ALL; // Add the thread to each of the objects' waiting threads. for (auto& object : objects) { @@ -393,7 +393,7 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha return ERR_SYNC_TIMEOUT; // Put the thread to sleep - thread->status = THREADSTATUS_WAIT_SYNCH; + thread->status = THREADSTATUS_WAIT_SYNCH_ANY; // Clear the thread's waitlist, we won't use it for wait_all = false thread->wait_objects.clear(); From fd95b6ee2606da4cd47c5f2916ad3b4f86c0e0f4 Mon Sep 17 00:00:00 2001 From: Subv Date: Wed, 4 Jan 2017 10:53:01 -0500 Subject: [PATCH 7/9] Kernel: Remove Thread::wait_objects_index and use wait_objects to hold all the objects that a thread is waiting on. --- src/core/hle/kernel/kernel.cpp | 8 +++++++- src/core/hle/kernel/thread.cpp | 5 +++++ src/core/hle/kernel/thread.h | 16 +++++++--------- src/core/hle/svc.cpp | 14 +++----------- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/core/hle/kernel/kernel.cpp b/src/core/hle/kernel/kernel.cpp index 6f61d526ad..955f50a9bf 100644 --- a/src/core/hle/kernel/kernel.cpp +++ b/src/core/hle/kernel/kernel.cpp @@ -55,10 +55,16 @@ SharedPtr WaitObject::GetHighestPriorityReadyThread() { if (ShouldWait(thread.get())) continue; - bool ready_to_run = std::none_of(thread->wait_objects.begin(), thread->wait_objects.end(), + // A thread is ready to run if it's either in THREADSTATUS_WAIT_SYNCH_ANY or + // in THREADSTATUS_WAIT_SYNCH_ALL and the rest of the objects it is waiting on are ready. + bool ready_to_run = true; + if (thread->status == THREADSTATUS_WAIT_SYNCH_ALL) { + ready_to_run = std::none_of(thread->wait_objects.begin(), thread->wait_objects.end(), [&thread](const SharedPtr& object) { return object->ShouldWait(thread.get()); }); + } + if (ready_to_run) { candidate = thread.get(); candidate_priority = thread->current_priority; diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index aa99d18c71..568cef5b91 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -579,6 +579,11 @@ void Thread::SetWaitSynchronizationOutput(s32 output) { context.cpu_registers[1] = output; } +s32 Thread::GetWaitObjectIndex(WaitObject* object) const { + auto match = std::find(wait_objects.rbegin(), wait_objects.rend(), object); + return std::distance(match, wait_objects.rend()) - 1; +} + //////////////////////////////////////////////////////////////////////////////////////////////////// void ThreadingInit() { diff --git a/src/core/hle/kernel/thread.h b/src/core/hle/kernel/thread.h index 6cd8c20e27..af72b76eab 100644 --- a/src/core/hle/kernel/thread.h +++ b/src/core/hle/kernel/thread.h @@ -135,13 +135,14 @@ public: /** * Retrieves the index that this particular object occupies in the list of objects - * that the thread passed to WaitSynchronizationN. + * that the thread passed to WaitSynchronizationN, starting the search from the last element. * It is used to set the output value of WaitSynchronizationN when the thread is awakened. + * When a thread wakes up due to an object signal, the kernel will use the index of the last + * matching object in the wait objects list in case of having multiple instances of the same + * object in the list. * @param object Object to query the index of. */ - s32 GetWaitObjectIndex(const WaitObject* object) const { - return wait_objects_index.at(object->GetObjectId()); - } + s32 GetWaitObjectIndex(WaitObject* object) const; /** * Stops a thread, invalidating it from further use @@ -190,13 +191,10 @@ public: SharedPtr owner_process; ///< Process that owns this thread - /// Objects that the thread is waiting on. - /// This is only populated when the thread should wait for all the objects to become ready. + /// Objects that the thread is waiting on, in the same order as they were + // passed to WaitSynchronization1/N. std::vector> wait_objects; - /// Mapping of Object ids to their position in the last waitlist that this object waited on. - boost::container::flat_map wait_objects_index; - VAddr wait_address; ///< If waiting on an AddressArbiter, this is the arbitration address /// True if the WaitSynchronizationN output parameter should be set on thread wakeup. diff --git a/src/core/hle/svc.cpp b/src/core/hle/svc.cpp index 1e1ca5180c..855f3af822 100644 --- a/src/core/hle/svc.cpp +++ b/src/core/hle/svc.cpp @@ -277,6 +277,7 @@ static ResultCode WaitSynchronization1(Kernel::Handle handle, s64 nano_seconds) if (nano_seconds == 0) return ERR_SYNC_TIMEOUT; + thread->wait_objects = {object}; object->AddWaitingThread(thread); thread->status = THREADSTATUS_WAIT_SYNCH_ANY; @@ -325,11 +326,6 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha objects[i] = object; } - // Clear the mapping of wait object indices. - // We don't want any lingering state in this map. - // It will be repopulated later in the wait_all = false case. - thread->wait_objects_index.clear(); - if (wait_all) { bool all_available = std::all_of(objects.begin(), objects.end(), @@ -358,7 +354,6 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha object->AddWaitingThread(thread); } - // Set the thread's waitlist to the list of objects passed to WaitSynchronizationN thread->wait_objects = std::move(objects); // Create an event to wake the thread up after the specified nanosecond delay has passed @@ -395,17 +390,14 @@ static ResultCode WaitSynchronizationN(s32* out, Kernel::Handle* handles, s32 ha // Put the thread to sleep thread->status = THREADSTATUS_WAIT_SYNCH_ANY; - // Clear the thread's waitlist, we won't use it for wait_all = false - thread->wait_objects.clear(); - // Add the thread to each of the objects' waiting threads. for (size_t i = 0; i < objects.size(); ++i) { Kernel::WaitObject* object = objects[i].get(); - // Set the index of this object in the mapping of Objects -> index for this thread. - thread->wait_objects_index[object->GetObjectId()] = static_cast(i); object->AddWaitingThread(thread); } + thread->wait_objects = std::move(objects); + // Note: If no handles and no timeout were given, then the thread will deadlock, this is // consistent with hardware behavior. From 7f1dca8cd26fe9be0080efee4e456630960af459 Mon Sep 17 00:00:00 2001 From: Subv Date: Wed, 4 Jan 2017 11:37:19 -0500 Subject: [PATCH 8/9] Kernel: Remove a thread from all of its waiting objects' waiting_threads list when it is awoken. This fixes a potential bug where threads would not get removed from said list if they awoke after waiting with WaitSynchronizationN with wait_all = false --- src/core/hle/kernel/kernel.cpp | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/src/core/hle/kernel/kernel.cpp b/src/core/hle/kernel/kernel.cpp index 955f50a9bf..47d4df69c3 100644 --- a/src/core/hle/kernel/kernel.cpp +++ b/src/core/hle/kernel/kernel.cpp @@ -32,19 +32,6 @@ void WaitObject::RemoveWaitingThread(Thread* thread) { } SharedPtr WaitObject::GetHighestPriorityReadyThread() { - // Remove the threads that are ready or already running from our waitlist - auto to_remove = waiting_threads.end(); - do { - to_remove = std::find_if(waiting_threads.begin(), waiting_threads.end(), - [](const SharedPtr& thread) { - return thread->status == THREADSTATUS_RUNNING || - thread->status == THREADSTATUS_READY || - thread->status == THREADSTATUS_DEAD; - }); - // Call RemoveWaitingThread so that child classes can override the behavior. - RemoveWaitingThread(to_remove->get()); - } while (to_remove != waiting_threads.end()); - Thread* candidate = nullptr; s32 candidate_priority = THREADPRIO_LOWEST + 1; @@ -86,17 +73,16 @@ void WaitObject::WakeupAllWaitingThreads() { } else { for (auto& object : thread->wait_objects) { object->Acquire(thread.get()); - object->RemoveWaitingThread(thread.get()); } // Note: This case doesn't update the output index of WaitSynchronizationN. - // Clear the thread's waitlist - thread->wait_objects.clear(); } + for (auto& object : thread->wait_objects) + object->RemoveWaitingThread(thread.get()); + thread->wait_objects.clear(); + thread->SetWaitSynchronizationResult(RESULT_SUCCESS); thread->ResumeFromWait(); - // Note: Removing the thread from the object's waitlist will be - // done by GetHighestPriorityReadyThread. } } From dda4ec93bea089e3286e9a965378d9411f480acd Mon Sep 17 00:00:00 2001 From: Subv Date: Wed, 4 Jan 2017 12:48:13 -0500 Subject: [PATCH 9/9] Kernel: Add some asserts to enforce the invariants in the scheduler. --- src/core/hle/kernel/kernel.cpp | 8 ++++++++ src/core/hle/kernel/thread.cpp | 7 +++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/core/hle/kernel/kernel.cpp b/src/core/hle/kernel/kernel.cpp index 47d4df69c3..f599916f0d 100644 --- a/src/core/hle/kernel/kernel.cpp +++ b/src/core/hle/kernel/kernel.cpp @@ -27,6 +27,9 @@ void WaitObject::AddWaitingThread(SharedPtr thread) { void WaitObject::RemoveWaitingThread(Thread* thread) { auto itr = std::find(waiting_threads.begin(), waiting_threads.end(), thread); + // If a thread passed multiple handles to the same object, + // the kernel might attempt to remove the thread from the object's + // waiting threads list multiple times. if (itr != waiting_threads.end()) waiting_threads.erase(itr); } @@ -36,6 +39,11 @@ SharedPtr WaitObject::GetHighestPriorityReadyThread() { s32 candidate_priority = THREADPRIO_LOWEST + 1; for (const auto& thread : waiting_threads) { + // The list of waiting threads must not contain threads that are not waiting to be awakened. + ASSERT_MSG(thread->status == THREADSTATUS_WAIT_SYNCH_ANY || + thread->status == THREADSTATUS_WAIT_SYNCH_ALL, + "Inconsistent thread statuses in waiting_threads"); + if (thread->current_priority >= candidate_priority) continue; diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index 568cef5b91..9109bd10ba 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -200,8 +200,8 @@ static void SwitchContext(Thread* new_thread) { // Load context of new thread if (new_thread) { - DEBUG_ASSERT_MSG(new_thread->status == THREADSTATUS_READY, - "Thread must be ready to become running."); + ASSERT_MSG(new_thread->status == THREADSTATUS_READY, + "Thread must be ready to become running."); // Cancel any outstanding wakeup events for this thread CoreTiming::UnscheduleEvent(ThreadWakeupEventType, new_thread->callback_handle); @@ -307,6 +307,8 @@ void Thread::WakeAfterDelay(s64 nanoseconds) { } void Thread::ResumeFromWait() { + ASSERT_MSG(wait_objects.empty(), "Thread is waking up while waiting for objects"); + switch (status) { case THREADSTATUS_WAIT_SYNCH_ALL: case THREADSTATUS_WAIT_SYNCH_ANY: @@ -580,6 +582,7 @@ void Thread::SetWaitSynchronizationOutput(s32 output) { } s32 Thread::GetWaitObjectIndex(WaitObject* object) const { + ASSERT_MSG(!wait_objects.empty(), "Thread is not waiting for anything"); auto match = std::find(wait_objects.rbegin(), wait_objects.rend(), object); return std::distance(match, wait_objects.rend()) - 1; }