The-Homebrew-Cloud/skyline
2
0
Fork 0
mirror of https://github.com/skyline-emu/skyline.git synced 2024-06-15 05:58:47 +02:00
Code Issues Packages Projects Releases Wiki Activity

Address CR Comments 2 (#132) + Fix Several Scheduler Bugs

Browse Source 
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
This commit is contained in:
PixelyIon 2021-03-05 14:36:41 +05:30 committed by ◱ Mark
parent fe5061a8e0
commit c282276b74
7 changed files with 118 additions and 129 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

120
app/src/main/cpp/skyline/kernel/scheduler.cpp
View File

@ -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<nce::ThreadContext *>(*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<std::chrono::nanoseconds>(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<std::chrono::nanoseconds>(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<std::chrono::nanoseconds>(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);
}
}

7
app/src/main/cpp/skyline/kernel/scheduler.h
View File

@ -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<type::KThread> &thread);

8
app/src/main/cpp/skyline/kernel/svc.cpp
View File

@ -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);

33
app/src/main/cpp/skyline/kernel/types/KProcess.cpp
View File

@ -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<KThread> 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);

53
app/src/main/cpp/skyline/kernel/types/KThread.cpp
View File

@ -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<long long>(constant::NsInSecond)),
.tv_sec = std::max(std::chrono::duration_cast<std::chrono::seconds>(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;
}
}
}
}

18
app/src/main/cpp/skyline/kernel/types/KThread.h
View File

@ -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
*/

8
app/src/main/cpp/skyline/loader/loader.cpp
View File

@ -84,17 +84,17 @@ namespace skyline::loader {
size_t length{};
std::unique_ptr<char, decltype(&std::free)> demangled{abi::__cxa_demangle(symbol.name, nullptr, &length, &status), std::free};
return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast<u64>(pointer), (status == 0) ? std::string(demangled.get()) : symbol.name, symbol.executableName);
return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast<uintptr_t>(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<u64>(pointer), symbol.executableName);
return fmt::format("\n* 0x{:X} (from {})", reinterpret_cast<uintptr_t>(pointer), symbol.executableName);
} else if (dladdr(pointer, &info)) {
int status{};
size_t length{};
std::unique_ptr<char, decltype(&std::free)> demangled{abi::__cxa_demangle(info.dli_sname, nullptr, &length, &status), std::free};
return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast<u64>(pointer), (status == 0) ? std::string(demangled.get()) : info.dli_sname, info.dli_fname);
return fmt::format("\n* 0x{:X} ({} from {})", reinterpret_cast<uintptr_t>(pointer), (status == 0) ? std::string(demangled.get()) : info.dli_sname, info.dli_fname);
} else {
return fmt::format("\n* 0x{:X}", reinterpret_cast<u64>(pointer));
return fmt::format("\n* 0x{:X}", reinterpret_cast<uintptr_t>(pointer));
}
}