search

Meziu / rust-horizon

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
3 stars 1 forks source link

Implement std threads support #10

Closed AzureMarker closed 2 years ago

AzureMarker commented 2 years ago

Requires:

Meziu commented 2 years ago

Also, while the function docs are fine, add an explanation of why we need those implemented.

AzureMarker commented 2 years ago

Also, while the function docs are fine, add an explanation of why we need those implemented.

I added a little bit, but can you give an example of what you're thinking?

AzureMarker commented 2 years ago

@ian-h-chamberlain based on your comment here I did some poking around and it may be possible to use libctru APIs to set the priority and affinity after-the-fact: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/std.3A.3Athread.20support.20for.20armv6k-nintendo-3ds

I'll look into supporting that in std, but we still may want the current API + the ability to set it after thread spawn.

AzureMarker commented 2 years ago

These are some interesting wiki pages: https://www.3dbrew.org/wiki/Multi-threading https://www.3dbrew.org/wiki/KThread

From this, we should change how we refer to the core ID we pass during thread creation. "Affinity" usually refers to a CPU affinity bit mask that tells the kernel which cores a thread could run on (or a list of masks, for multiple CPUs). The 3DS wiki refers to the core ID passed during thread creation as the "ideal processor" ID. The 3DS kernel stores both pieces of information (see offsets 0x7E and 0x90 on the KThread page).

The affinity mask concept is pretty common, so there are some existing pthread APIs we could reuse. Similar for thread priority by the way. But the ideal processor idea is not so common, so I think we will still need our own nonstandard pthread function for that. Based on the naming scheme used below, I nominate pthread_attr_setidealprocessor_np and pthread_setidealprocessor_np (plus get variants).

Here is a listing of the known (and slightly less known - nonstandard) pthread functions we could reuse: Function Usage Link
pthread_attr_setschedparam Set the priority attribute value https://man7.org/linux/man-pages/man3/pthread_attr_getschedparam.3p.html
pthread_attr_getschedparam Get the priority attribute value https://man7.org/linux/man-pages/man3/pthread_attr_getschedparam.3p.html
pthread_setschedparam Set the thread's priority value https://man7.org/linux/man-pages/man3/pthread_setschedparam.3.html
pthread_getschedparam Get the thread's priority value https://man7.org/linux/man-pages/man3/pthread_setschedparam.3.html
pthread_attr_setaffinity_np Set the affinity mask attribute value https://man7.org/linux/man-pages/man3/pthread_attr_setaffinity_np.3.html
pthread_attr_getaffinity_np Get the affinity mask attribute value https://man7.org/linux/man-pages/man3/pthread_attr_setaffinity_np.3.html
pthread_setaffinity_np Set the thread's affinity mask value https://man7.org/linux/man-pages/man3/pthread_setaffinity_np.3.html
pthread_getaffinity_np Get the thread's affinity mask value https://man7.org/linux/man-pages/man3/pthread_setaffinity_np.3.html

Note that the pthread_attr_setaffinity_np function, if we used it, would only apply after creating the thread since we don't have an API to set it during creation. See also https://stackoverflow.com/a/35278467.

I think we should keep the thread builder extension trait added in this PR, but we could also consider adding APIs for modifying the priority, ideal processor, and affinity mask of existing threads (might want to keep these in std::os::horizon::thread).

Meziu commented 2 years ago

Great. This PR will be merged after the std PR, so you can make all the changes needed. Just rename them in libc and we are good to go.

AzureMarker commented 2 years ago

We could also merge this PR when it's ready to get it out of the way, and make a separate branch for the std PR. We may need to clean up the commit history or split it into two PRs, so making a separate branch might be necessary either way.

Meziu commented 2 years ago

Yeah we should squash some stuff and clean up the repo. Let’s make a branch.

AzureMarker commented 2 years ago

By the way, this thread is interesting (even though it never got properly answered): https://gbatemp.net/threads/can-horizon-indepently-relocate-threads-to-cpus-in-their-affinity-masks.556096/

I don't have a New 3DS/2DS to test with (to see if it balances across multiple app cores), but it does seem a little odd that there's the whole affinity mask feature if it's not really used by the kernel after initial scheduling.

AzureMarker commented 2 years ago

I did a quick test to try migrating a thread from sys core to app core (and vice versa) via svcSetThreadIdealProcessor. It fails with an error (result), level = RL_FATAL, summary = RS_NOTSUPPORTED, description = RD_NOT_IMPLEMENTED. This happens even if the thread's affinity mask allows it to exist on either app or sys core (mask = 0x3).

