From 0ea02f2d56eb55eb8eabdf6884a61787659f7dc5 Mon Sep 17 00:00:00 2001
From: PixelyIon <pixelyion@protonmail.com>
Date: Tue, 2 Mar 2021 15:19:13 +0530
Subject: [PATCH] Fix Non-Cooperative Core Migration + Fix
 `yieldWithCoreMigration` + Improve Mutex Locking in `ConditionalVariableWait`

The case of a thread not being in the core queue during a non-cooperative core affinity change would break things as the thread was non-conditionally removed and inserted, this has been fixed by adding a check to see if the thread exists in the core's queue prior to migration. In addition, `yieldWithCoreMigration` was broken by the previous commit as the fallthrough was intentional and removing it cause core migration without a yield which led to breakage in certain circumstances. The mutex locking logic was also improved in `ConditionalVariableWait` to use atomics in a more effective manner with less atomic operations being performed overall.
---
 .idea/inspectionProfiles/Project_Default.xml  |  2 +-
 app/src/main/cpp/skyline/kernel/scheduler.cpp | 44 +++++++++++--------
 app/src/main/cpp/skyline/kernel/scheduler.h   |  6 +++
 app/src/main/cpp/skyline/kernel/svc.cpp       | 10 ++---
 .../cpp/skyline/kernel/types/KProcess.cpp     | 24 +++++-----
 5 files changed, 47 insertions(+), 39 deletions(-)

diff --git a/.idea/inspectionProfiles/Project_Default.xml b/.idea/inspectionProfiles/Project_Default.xml
index ef2d25b4..dfe4c22f 100644
--- a/.idea/inspectionProfiles/Project_Default.xml
+++ b/.idea/inspectionProfiles/Project_Default.xml
@@ -171,7 +171,7 @@
     </inspection_tool>
     <inspection_tool class="CheckedExceptionClass" enabled="true" level="WARNING" enabled_by_default="true" />
     <inspection_tool class="ClangTidy" enabled="true" level="WARNING" enabled_by_default="true">
