Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core timing 2.0 #4913

Merged
merged 9 commits into from Feb 21, 2020
Merged

Core timing 2.0 #4913

merged 9 commits into from Feb 21, 2020

Conversation

B3n30
Copy link
Contributor

@B3n30 B3n30 commented Sep 1, 2019

This is a rewrite of Core::CoreTiming. The goal was to be able to emulate multiple cores on a single host thread

What Changed?

  • There is a timer for each core.
  • All timers are handled by a TimingManager (called Core::Timing)
  • Cores are handled consecutively by running a fixed amount of ticks or until the next event on that core
  • If an event is scheduled in the same core, it could execute less ticks
  • Events scheduled on another core will be scheduled as exact as possible. This means for cores that weren't already executed in the current cycle we can schedule them without a hassle. But for events scheduled on a core that already executed the current cycle and thus is ahead of time we will just schedule it from the time on that that core is already (it's a small delay).
  • All this event scheduling could lead to some cores being ahead of others. Thus in the next cycle of core execution, we try to bring all cores back to the one that is the most advanced

What is not in this PR?

  • All cores currently using the same type of scheduling threads. At a later stage we could think about having different scheduling types for cores (e.g. syscore is believed to use preemtive scheduling)
  • Layer2 cache
  • Adjusting CPU Clock Frequency
  • ApplicationCpuTimeLimit for syscore
  • The Register Debugger Widget will only show registers of core 0 for now. This needs a rework but can be done in a sperate PR
  • Unittests only test the case for one timer in TimingManager. So test for multiple timers would be nice

What I'm not sure of

  • Changes in gdbstub
  • Changes in cheats
  • Changes in RPC_Server

What does it fix? Why do we need this?

  • Games that broke due to removing the priority boost hack
  • Without that change a accurate new 3ds emulation is not possible

Sorry that it became such a big PR...


This change is Reviewable

@B3n30
Copy link
Contributor Author

B3n30 commented Sep 1, 2019

ci failure is unrelated

@@ -115,7 +115,7 @@ void RO::Initialize(Kernel::HLERequestContext& ctx) {
return;
}

CROHelper crs(crs_address, *process, system.Memory(), system.CPU());
CROHelper crs(crs_address, *process, system.Memory(), system.GetRunningCore());
Copy link
Member

@wwylele wwylele Sep 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should invalidate the cache of all core instead of just the current one.

Core::CPU().InvalidateCacheRange(address, data_size);

// Is current core correct here?
Core::GetRunningCore().InvalidateCacheRange(address, data_size);
Copy link
Member

@wwylele wwylele Sep 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, this should invalidate cache of all cores

@@ -33,13 +33,19 @@ void Thread::Acquire(Thread* thread) {
ASSERT_MSG(!ShouldWait(thread), "object unavailable!");
}

u32 ThreadManager::next_thread_id = 1;
Copy link
Member

@wwylele wwylele Sep 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let this live in the kernel object instead of putting it as a global state.

};

using SharedTimer = std::shared_ptr<Timing::Timer>;
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I feel this typedef is pretty useless and actually add to the difficulty when reading and understanding the code.

