From c282276b74e253a5b0496d60072b02fbda061c73 Mon Sep 17 00:00:00 2001 From: PixelyIon Date: Fri, 5 Mar 2021 14:36:41 +0530 Subject: [PATCH] Address CR Comments 2 (#132) + Fix Several Scheduler Bugs The following scheduler bugs were fixed: * It was assumed that all non-cooperative `Rotate` calls were from a preemptive yield and changed the state of `KThread::isPreempted` incorrectly which could lead to UB, an example of a scenario with it would be: * * Preemptive thread A gets a signal to yield from cooperative thread B due to it being ready to schedule and higher priority * * A complies with this request but there's an assumption that the signal was actually from it's preemption timer therefore it doesn't reset it (As it isn't required if the timer was responsible for the signal) * * A receives the actual preemption signal a while later, causing UB as the signal handler is invoked twice * `Scheduler::UpdatePriority` * * A check for `currentIt == core->queue.begin()` existed which caused an incorrect early return * * The preemption timer was armed correctly when a priority transition from cooperative priority -> preemption priority occurred but not disarmed when a transition from preemption priority -> cooperative priority occurred * * The timer was unnecessarily disarmed in the case of updating the priority of a non-running thread, this isn't as much a bug as it is just pointless * Priority inheritance in `KProcess::MutexLock` is fundamentally broken as it performs UB with `waitThread` being accessed prior to being assigned * When a thread sets its own priority using `SvcSetThreadCoreMask` and its current core is no longer in the affinity mask, it wouldn't actually move to the new thread until the next time the thread is load balanced --- app/src/main/cpp/skyline/kernel/scheduler.cpp | 120 +++++------------- app/src/main/cpp/skyline/kernel/scheduler.h | 7 +- app/src/main/cpp/skyline/kernel/svc.cpp | 8 +- .../cpp/skyline/kernel/types/KProcess.cpp | 33 +---- .../main/cpp/skyline/kernel/types/KThread.cpp | 53 +++++++- .../main/cpp/skyline/kernel/types/KThread.h | 18 ++- app/src/main/cpp/skyline/loader/loader.cpp | 8 +- 7 files changed, 118 insertions(+), 129 deletions(-) diff --git a/app/src/main/cpp/skyline/kernel/scheduler.cpp b/app/src/main/cpp/skyline/kernel/scheduler.cpp index 117710b7..75d2d62a 100644 --- a/app/src/main/cpp/skyline/kernel/scheduler.cpp +++ b/app/src/main/cpp/skyline/kernel/scheduler.cpp @@ -14,6 +14,8 @@ namespace skyline::kernel { void Scheduler::SignalHandler(int signal, siginfo *info, ucontext *ctx, void **tls) { if (*tls) { const auto &state{*reinterpret_cast(*tls)->state}; + if (signal == PreemptionSignal) + state.thread->isPreempted = false; state.scheduler->Rotate(false); YieldPending = false; state.scheduler->WaitSchedule(); @@ -64,33 +66,10 @@ namespace skyline::kernel { } } - if (optimalCore != currentCore) { - /* - bool isInserted; - if (conditionalInsert) { - auto it{std::find(currentCore->queue.begin(), currentCore->queue.end(), thread)}; - isInserted = it != currentCore->queue.end(); - if (isInserted && thread == state.thread) { - // If the thread is in it's current core's queue - // We need to remove the thread from the current core's queue - it = currentCore->queue.erase(it); - if (it == currentCore->queue.begin() && it != currentCore->queue.end()) - (*it)->scheduleCondition.notify_one(); - } else if (isInserted && thread != state.thread) [[unlikely]] { - // It's not very useful to insert an external thread on the core optimal for it - // We leave any sort of load balancing to a thread to do on it's own - // Our systems can support it but it's pointless to do and would waste precious CPU cycles - throw exception("Migrating an external thread (T{}) which is potentially inserted in its resident core's queue isn't supported", thread->id); - } - } else { - isInserted = false; - } - thread->coreId = optimalCore->id; - */ + if (optimalCore != currentCore) state.logger->Debug("Load Balancing T{}: C{} -> C{}", thread->id, currentCore->id, optimalCore->id); - } else { + else state.logger->Debug("Load Balancing T{}: C{} (Late)", thread->id, currentCore->id); - } return *optimalCore; } @@ -115,7 +94,7 @@ namespace skyline::kernel { core.queue.push_front(thread); if (state.thread != front) { - // If the inserting thread isn't at the front, we need to send it an OS signal to yield + // If the calling thread isn't at the front, we need to send it an OS signal to yield if (!front->pendingYield) { // We only want to yield the thread if it hasn't already been sent a signal to yield in the past // Not doing this can lead to races and deadlocks but is also slower as it prevents redundant signals @@ -123,8 +102,8 @@ namespace skyline::kernel { front->pendingYield = true; } } else { - // If the thread at the front is being yielded, we can just set the YieldPending flag - // This avoids an OS signal and would cause a deadlock otherwise as the core lock would be relocked + // If the calling thread at the front is being yielded, we can just set the YieldPending flag + // This avoids an OS signal which would just flip the YieldPending flag but with significantly more overhead YieldPending = true; } } else { @@ -187,12 +166,9 @@ namespace skyline::kernel { thread->scheduleCondition.wait(lock, wakeFunction); } - if (thread->priority == core->preemptionPriority) { + if (thread->priority == core->preemptionPriority) // If the thread needs to be preempted then arm its preemption timer - struct itimerspec spec{.it_value = {.tv_nsec = std::chrono::duration_cast(PreemptiveTimeslice).count()}}; - timer_settime(thread->preemptionTimer, 0, &spec, nullptr); - thread->isPreempted = true; - } + thread->ArmPreemptionTimer(PreemptiveTimeslice); thread->timesliceStart = util::GetTimeTicks(); } @@ -209,11 +185,8 @@ namespace skyline::kernel { } return !core->queue.empty() && core->queue.front() == thread; })) { - if (thread->priority == core->preemptionPriority) { - struct itimerspec spec{.it_value = {.tv_nsec = std::chrono::duration_cast(PreemptiveTimeslice).count()}}; - timer_settime(thread->preemptionTimer, 0, &spec, nullptr); - thread->isPreempted = true; - } + if (thread->priority == core->preemptionPriority) + thread->ArmPreemptionTimer(PreemptiveTimeslice); thread->timesliceStart = util::GetTimeTicks(); @@ -243,13 +216,7 @@ namespace skyline::kernel { thread->averageTimeslice = (thread->averageTimeslice / 4) + (3 * (util::GetTimeTicks() - thread->timesliceStart / 4)); - if (cooperative && thread->isPreempted) { - // If a preemptive thread did a cooperative yield then we need to disarm the preemptive timer - struct itimerspec spec{}; - timer_settime(thread->preemptionTimer, 0, &spec, nullptr); - } - - thread->isPreempted = false; + thread->DisarmPreemptionTimer(); // If a preemptive thread did a cooperative yield then we need to disarm the preemptive timer thread->pendingYield = false; thread->forceYield = false; } @@ -273,12 +240,9 @@ namespace skyline::kernel { } } - if (thread->isPreempted) { - struct itimerspec spec{}; - timer_settime(thread->preemptionTimer, 0, &spec, nullptr); - thread->isPreempted = false; - } - + thread->DisarmPreemptionTimer(); + thread->pendingYield = false; + thread->forceYield = false; YieldPending = false; } @@ -287,50 +251,36 @@ namespace skyline::kernel { auto *core{&cores.at(thread->coreId)}; std::unique_lock coreLock(core->mutex); - auto currentIt{std::find(core->queue.begin(), core->queue.end(), thread)}; - if (currentIt == core->queue.end() || currentIt == core->queue.begin()) - // If the thread isn't in the queue then the new priority will be handled automatically on insertion - return; + auto currentIt{std::find(core->queue.begin(), core->queue.end(), thread)}, nextIt{std::next(currentIt)}; if (currentIt == core->queue.begin()) { - // Alternatively, if it's currently running then we'd just want to cause it to yield if its priority is lower than the the thread behind it - auto nextIt{std::next(currentIt)}; + // Alternatively, if it's currently running then we'd just want to yield if there's a higher priority thread to run instead if (nextIt != core->queue.end() && (*nextIt)->priority < thread->priority) { if (!thread->pendingYield) { thread->SendSignal(YieldSignal); thread->pendingYield = true; } } else if (!thread->isPreempted && thread->priority == core->preemptionPriority) { - // If the thread needs to be preempted due to the new priority then arm its preemption timer - struct itimerspec spec{.it_value = {.tv_nsec = std::chrono::duration_cast(PreemptiveTimeslice).count()}}; - timer_settime(thread->preemptionTimer, 0, &spec, nullptr); - thread->isPreempted = true; + // If the thread needs to be preempted due to its new priority then arm its preemption timer + thread->ArmPreemptionTimer(PreemptiveTimeslice); + } else if (thread->isPreempted && thread->priority != core->preemptionPriority) { + // If the thread no longer needs to be preempted due to its new priority then disarm its preemption timer + thread->DisarmPreemptionTimer(); } - return; - } + } else if (currentIt != core->queue.end() && (thread->priority < (*std::prev(currentIt))->priority || (nextIt != core->queue.end() && thread->priority > (*nextIt)->priority))) { + // If the thread is in the queue and it's position is affected by the priority change then need to remove and re-insert the thread + core->queue.erase(currentIt); - auto targetIt{std::upper_bound(core->queue.begin(), core->queue.end(), thread->priority.load(), type::KThread::IsHigherPriority)}; - if (currentIt == targetIt) - // If this thread's position isn't affected by the priority change then we have nothing to do - return; - - core->queue.erase(currentIt); - - if (thread->isPreempted && thread->priority != core->preemptionPriority) { - struct itimerspec spec{}; - timer_settime(thread->preemptionTimer, 0, &spec, nullptr); - thread->isPreempted = false; - } - - targetIt = std::upper_bound(core->queue.begin(), core->queue.end(), thread->priority.load(), type::KThread::IsHigherPriority); // Iterator invalidation - if (targetIt == core->queue.begin() && targetIt != core->queue.end()) { - core->queue.insert(std::next(core->queue.begin()), thread); - auto front{core->queue.front()}; - if (!front->pendingYield) { - front->SendSignal(YieldSignal); - front->pendingYield = true; + auto targetIt{std::upper_bound(core->queue.begin(), core->queue.end(), thread->priority.load(), type::KThread::IsHigherPriority)}; + if (targetIt == core->queue.begin() && targetIt != core->queue.end()) { + core->queue.insert(std::next(core->queue.begin()), thread); + auto front{core->queue.front()}; + if (!front->pendingYield) { + front->SendSignal(YieldSignal); + front->pendingYield = true; + } + } else { + core->queue.insert(targetIt, thread); } - } else { - core->queue.insert(targetIt, thread); } } diff --git a/app/src/main/cpp/skyline/kernel/scheduler.h b/app/src/main/cpp/skyline/kernel/scheduler.h index eeca3d53..54b88f60 100644 --- a/app/src/main/cpp/skyline/kernel/scheduler.h +++ b/app/src/main/cpp/skyline/kernel/scheduler.h @@ -66,8 +66,9 @@ namespace skyline { 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 - inline static thread_local bool YieldPending{}; //!< A flag denoting if a yield is pending on this thread, it's checked at SVC exit + inline static int YieldSignal{SIGRTMIN}; //!< The signal used to cause a non-cooperative yield in running threads + inline static int PreemptionSignal{SIGRTMIN + 1}; //!< The signal used to cause a preemptive yield in running threads + inline static thread_local bool YieldPending{}; //!< A flag denoting if a yield is pending on this thread, it's checked prior to entering guest code as signals cannot interrupt host code Scheduler(const DeviceState &state); @@ -115,7 +116,7 @@ namespace skyline { void RemoveThread(); /** - * @brief Updates the placement of the supplied thread in its resident core's queue according to its new priority + * @brief Updates the placement of the supplied thread in its resident core's queue according to its current priority */ void UpdatePriority(const std::shared_ptr &thread); diff --git a/app/src/main/cpp/skyline/kernel/svc.cpp b/app/src/main/cpp/skyline/kernel/svc.cpp index dbd56f7c..2bd70100 100644 --- a/app/src/main/cpp/skyline/kernel/svc.cpp +++ b/app/src/main/cpp/skyline/kernel/svc.cpp @@ -358,6 +358,7 @@ namespace skyline::kernel::svc { newPriority = std::min(newPriority, priority); } while (newPriority != priority && thread->priority.compare_exchange_strong(newPriority, priority)); state.scheduler->UpdatePriority(thread); + thread->UpdatePriorityInheritance(); } state.ctx->gpr.w0 = Result{}; } catch (const std::out_of_range &) { @@ -423,6 +424,7 @@ namespace skyline::kernel::svc { if (thread == state.thread) { state.scheduler->RemoveThread(); + thread->coreId = idealCore; state.scheduler->InsertThread(state.thread); state.scheduler->WaitSchedule(); } else if (!thread->running) { @@ -440,6 +442,7 @@ namespace skyline::kernel::svc { } void GetCurrentProcessorNumber(const DeviceState &state) { + std::lock_guard guard(state.thread->coreMigrationMutex); auto coreId{state.thread->coreId}; state.logger->Debug("svcGetCurrentProcessorNumber: C{}", coreId); state.ctx->gpr.w0 = coreId; @@ -812,10 +815,9 @@ namespace skyline::kernel::svc { void Break(const DeviceState &state) { auto reason{state.ctx->gpr.x0}; if (reason & (1ULL << 31)) { - state.logger->Error("svcBreak: Debugger is being engaged ({})", reason); - __builtin_trap(); + state.logger->Debug("svcBreak: Debugger is being engaged ({})", reason); } else { - state.logger->Error("svcBreak: Stack Trace ({}){}", reason, state.loader->GetStackTrace()); + state.logger->Error("svcBreak: Exit Stack Trace ({}){}", reason, state.loader->GetStackTrace()); if (state.thread->id) state.process->Kill(false); std::longjmp(state.thread->originalCtx, true); diff --git a/app/src/main/cpp/skyline/kernel/types/KProcess.cpp b/app/src/main/cpp/skyline/kernel/types/KProcess.cpp index 219c4257..8f97d927 100644 --- a/app/src/main/cpp/skyline/kernel/types/KProcess.cpp +++ b/app/src/main/cpp/skyline/kernel/types/KProcess.cpp @@ -131,38 +131,9 @@ namespace skyline::kernel::type { state.thread->waitTag = tag; } - if (isHighestPriority) { + if (isHighestPriority) // If we were the highest priority thread then we need to inherit priorities for all threads we're waiting on recursively - do { - u8 priority, ownerPriority; - do { - // Try to CAS the priority of the owner with the current thread - // If the new priority is equivalent to the current priority then we don't need to CAS - ownerPriority = owner->priority.load(); - priority = std::min(ownerPriority, state.thread->priority.load()); - } while (ownerPriority != priority && owner->priority.compare_exchange_strong(ownerPriority, priority)); - - if (ownerPriority != priority) { - std::shared_ptr waitThread; - { - std::lock_guard lock(waitThread->waiterMutex); - waitThread = owner->waitThread; - - // We need to update the location of the owner thread in the waiter queue of the thread it's waiting on - auto &waiters{waitThread->waiters}; - waiters.erase(std::find(waiters.begin(), waiters.end(), waitThread)); - waiters.insert(std::upper_bound(waiters.begin(), waiters.end(), state.thread->priority.load(), KThread::IsHigherPriority), owner); - break; - } - - state.scheduler->UpdatePriority(owner); - - owner = waitThread; - } else { - break; - } - } while (owner); - } + state.thread->UpdatePriorityInheritance(); state.scheduler->WaitSchedule(false); diff --git a/app/src/main/cpp/skyline/kernel/types/KThread.cpp b/app/src/main/cpp/skyline/kernel/types/KThread.cpp index 84b5e75e..23390c24 100644 --- a/app/src/main/cpp/skyline/kernel/types/KThread.cpp +++ b/app/src/main/cpp/skyline/kernel/types/KThread.cpp @@ -56,7 +56,7 @@ namespace skyline::kernel::type { } struct sigevent event{ - .sigev_signo = Scheduler::YieldSignal, + .sigev_signo = Scheduler::PreemptionSignal, .sigev_notify = SIGEV_THREAD_ID, .sigev_notify_thread_id = gettid(), }; @@ -64,7 +64,7 @@ namespace skyline::kernel::type { throw exception("timer_create has failed with '{}'", strerror(errno)); signal::SetSignalHandler({SIGINT, SIGILL, SIGTRAP, SIGBUS, SIGFPE, SIGSEGV}, nce::NCE::SignalHandler); - signal::SetSignalHandler({Scheduler::YieldSignal}, Scheduler::SignalHandler, false); // We want futexes to fail and their predicates rechecked + signal::SetSignalHandler({Scheduler::YieldSignal, Scheduler::PreemptionSignal}, Scheduler::SignalHandler, false); // We want futexes to fail and their predicates rechecked { std::lock_guard lock(statusMutex); @@ -226,4 +226,53 @@ namespace skyline::kernel::type { if (!killed && running) pthread_kill(pthread, signal); } + + void KThread::ArmPreemptionTimer(std::chrono::nanoseconds timeToFire) { + struct itimerspec spec{.it_value = { + .tv_nsec = std::min(timeToFire.count(), static_cast(constant::NsInSecond)), + .tv_sec = std::max(std::chrono::duration_cast(timeToFire).count() - 1, 0LL), + }}; + timer_settime(preemptionTimer, 0, &spec, nullptr); + isPreempted = true; + } + + void KThread::DisarmPreemptionTimer() { + if (isPreempted) { + struct itimerspec spec{}; + timer_settime(preemptionTimer, 0, &spec, nullptr); + isPreempted = false; + } + } + + void KThread::UpdatePriorityInheritance() { + auto waitingOn{waitThread}; + u8 currentPriority{priority.load()}; + while (waitingOn) { + u8 ownerPriority; + do { + // Try to CAS the priority of the owner with the current thread + // If the new priority is equivalent to the current priority then we don't need to CAS + ownerPriority = waitingOn->priority.load(); + if (ownerPriority <= currentPriority) + return; + } while (waitingOn->priority.compare_exchange_strong(ownerPriority, currentPriority)); + + if (ownerPriority != currentPriority) { + std::lock_guard waiterLock(waitingOn->waiterMutex); + auto nextThread{waitingOn->waitThread}; + if (nextThread){ + // We need to update the location of the owner thread in the waiter queue of the thread it's waiting on + std::lock_guard nextWaiterLock(nextThread->waiterMutex); + auto &piWaiters{nextThread->waiters}; + piWaiters.erase(std::find(piWaiters.begin(), piWaiters.end(), waitingOn)); + piWaiters.insert(std::upper_bound(piWaiters.begin(), piWaiters.end(), currentPriority, KThread::IsHigherPriority), waitingOn); + break; + } + state.scheduler->UpdatePriority(waitingOn); + waitingOn = nextThread; + } else { + break; + } + } + } } diff --git a/app/src/main/cpp/skyline/kernel/types/KThread.h b/app/src/main/cpp/skyline/kernel/types/KThread.h index 2f8c2542..a2b640df 100644 --- a/app/src/main/cpp/skyline/kernel/types/KThread.h +++ b/app/src/main/cpp/skyline/kernel/types/KThread.h @@ -21,6 +21,7 @@ namespace skyline { KProcess *parent; std::thread thread; //!< If this KThread is backed by a host thread then this'll hold it pthread_t pthread{}; //!< The pthread_t for the host thread running this guest thread + timer_t preemptionTimer{}; //!< A kernel timer used for preemption interrupts /** * @brief Entry function any guest threads, sets up necessary context and jumps into guest code from the calling thread @@ -56,7 +57,6 @@ namespace skyline { u64 timesliceStart{}; //!< A timestamp in host CNTVCT ticks of when the thread's current timeslice started u64 averageTimeslice{}; //!< A weighted average of the timeslice duration for this thread - timer_t preemptionTimer{}; //!< A kernel timer used for preemption interrupts bool isPreempted{}; //!< If the preemption timer has been armed and will fire bool pendingYield{}; //!< If the thread has been yielded and hasn't been acted upon it yet @@ -93,6 +93,22 @@ namespace skyline { */ void SendSignal(int signal); + /** + * @brief Arms the preemption kernel timer to fire in the specified amount of time + */ + void ArmPreemptionTimer(std::chrono::nanoseconds timeToFire); + + /** + * @brief Disarms the preemption kernel timer, any scheduled firings will be cancelled + */ + void DisarmPreemptionTimer(); + + /** + * @brief Recursively updates the priority for any threads this thread might be waiting on + * @note PI is performed by temporarily upgrading a thread's priority if a thread waiting on it has a higher priority to prevent priority inversion + */ + void UpdatePriorityInheritance(); + /** * @return If the supplied priority value is higher than the supplied thread's priority value */ diff --git a/app/src/main/cpp/skyline/loader/loader.cpp b/app/src/main/cpp/skyline/loader/loader.cpp index e020fca3..e94e0919 100644 --- a/app/src/main/cpp/skyline/loader/loader.cpp +++ b/app/src/main/cpp/skyline/loader/loader.cpp @@ -84,17 +84,17 @@ namespace skyline::loader { size_t length{}; std::unique_ptr demangled{abi::__cxa_demangle(symbol.name, nullptr, &length, &status), std::free}; - return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast(pointer), (status == 0) ? std::string(demangled.get()) : symbol.name, symbol.executableName); + return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast(pointer), (status == 0) ? std::string(demangled.get()) : symbol.name, symbol.executableName); } else if (!symbol.executableName.empty()) { - return fmt::format("\n* 0x{:X} (from {})", reinterpret_cast(pointer), symbol.executableName); + return fmt::format("\n* 0x{:X} (from {})", reinterpret_cast(pointer), symbol.executableName); } else if (dladdr(pointer, &info)) { int status{}; size_t length{}; std::unique_ptr demangled{abi::__cxa_demangle(info.dli_sname, nullptr, &length, &status), std::free}; - return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast(pointer), (status == 0) ? std::string(demangled.get()) : info.dli_sname, info.dli_fname); + return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast(pointer), (status == 0) ? std::string(demangled.get()) : info.dli_sname, info.dli_fname); } else { - return fmt::format("\n* 0x{:X}", reinterpret_cast(pointer)); + return fmt::format("\n* 0x{:X}", reinterpret_cast(pointer)); } }