-      <option name="clangTidyChecks" value="-*,bugprone-argument-comment,bugprone-assert-side-effect,bugprone-bad-signal-to-kill-thread,bugprone-branch-clone,bugprone-copy-constructor-init,bugprone-dangling-handle,bugprone-dynamic-static-initializers,bugprone-fold-init-type,bugprone-forward-declaration-namespace,bugprone-forwarding-reference-overload,bugprone-inaccurate-erase,bugprone-incorrect-roundings,bugprone-integer-division,bugprone-lambda-function-name,bugprone-macro-parentheses,bugprone-macro-repeated-side-effects,bugprone-misplaced-operator-in-strlen-in-alloc,bugprone-misplaced-pointer-arithmetic-in-alloc,bugprone-misplaced-widening-cast,bugprone-move-forwarding-reference,bugprone-multiple-statement-macro,bugprone-no-escape,bugprone-not-null-terminated-result,bugprone-parent-virtual-call,bugprone-posix-return,bugprone-reserved-identifier,bugprone-sizeof-container,bugprone-sizeof-expression,bugprone-spuriously-wake-up-functions,bugprone-string-constructor,bugprone-string-integer-assignment,bugprone-string-literal-with-embedded-nul,bugprone-suspicious-enum-usage,bugprone-suspicious-include,bugprone-suspicious-memset-usage,bugprone-suspicious-missing-comma,bugprone-suspicious-semicolon,bugprone-suspicious-string-compare,bugprone-swapped-arguments,bugprone-terminating-continue,bugprone-throw-keyword-missing,bugprone-too-small-loop-variable,bugprone-undefined-memory-manipulation,bugprone-undelegated-constructor,bugprone-unhandled-self-assignment,bugprone-unused-raii,bugprone-unused-return-value,bugprone-use-after-move,bugprone-virtual-near-miss,cert-dcl21-cpp,cert-dcl58-cpp,cert-err34-c,cert-err52-cpp,cert-err58-cpp,cert-err60-cpp,cert-flp30-c,cert-msc50-cpp,cert-msc51-cpp,cert-str34-c,cppcoreguidelines-interfaces-global-init,cppcoreguidelines-narrowing-conversions,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-pro-type-static-cast-downcast,cppcoreguidelines-slicing,google-default-arguments,google-explicit-constructor,google-runtime-operator,hicpp-exception-baseclass,hicpp-multiway-paths-covered,misc-misplaced-const,misc-new-delete-overloads,misc-no-recursion,misc-non-copyable-objects,misc-throw-by-value-catch-by-reference,misc-unconventional-assign-operator,misc-uniqueptr-reset-release,modernize-avoid-bind,modernize-concat-nested-namespaces,modernize-deprecated-headers,modernize-deprecated-ios-base-aliases,modernize-loop-convert,modernize-make-shared,modernize-make-unique,modernize-pass-by-value,modernize-raw-string-literal,modernize-redundant-void-arg,modernize-replace-auto-ptr,modernize-replace-disallow-copy-and-assign-macro,modernize-replace-random-shuffle,modernize-return-braced-init-list,modernize-shrink-to-fit,modernize-unary-static-assert,modernize-use-auto,modernize-use-bool-literals,modernize-use-emplace,modernize-use-equals-default,modernize-use-equals-delete,modernize-use-nodiscard,modernize-use-noexcept,modernize-use-nullptr,modernize-use-override,modernize-use-transparent-functors,modernize-use-uncaught-exceptions,mpi-buffer-deref,mpi-type-mismatch,openmp-use-default-none,performance-faster-string-find,performance-for-range-copy,performance-implicit-conversion-in-loop,performance-inefficient-algorithm,performance-inefficient-string-concatenation,performance-inefficient-vector-operation,performance-move-const-arg,performance-move-constructor-init,performance-no-automatic-move,performance-noexcept-move-constructor,performance-trivially-destructible,performance-type-promotion-in-math-fn,performance-unnecessary-copy-initialization,performance-unnecessary-value-param,portability-simd-intrinsics,readability-avoid-const-params-in-decls,readability-const-return-type,readability-container-size-empty,readability-convert-member-functions-to-static,readability-delete-null-pointer,readability-deleted-default,readability-inconsistent-declaration-parameter-name,readability-make-member-function-const,readability-misleading-indentation,readability-misplaced-array-index,readability-redundant-control-flow,readability-redundant-declaration,readability-redundant-function-ptr-dereference,readability-redundant-smartptr-get,readability-redundant-string-cstr,readability-redundant-string-init,readability-simplify-subscript-expr,readability-static-accessed-through-instance,readability-static-definition-in-anonymous-namespace,readability-string-compare,readability-uniqueptr-delete-release,readability-use-anyofallof" />
+      <option name="clangTidyChecks" value="-*,bugprone-argument-comment,bugprone-assert-side-effect,bugprone-bad-signal-to-kill-thread,bugprone-branch-clone,bugprone-copy-constructor-init,bugprone-dangling-handle,bugprone-dynamic-static-initializers,bugprone-fold-init-type,bugprone-forward-declaration-namespace,bugprone-forwarding-reference-overload,bugprone-inaccurate-erase,bugprone-incorrect-roundings,bugprone-integer-division,bugprone-lambda-function-name,bugprone-macro-parentheses,bugprone-macro-repeated-side-effects,bugprone-misplaced-operator-in-strlen-in-alloc,bugprone-misplaced-pointer-arithmetic-in-alloc,bugprone-misplaced-widening-cast,bugprone-move-forwarding-reference,bugprone-multiple-statement-macro,bugprone-no-escape,bugprone-not-null-terminated-result,bugprone-parent-virtual-call,bugprone-posix-return,bugprone-reserved-identifier,bugprone-sizeof-container,bugprone-sizeof-expression,bugprone-spuriously-wake-up-functions,bugprone-string-constructor,bugprone-string-integer-assignment,bugprone-string-literal-with-embedded-nul,bugprone-suspicious-enum-usage,bugprone-suspicious-include,bugprone-suspicious-memset-usage,bugprone-suspicious-missing-comma,bugprone-suspicious-semicolon,bugprone-suspicious-string-compare,bugprone-swapped-arguments,bugprone-terminating-continue,bugprone-throw-keyword-missing,bugprone-too-small-loop-variable,bugprone-undefined-memory-manipulation,bugprone-undelegated-constructor,bugprone-unhandled-self-assignment,bugprone-unused-raii,bugprone-unused-return-value,bugprone-use-after-move,bugprone-virtual-near-miss,cert-dcl21-cpp,cert-dcl58-cpp,cert-err34-c,cert-err52-cpp,cert-err58-cpp,cert-err60-cpp,cert-flp30-c,cert-msc50-cpp,cert-msc51-cpp,cppcoreguidelines-interfaces-global-init,cppcoreguidelines-narrowing-conversions,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-pro-type-static-cast-downcast,cppcoreguidelines-slicing,google-default-arguments,google-explicit-constructor,google-runtime-operator,hicpp-exception-baseclass,hicpp-multiway-paths-covered,misc-misplaced-const,misc-new-delete-overloads,misc-no-recursion,misc-non-copyable-objects,misc-throw-by-value-catch-by-reference,misc-unconventional-assign-operator,misc-uniqueptr-reset-release,modernize-avoid-bind,modernize-concat-nested-namespaces,modernize-deprecated-headers,modernize-deprecated-ios-base-aliases,modernize-loop-convert,modernize-make-shared,modernize-make-unique,modernize-pass-by-value,modernize-raw-string-literal,modernize-redundant-void-arg,modernize-replace-auto-ptr,modernize-replace-disallow-copy-and-assign-macro,modernize-replace-random-shuffle,modernize-return-braced-init-list,modernize-shrink-to-fit,modernize-unary-static-assert,modernize-use-auto,modernize-use-bool-literals,modernize-use-emplace,modernize-use-equals-default,modernize-use-equals-delete,modernize-use-nodiscard,modernize-use-noexcept,modernize-use-nullptr,modernize-use-override,modernize-use-transparent-functors,modernize-use-uncaught-exceptions,mpi-buffer-deref,mpi-type-mismatch,openmp-use-default-none,performance-faster-string-find,performance-for-range-copy,performance-implicit-conversion-in-loop,performance-inefficient-algorithm,performance-inefficient-string-concatenation,performance-inefficient-vector-operation,performance-move-const-arg,performance-move-constructor-init,performance-no-automatic-move,performance-noexcept-move-constructor,performance-trivially-destructible,performance-type-promotion-in-math-fn,performance-unnecessary-copy-initialization,performance-unnecessary-value-param,portability-simd-intrinsics,readability-avoid-const-params-in-decls,readability-const-return-type,readability-container-size-empty,readability-convert-member-functions-to-static,readability-delete-null-pointer,readability-deleted-default,readability-inconsistent-declaration-parameter-name,readability-make-member-function-const,readability-misleading-indentation,readability-misplaced-array-index,readability-non-const-parameter,readability-redundant-control-flow,readability-redundant-declaration,readability-redundant-function-ptr-dereference,readability-redundant-smartptr-get,readability-redundant-string-cstr,readability-redundant-string-init,readability-simplify-subscript-expr,readability-static-accessed-through-instance,readability-static-definition-in-anonymous-namespace,readability-string-compare,readability-uniqueptr-delete-release,readability-use-anyofallof" />
     </inspection_tool>
     <inspection_tool class="ClassComplexity" enabled="true" level="WARNING" enabled_by_default="true">
       <option name="m_limit" value="80" />
