From 8b4380a0685d300f7fa3e3db5367f9caa00fe437 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Fri, 15 Mar 2019 01:02:13 -0400 Subject: [PATCH 1/4] kernel/thread: Maintain priority ordering of added mutex waiting threads The kernel keeps the internal waiting list ordered by priority. This is trivial to do with std::find_if followed by an insertion. --- src/core/hle/kernel/thread.cpp | 36 ++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index eb54d66516..7706ca9e53 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -7,8 +7,6 @@ #include #include -#include - #include "common/assert.h" #include "common/common_types.h" #include "common/logging/log.h" @@ -269,8 +267,8 @@ void Thread::AddMutexWaiter(SharedPtr thread) { if (thread->lock_owner == this) { // If the thread is already waiting for this thread to release the mutex, ensure that the // waiters list is consistent and return without doing anything. - auto itr = std::find(wait_mutex_threads.begin(), wait_mutex_threads.end(), thread); - ASSERT(itr != wait_mutex_threads.end()); + const auto iter = std::find(wait_mutex_threads.begin(), wait_mutex_threads.end(), thread); + ASSERT(iter != wait_mutex_threads.end()); return; } @@ -278,11 +276,16 @@ void Thread::AddMutexWaiter(SharedPtr thread) { ASSERT(thread->lock_owner == nullptr); // Ensure that the thread is not already in the list of mutex waiters - auto itr = std::find(wait_mutex_threads.begin(), wait_mutex_threads.end(), thread); - ASSERT(itr == wait_mutex_threads.end()); + const auto iter = std::find(wait_mutex_threads.begin(), wait_mutex_threads.end(), thread); + ASSERT(iter == wait_mutex_threads.end()); + // Keep the list in an ordered fashion + const auto insertion_point = std::find_if( + wait_mutex_threads.begin(), wait_mutex_threads.end(), + [&thread](const auto& entry) { return entry->GetPriority() > thread->GetPriority(); }); + wait_mutex_threads.insert(insertion_point, thread); thread->lock_owner = this; - wait_mutex_threads.emplace_back(std::move(thread)); + UpdatePriority(); } @@ -290,10 +293,11 @@ void Thread::RemoveMutexWaiter(SharedPtr thread) { ASSERT(thread->lock_owner == this); // Ensure that the thread is in the list of mutex waiters - auto itr = std::find(wait_mutex_threads.begin(), wait_mutex_threads.end(), thread); - ASSERT(itr != wait_mutex_threads.end()); + const auto iter = std::find(wait_mutex_threads.begin(), wait_mutex_threads.end(), thread); + ASSERT(iter != wait_mutex_threads.end()); + + wait_mutex_threads.erase(iter); - boost::remove_erase(wait_mutex_threads, thread); thread->lock_owner = nullptr; UpdatePriority(); } @@ -310,12 +314,18 @@ void Thread::UpdatePriority() { return; scheduler->SetThreadPriority(this, new_priority); - current_priority = new_priority; + if (!lock_owner) { + return; + } + + // Ensure that the thread is within the correct location in the waiting list. + lock_owner->RemoveMutexWaiter(this); + lock_owner->AddMutexWaiter(this); + // Recursively update the priority of the thread that depends on the priority of this one. - if (lock_owner) - lock_owner->UpdatePriority(); + lock_owner->UpdatePriority(); } void Thread::ChangeCore(u32 core, u64 mask) { From f68087ce1251adce4860c4afb254d57aa38c22d1 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 14 Mar 2019 21:47:46 -0400 Subject: [PATCH 2/4] kernel/thread: Amend condition within UpdatePriority() This condition was checking against the nominal thread priority, whereas the kernel itself checks against the current priority instead. We were also assigning the nominal priority, when we should be assigning current_priority, which takes priority inheritance into account. This can lead to the incorrect priority being assigned to a thread. Given we recursively update the relevant threads, we don't need to go through the whole mutex waiter list. This matches what the kernel does as well (only accessing the first entry within the waiting list). --- src/core/hle/kernel/thread.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index 7706ca9e53..4b68b555f8 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -305,9 +305,9 @@ void Thread::RemoveMutexWaiter(SharedPtr thread) { void Thread::UpdatePriority() { // Find the highest priority among all the threads that are waiting for this thread's lock u32 new_priority = nominal_priority; - for (const auto& thread : wait_mutex_threads) { - if (thread->nominal_priority < new_priority) - new_priority = thread->nominal_priority; + if (!wait_mutex_threads.empty()) { + if (wait_mutex_threads.front()->current_priority < new_priority) + new_priority = wait_mutex_threads.front()->current_priority; } if (new_priority == current_priority) From c92e8f4e3068baaf22728a4e85340880842baf4b Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 14 Mar 2019 21:51:03 -0400 Subject: [PATCH 3/4] kernel/thread: Make bracing consistent within UpdatePriority() --- src/core/hle/kernel/thread.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index 4b68b555f8..c5cee12dd3 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -306,12 +306,14 @@ void Thread::UpdatePriority() { // Find the highest priority among all the threads that are waiting for this thread's lock u32 new_priority = nominal_priority; if (!wait_mutex_threads.empty()) { - if (wait_mutex_threads.front()->current_priority < new_priority) + if (wait_mutex_threads.front()->current_priority < new_priority) { new_priority = wait_mutex_threads.front()->current_priority; + } } - if (new_priority == current_priority) + if (new_priority == current_priority) { return; + } scheduler->SetThreadPriority(this, new_priority); current_priority = new_priority; From 793d3102c28778c3557630068549b336ce3cae17 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 14 Mar 2019 21:58:23 -0400 Subject: [PATCH 4/4] kernel/thread: Expand documentation of nominal_priority and current_priority Aims to disambiguate why each priority instance exists a little bit. While we're at it, also add an explanatory comment to UpdatePriority(). --- src/core/hle/kernel/thread.cpp | 4 +++- src/core/hle/kernel/thread.h | 10 ++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/core/hle/kernel/thread.cpp b/src/core/hle/kernel/thread.cpp index c5cee12dd3..202997d204 100644 --- a/src/core/hle/kernel/thread.cpp +++ b/src/core/hle/kernel/thread.cpp @@ -303,7 +303,9 @@ void Thread::RemoveMutexWaiter(SharedPtr thread) { } void Thread::UpdatePriority() { - // Find the highest priority among all the threads that are waiting for this thread's lock + // If any of the threads waiting on the mutex have a higher priority + // (taking into account priority inheritance), then this thread inherits + // that thread's priority. u32 new_priority = nominal_priority; if (!wait_mutex_threads.empty()) { if (wait_mutex_threads.front()->current_priority < new_priority) { diff --git a/src/core/hle/kernel/thread.h b/src/core/hle/kernel/thread.h index c48b21abab..96d9982d8e 100644 --- a/src/core/hle/kernel/thread.h +++ b/src/core/hle/kernel/thread.h @@ -398,8 +398,14 @@ private: VAddr entry_point = 0; VAddr stack_top = 0; - u32 nominal_priority = 0; ///< Nominal thread priority, as set by the emulated application - u32 current_priority = 0; ///< Current thread priority, can be temporarily changed + /// Nominal thread priority, as set by the emulated application. + /// The nominal priority is the thread priority without priority + /// inheritance taken into account. + u32 nominal_priority = 0; + + /// Current thread priority. This may change over the course of the + /// thread's lifetime in order to facilitate priority inheritance. + u32 current_priority = 0; u64 total_cpu_time_ticks = 0; ///< Total CPU running ticks. u64 last_running_ticks = 0; ///< CPU tick when thread was last running