From 90570c153b43614af4f5cd296bb3c400178cdd47 Mon Sep 17 00:00:00 2001 From: Subv Date: Sun, 1 Jan 2017 16:53:22 -0500 Subject: 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/thread.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/core/hle/kernel/thread.cpp') 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 -- cgit v1.2.3 From 4e6369d41d72fd4b57f136a6124dcf679ef44702 Mon Sep 17 00:00:00 2001 From: Subv Date: Sun, 1 Jan 2017 16:59:30 -0500 Subject: 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(-) (limited to 'src/core/hle/kernel/thread.cpp') 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 -- cgit v1.2.3 From 8503e5668920848b91fe02a0de504d608078d9a8 Mon Sep 17 00:00:00 2001 From: Subv Date: Mon, 2 Jan 2017 19:38:08 -0500 Subject: 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(-) (limited to 'src/core/hle/kernel/thread.cpp') 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; } -- cgit v1.2.3 From bcad1d6f12d32287ed4479c32409a98368f2138d Mon Sep 17 00:00:00 2001 From: Subv Date: Wed, 4 Jan 2017 10:44:38 -0500 Subject: 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(-) (limited to 'src/core/hle/kernel/thread.cpp') 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(); -- cgit v1.2.3 From 3ce2c24665a0b0c020da56fbc882c920bf19f118 Mon Sep 17 00:00:00 2001 From: Subv Date: Wed, 4 Jan 2017 10:53:01 -0500 Subject: 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(-) (limited to 'src/core/hle/kernel/thread.cpp') 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. -- cgit v1.2.3 From a0ae8f7c269e352e964ec0c66f064a3bf6cddddb Mon Sep 17 00:00:00 2001 From: Subv Date: Wed, 4 Jan 2017 12:48:13 -0500 Subject: 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(-) (limited to 'src/core/hle/kernel/thread.cpp') 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; } -- cgit v1.2.3