Due to this, I don't think it makes sense to add this feature (setting the ideal processor after spawning) to std. I'm not sure if it's worthwhile to add the get version either, since that could return stuff like -2 or -1 which don't tell you what processor it is actually on. The only API I know of to do that is svcGetProcessorID, and that only gets the current thread's value.

So basically, (I think) we should just have functions to get the current thread's priority and processor ID in std, but not the ideal thread or affinity mask.

ian-h-chamberlain commented 2 years ago

I don't have a New 3DS/2DS to test with (to see if it balances across multiple app cores), but it does seem a little odd that there's the whole affinity mask feature if it's not really used by the kernel after initial scheduling.

I have a N3DS if you have an example you'd like me to test – if those functions are implemented on the new processors perhaps it would be worth keeping the implementation?

AzureMarker commented 2 years ago

@ian-h-chamberlain nice, I was testing with a variant of this example: https://github.com/Meziu/ctru-rs/pull/46/commits/353b6c1c77489a0c56e83418a67d572964b30bc4

I think you have to enable the new cores through some mechanism. This is what the docs say:

Processor #2 is New3DS exclusive. Normal applications can create threads on this core if the exheader kernel flags bitmask has 0x2000 set.

Then just change the ideal processor processor ID setting of the thread to -1 and try to move the thread to another core with something like this:

println!("Moving to app core 2");
let result =
    unsafe { ctru_sys::svcSetThreadIdealProcessor(ctru_sys::CUR_THREAD_HANDLE, 2) };
if ctru_sys::R_FAILED(result) {
    println!("result level = {}", ctru_sys::R_LEVEL(result));
    println!("result summary = {}", ctru_sys::R_SUMMARY(result));
    println!("result description = {}", ctru_sys::R_DESCRIPTION(result));
    return;
}

println!("On app core 2");
print_affinity_mask("app 2 thread");
print_thread_id("app 2 thread");
print_thread_list();
Meziu commented 2 years ago

Even if it was technically possible to move threads around, it looks like the 3DS is the worst system to do so. There are 2 VERY different cores on a normal 3DS, and 4 (yet one is totally unusable) on a New3DS. I have never even thought of trying: why would you try to move a thread that’s supposed to be preemptive to a core that isn’t? Or try to move a thread to a core some systems don’t even have? It usually ends up being: common threads on core#0, and something important on core#1. I don’t think it matters.

ian-h-chamberlain commented 2 years ago

Even if it was technically possible to move threads around, it looks like the 3DS is the worst system to do so. There are 2 VERY different cores on a normal 3DS, and 4 (yet one is totally unusable) on a New3DS. I have never even thought of trying: why would you try to move a thread that’s supposed to be preemptive to a core that isn’t? Or try to move a thread to a core some systems don’t even have? It usually ends up being: common threads on core#0, and something important on core#1. I don’t think it matters.

Ok, I got a chance to test this on my device and it seems that it's not supported anyway. I got an error code with summary code "Not supported", description "Not implemented", so I think we can safely ignore this functionality for the reasons you describe and the fact that it doesn't work anyway.

AzureMarker commented 2 years ago

I think the PR is ready to merge, whenever we want to do that. For the std PRs we can cherry-pick changes to separate branches, so I don't think that would block merging this PR.

AzureMarker commented 2 years ago

@Meziu Are you waiting to merge this? What's the plan?

Meziu commented 2 years ago

@Meziu Are you waiting to merge this? What's the plan?

Aren’t there some memory issues related with #15 and https://github.com/Meziu/ctru-rs/issues/48 ? I thought the code wasn’t final yet.

AzureMarker commented 2 years ago

@Meziu Are you waiting to merge this? What's the plan?

Aren’t there some memory issues related with #15 and Meziu/ctru-rs#48 ? I thought the code wasn’t final yet.

Oh yeah, not sure how I forgot that blocker. I guess I thought of this code as "done", and any future changes would need another PR. With that in mind though, I do have one small change planned for this PR before it merges (along with the review comments).

AzureMarker commented 2 years ago

Once https://github.com/Meziu/pthread-3ds/pull/14 merges, I think this could merge too. The small change I wanted to make was https://github.com/Meziu/rust-horizon/pull/10/commits/3b5e287a9ec06158191cea81645870ef8a1e8677.

AzureMarker commented 2 years ago

Thanks @Meziu and @ian-h-chamberlain for reviewing and keeping up with this PR!