diff --git a/app/src/main/cpp/skyline/kernel/scheduler.cpp b/app/src/main/cpp/skyline/kernel/scheduler.cpp
index 54d73ff7..b5494971 100644
--- a/app/src/main/cpp/skyline/kernel/scheduler.cpp
+++ b/app/src/main/cpp/skyline/kernel/scheduler.cpp
@@ -120,6 +120,27 @@ namespace skyline::kernel {
         }
     }
 
+    void Scheduler::MigrateToIdealCore(const std::shared_ptr<type::KThread> &thread, CoreContext *&core, std::unique_lock<std::mutex> &lock) {
+        // We need to check if the thread was in it's resident core's queue
+        // If it was, we need to remove it from the queue
+        auto it{std::find(core->queue.begin(), core->queue.end(), thread)};
+        bool wasInserted{it != core->queue.end()};
+        if (wasInserted) {
+            it = core->queue.erase(it);
+            if (it == core->queue.begin() && it != core->queue.end())
+                (*it)->wakeCondition.notify_one();
+        }
+        lock.unlock();
+
+        thread->coreId = thread->idealCore;
+        if (wasInserted)
+            // We need to add the thread to the ideal core queue, if it was previously it's resident core's queue
+            InsertThread(thread);
+
+        core = &cores.at(thread->coreId);
+        lock = std::unique_lock(core->mutex);
+    }
+
     void Scheduler::WaitSchedule(bool loadBalance) {
         auto &thread{state.thread};
         CoreContext *core{&cores.at(thread->coreId)};
@@ -127,14 +148,7 @@ namespace skyline::kernel {
 
         auto wakeFunction{[&]() {
             if (!thread->affinityMask.test(thread->coreId)) [[unlikely]] {
-                lock.unlock();
-
-                RemoveThread();
-                thread->coreId = thread->idealCore;
-                InsertThread(thread);
-
-                core = &cores.at(thread->coreId);
-                lock = std::unique_lock(core->mutex);
+                MigrateToIdealCore(thread, core, lock);
             }
             return !core->queue.empty() && core->queue.front() == thread;
         }};
@@ -173,14 +187,7 @@ namespace skyline::kernel {
         std::unique_lock lock(core->mutex);
         if (thread->wakeCondition.wait_for(lock, timeout, [&]() {
             if (!thread->affinityMask.test(thread->coreId)) [[unlikely]] {
-                lock.unlock();
-
-                RemoveThread();
-                thread->coreId = thread->idealCore;
-                InsertThread(thread);
-
-                core = &cores.at(thread->coreId);
-                lock = std::unique_lock(core->mutex);
+                MigrateToIdealCore(thread, core, lock);
             }
             return !core->queue.empty() && core->queue.front() == thread;
         })) {
@@ -212,9 +219,8 @@ namespace skyline::kernel {
             auto &front{core.queue.front()};
             if (front != thread)
                 front->wakeCondition.notify_one(); // If we aren't at the front of the queue, only then should we wake the thread at the front up
-        } else if (!thread->forceYield) {
-            [[unlikely]]
-                throw exception("T{} called Rotate while not being in C{}'s queue", thread->id, thread->coreId);
+        } else if (!thread->forceYield) [[unlikely]] {
+            throw exception("T{} called Rotate while not being in C{}'s queue", thread->id, thread->coreId);
         }
 
         thread->averageTimeslice = (thread->averageTimeslice / 4) + (3 * (util::GetTimeTicks() - thread->timesliceStart / 4));
diff --git a/app/src/main/cpp/skyline/kernel/scheduler.h b/app/src/main/cpp/skyline/kernel/scheduler.h
index 81d95203..f355b5c9 100644
--- a/app/src/main/cpp/skyline/kernel/scheduler.h
+++ b/app/src/main/cpp/skyline/kernel/scheduler.h
@@ -57,6 +57,12 @@ namespace skyline {
             std::mutex parkedMutex; //!< Synchronizes all operations on the queue of parked threads
             std::list<std::shared_ptr<type::KThread>> parkedQueue; //!< A queue of threads which are parked and waiting on core migration
 
+            /**
+             * @brief Migrate a thread from it's resident core to it's ideal core
+             * @note This is used to handle non-cooperative core affinity mask changes where the resident core is not in it's new affinity mask
+             */
+            void MigrateToIdealCore(const std::shared_ptr<type::KThread>& thread, CoreContext*& core, std::unique_lock<std::mutex>& lock);
+
           public:
             static constexpr std::chrono::milliseconds PreemptiveTimeslice{10}; //!< The duration of time a preemptive thread can run before yielding
             inline static int YieldSignal{SIGRTMIN}; //!< The signal used to cause a yield in running threads
diff --git a/app/src/main/cpp/skyline/kernel/svc.cpp b/app/src/main/cpp/skyline/kernel/svc.cpp
index 10eff2b6..8b207b4c 100644
--- a/app/src/main/cpp/skyline/kernel/svc.cpp
+++ b/app/src/main/cpp/skyline/kernel/svc.cpp
@@ -300,12 +300,12 @@ namespace skyline::kernel::svc {
         } else {
             switch (in) {
                 case yieldWithCoreMigration:
-                    state.logger->Debug("svcSleepThread: Waking any appropriate parked threads");
+                    state.logger->Debug("svcSleepThread: Waking any appropriate parked threads and yielding");
                     state.scheduler->WakeParkedThread();
-                    break;
-
+                    [[fallthrough]];
                 case yieldWithoutCoreMigration:
-                    state.logger->Debug("svcSleepThread: Cooperative Yield");
+                    if (in == yieldWithoutCoreMigration)
+                        state.logger->Debug("svcSleepThread: Cooperative yield");
                     state.scheduler->Rotate();
                     state.scheduler->WaitSchedule();
                     break;
@@ -710,7 +710,7 @@ namespace skyline::kernel::svc {
         else if (result == result::InvalidCurrentMemory)
             result = Result{}; // If the mutex value isn't expected then it's still successful
         else if (result == result::InvalidHandle)
-            state.logger->Warn("svcArbitrateLock: 'ownerHandle' invalid: 0x{:X}", ownerHandle);
+            state.logger->Warn("svcArbitrateLock: 'ownerHandle' invalid: 0x{:X} (0x{:X})", ownerHandle, mutex);
 
         state.ctx->gpr.w0 = result;
     }
diff --git a/app/src/main/cpp/skyline/kernel/types/KProcess.cpp b/app/src/main/cpp/skyline/kernel/types/KProcess.cpp
index 41097463..d731047a 100644
--- a/app/src/main/cpp/skyline/kernel/types/KProcess.cpp
+++ b/app/src/main/cpp/skyline/kernel/types/KProcess.cpp
@@ -264,16 +264,14 @@ namespace skyline::kernel::type {
             state.scheduler->WaitSchedule(false);
         }
 
-        while (true) {
-            KHandle value{};
-            if (__atomic_compare_exchange_n(mutex, &value, tag, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST))
-                return {};
-            if (!(value & HandleWaitersBit))
-                if (!__atomic_compare_exchange_n(mutex, &value, value | HandleWaitersBit, false, __ATOMIC_SEQ_CST, __ATOMIC_RELAXED))
-                    continue;
-            if (MutexLock(mutex, value & ~HandleWaitersBit, tag) == Result{})
-                return {};
-        }
+        KHandle value{};
+        if (!__atomic_compare_exchange_n(mutex, &value, tag, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST))
+            while (MutexLock(mutex, value & ~HandleWaitersBit, tag) != Result{})
+                if ((value = __atomic_or_fetch(mutex, HandleWaitersBit, __ATOMIC_SEQ_CST)) == HandleWaitersBit)
+                    if (__atomic_compare_exchange_n(mutex, &value, tag, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST))
+                        break;
+
+        return {};
     }
 
     void KProcess::ConditionalVariableSignal(u32 *key, i32 amount) {
@@ -281,7 +279,6 @@ namespace skyline::kernel::type {
         auto queue{syncWaiters.equal_range(key)};
 
         auto it{queue.first};
-        if (queue.first != queue.second)
             for (i32 waiterCount{amount}; it != queue.second && (amount <= 0 || waiterCount); it = syncWaiters.erase(it), waiterCount--)
                 state.scheduler->InsertThread(it->second);
 
@@ -330,9 +327,8 @@ namespace skyline::kernel::type {
                 return result::InvalidState;
 
         i32 waiterCount{amount};
-        if (queue.first != queue.second)
-            for (auto it{queue.first}; it != queue.second && (amount <= 0 || waiterCount); it = syncWaiters.erase(it), waiterCount--)
-                state.scheduler->InsertThread(it->second);
+        for (auto it{queue.first}; it != queue.second && (amount <= 0 || waiterCount); it = syncWaiters.erase(it), waiterCount--)
+            state.scheduler->InsertThread(it->second);
 
         return {};
     }