void KernelSystem::SetCPUs(std::vector<std::shared_ptr<ARM_Interface>> cpus) {
ASSERT(cpus.size() == thread_managers.size());
u32 i = 0;
for (auto cpu : cpus) {
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto cpu : cpus) {
for (auto& cpu : cpus) {

current_process = process;
SetCurrentMemoryPageTable(&process->vm_manager.page_table);
} else {
stored_processes[core_id];
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh.. what is this? I feel like this isn't doing anything

@@ -177,31 +229,50 @@ void System::Reschedule() {
}

reschedule_pending = false;
kernel->GetThreadManager().Reschedule();
for (auto core : cpu_cores) {
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto core : cpu_cores) {
for (const auto& core : cpu_cores) {

// executing the first cycle of each slice to prepare the slice length and downcount for
// that slice.
bool is_global_timer_sane = true;
std::unordered_map<std::size_t, std::shared_ptr<Timer>> timers;
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this can just be a vector

u64 idled_cycles;
};

Timing(std::size_t num_cores);
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Timing(std::size_t num_cores);
explicit Timing(std::size_t num_cores);

@@ -125,6 +125,8 @@ inline u64 cyclesToMs(s64 cycles) {

namespace Core {

class System;
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used?

@@ -7,19 +7,27 @@
#include <tuple>
#include "common/assert.h"
#include "common/logging/log.h"
#include "core/core.h"
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessary

thread->status = ThreadStatus::Dormant;
thread->entry_point = entry_point;
thread->stack_top = stack_top;
thread->nominal_priority = thread->current_priority = priority;
thread->last_running_ticks = timing.GetTicks();
thread->last_running_ticks = timing.GetGlobalTicks();
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is global ticks used here? Should this be the ticks of the target processer, or the current core instead?

Copy link
Contributor Author

@B3n30 B3n30 Feb 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to go with GlobalTicks which is the ticks of the core thats advanced the most. The reason for this is because in any other case it would always create a thread in the past either for the current core or the target one. We could also take tha max of ticks from current core and target core but I think the overall max is fine too

@robzombie91
Copy link

robzombie91 commented Sep 3, 2019

Tested my first game with this "SMT Devil Survivor Overclocked" The game refused to process the fmv that comes after the intro logo's, thus you cant go any farther into the game.

Edit: Log Output

@FearlessTobi
Copy link
Contributor

FearlessTobi commented Sep 3, 2019

Fixes Donkey Kong Country Returns 3D and Sonic Boom: Fire & Ice.

src/core/core.h Outdated
@@ -248,6 +266,8 @@ class System {
return registered_swkbd;
}

bool initalized = false;
Copy link
Member

@lioncash lioncash Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be private

@B3n30
Copy link
Contributor Author

B3n30 commented Sep 3, 2019

After looking into the threads SMT Devil Survivor Overclocked creates, they are all created in core0, so multicore implementation can't fix it.
I wonder if there is still something wrong with our scheduler?

Edit: List of all threads created:

core/hle/kernel/svc.cpp:CreateThread:922: called entrypoint=0x002DA5D8 (thread-002DA5D8), arg=0x003EA880, stacktop=0x003EA880, threadpriority=0x00000018, processorid=0x00000000 : created handle=0x00020006
core/hle/kernel/svc.cpp:CreateThread:922: called entrypoint=0x002DA5D8 (thread-002DA5D8), arg=0x003DE890, stacktop=0x003DE890, threadpriority=0x0000002F, processorid=0x00000000 : created handle=0x0005000F
core/hle/kernel/svc.cpp:CreateThread:922: called entrypoint=0x002DA5D8 (thread-002DA5D8), arg=0x003E7650, stacktop=0x003E7650, threadpriority=0x0000001A, processorid=0x00000000 : created handle=0x000B0022
core/hle/kernel/svc.cpp:CreateThread:922: called entrypoint=0x002DA5D8 (thread-002DA5D8), arg=0x09000FC8, stacktop=0x09000FC8, threadpriority=0x00000022, processorid=0x00000000 : created handle=0x000D8028
core/hle/kernel/svc.cpp:CreateThread:922: called entrypoint=0x002DA5D8 (thread-002DA5D8), arg=0x003A9D38, stacktop=0x003A9D38, threadpriority=0x00000031, processorid=0x00000000 : created handle=0x000E0029
core/hle/kernel/svc.cpp:CreateThread:922: called entrypoint=0x002DA5D8 (thread-002DA5D8), arg=0x003ADE50, stacktop=0x003ADE50, threadpriority=0x00000032, processorid=0x00000000 : created handle=0x000E802A

@Timmy5942

This comment has been minimized.

@B3n30 B3n30 force-pushed the core-timing_2.0 branch 2 times, most recently from f33464e to 58b2abd Compare Feb 9, 2020
@B3n30
Copy link
Contributor Author

B3n30 commented Feb 9, 2020

Rebased and addressed comments. I hope I didn't miss anything

@purpasmart96
Copy link
Member

purpasmart96 commented Feb 19, 2020

As long as it doesn't break anything. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants