[PATCH 0/5] MR7317: Server-side thread priorities implementation (Part 2)
This adds Mach thread priority support (both in the application and realtime band) and recalculates thread priorities when the process priority changes. Part 3, which is still a bit WIP deals with implementing priority boosts (for main threads and threads which are processing window messages), effectively fully replacing https://gitlab.winehq.org/wine/wine/-/merge_requests/1232. Currently the implementation in this MR already technically overrides what https://gitlab.winehq.org/wine/wine/-/merge_requests/1232 does, if it makes sense I can also revert it here. I added a few comments regarding the Mach thread priority API usage, as there is limited documentation available, and much was inferred from the source or by testing. If this is too verbose I can also remove that... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7317
From: Marc-Aurel Zent <marc_aurel(a)me.com> --- server/process.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/server/process.c b/server/process.c index e06350f7311..4993b8682b0 100644 --- a/server/process.c +++ b/server/process.c @@ -1599,6 +1599,17 @@ DECL_HANDLER(get_process_vm_counters) release_object( process ); } +static void set_process_priority( struct process *process, int priority ) +{ + struct thread *thread; + process->priority = priority; + + LIST_FOR_EACH_ENTRY( thread, &process->thread_list, struct thread, proc_entry ) + { + set_thread_priority( thread, priority, thread->priority ); + } +} + static void set_process_affinity( struct process *process, affinity_t affinity ) { struct thread *thread; @@ -1624,7 +1635,7 @@ DECL_HANDLER(set_process_info) if ((process = get_process_from_handle( req->handle, PROCESS_SET_INFORMATION ))) { - if (req->mask & SET_PROCESS_INFO_PRIORITY) process->priority = req->priority; + if (req->mask & SET_PROCESS_INFO_PRIORITY) set_process_priority( process, req->priority ); if (req->mask & SET_PROCESS_INFO_AFFINITY) set_process_affinity( process, req->affinity ); if (req->mask & SET_PROCESS_INFO_TOKEN) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7317
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernel32/tests/loader.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 2c7cc784be4..6cf6971ba04 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -3672,6 +3672,7 @@ static void test_ExitProcess(void) struct PROCESS_BASIC_INFORMATION_PRIVATE pbi; MEMORY_BASIC_INFORMATION mbi; DWORD_PTR affinity; + PROCESS_PRIORITY_CLASS ppc; void *addr; LARGE_INTEGER offset; SIZE_T size; @@ -4011,6 +4012,10 @@ static void test_ExitProcess(void) affinity = 1; ret = pNtSetInformationProcess(pi.hProcess, ProcessAffinityMask, &affinity, sizeof(affinity)); ok(ret == STATUS_PROCESS_IS_TERMINATING, "expected STATUS_PROCESS_IS_TERMINATING, got %#lx\n", ret); + ppc.Foreground = FALSE; + ppc.PriorityClass = PROCESS_PRIOCLASS_BELOW_NORMAL; + ret = pNtSetInformationProcess(pi.hProcess, ProcessPriorityClass, &ppc, sizeof(ppc)); + ok(ret == STATUS_SUCCESS, "expected STATUS_SUCCESS, got status %#lx\n", ret); SetLastError(0xdeadbeef); ctx.ContextFlags = CONTEXT_INTEGER; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7317
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/thread.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/server/thread.c b/server/thread.c index 3c7e4541a09..2e7e79f66b3 100644 --- a/server/thread.c +++ b/server/thread.c @@ -40,6 +40,11 @@ #ifdef HAVE_SYS_RESOURCE_H #include <sys/resource.h> #endif +#ifdef __APPLE__ +#include <mach/mach_init.h> +#include <mach/mach_port.h> +#include <mach/thread_act.h> +#endif #include "ntstatus.h" #define WIN32_NO_STATUS @@ -252,6 +257,79 @@ static void apply_thread_priority( struct thread *thread, int base_priority ) setpriority( PRIO_PROCESS, thread->unix_tid, niceness ); } +#elif defined(__APPLE__) + +void init_threading(void) +{ +} + +static int get_mach_importance( int base_priority ) +{ + int min = -31, max = 32, range = max - min; + return min + (base_priority - 1) * range / 14; +} + +static void apply_thread_priority( struct thread *thread, int base_priority ) +{ + kern_return_t kr; + mach_msg_type_name_t type; + int throughput_qos, latency_qos; + struct thread_extended_policy thread_extended_policy; + struct thread_precedence_policy thread_precedence_policy; + mach_port_t thread_port, process_port = thread->process->trace_data; + + if (!process_port) return; + kr = mach_port_extract_right( process_port, thread->unix_tid, + MACH_MSG_TYPE_COPY_SEND, &thread_port, &type ); + if (kr != KERN_SUCCESS) return; + /* base priority 15 is for time-critical threads, so not compute-bound */ + thread_extended_policy.timeshare = base_priority > 14 ? 0 : 1; + thread_precedence_policy.importance = get_mach_importance( base_priority ); + /* adapted from the QoS table at xnu/osfmk/kern/thread_policy.c */ + switch (thread->priority) + { + case THREAD_PRIORITY_IDLE: /* THREAD_QOS_MAINTENANCE */ + case THREAD_PRIORITY_LOWEST: /* THREAD_QOS_BACKGROUND */ + throughput_qos = THROUGHPUT_QOS_TIER_5; + latency_qos = LATENCY_QOS_TIER_3; + break; + case THREAD_PRIORITY_BELOW_NORMAL: /* THREAD_QOS_UTILITY */ + throughput_qos = THROUGHPUT_QOS_TIER_2; + latency_qos = LATENCY_QOS_TIER_3; + break; + case THREAD_PRIORITY_NORMAL: /* THREAD_QOS_LEGACY */ + case THREAD_PRIORITY_ABOVE_NORMAL: /* THREAD_QOS_USER_INITIATED */ + throughput_qos = THROUGHPUT_QOS_TIER_1; + latency_qos = LATENCY_QOS_TIER_1; + break; + case THREAD_PRIORITY_HIGHEST: /* THREAD_QOS_USER_INTERACTIVE */ + throughput_qos = THROUGHPUT_QOS_TIER_0; + latency_qos = LATENCY_QOS_TIER_0; + break; + case THREAD_PRIORITY_TIME_CRITICAL: + default: /* THREAD_QOS_UNSPECIFIED */ + throughput_qos = THROUGHPUT_QOS_TIER_UNSPECIFIED; + latency_qos = LATENCY_QOS_TIER_UNSPECIFIED; + break; + } + /* QOS_UNSPECIFIED is assigned the highest tier available, so it does not provide a limit */ + if (base_priority > THREAD_BASE_PRIORITY_LOWRT) + { + throughput_qos = THROUGHPUT_QOS_TIER_UNSPECIFIED; + latency_qos = LATENCY_QOS_TIER_UNSPECIFIED; + } + + thread_policy_set( thread_port, THREAD_LATENCY_QOS_POLICY, (thread_policy_t)&latency_qos, + THREAD_LATENCY_QOS_POLICY_COUNT ); + thread_policy_set( thread_port, THREAD_THROUGHPUT_QOS_POLICY, (thread_policy_t)&throughput_qos, + THREAD_THROUGHPUT_QOS_POLICY_COUNT ); + thread_policy_set( thread_port, THREAD_EXTENDED_POLICY, (thread_policy_t)&thread_extended_policy, + THREAD_EXTENDED_POLICY_COUNT ); + thread_policy_set( thread_port, THREAD_PRECEDENCE_POLICY, (thread_policy_t)&thread_precedence_policy, + THREAD_PRECEDENCE_POLICY_COUNT ); + mach_port_deallocate( mach_task_self(), thread_port ); +} + #else void init_threading(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7317
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/thread.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/server/thread.c b/server/thread.c index 2e7e79f66b3..8665c250c5a 100644 --- a/server/thread.c +++ b/server/thread.c @@ -42,6 +42,7 @@ #endif #ifdef __APPLE__ #include <mach/mach_init.h> +#include <mach/mach_time.h> #include <mach/mach_port.h> #include <mach/thread_act.h> #endif @@ -258,9 +259,21 @@ static void apply_thread_priority( struct thread *thread, int base_priority ) } #elif defined(__APPLE__) +static unsigned int mach_ticks_per_second; void init_threading(void) { + struct mach_timebase_info tb_info; + if (mach_timebase_info( &tb_info ) == KERN_SUCCESS) + { + mach_ticks_per_second = (tb_info.denom * 1000000000U) / tb_info.numer; + } + else + { + const unsigned int best_guess = 24000000U; + fprintf(stderr, "wine: mach_timebase_info failed, guessing %u mach ticks per second\n", best_guess); + mach_ticks_per_second = best_guess; + } } static int get_mach_importance( int base_priority ) @@ -327,6 +340,35 @@ static void apply_thread_priority( struct thread *thread, int base_priority ) THREAD_EXTENDED_POLICY_COUNT ); thread_policy_set( thread_port, THREAD_PRECEDENCE_POLICY, (thread_policy_t)&thread_precedence_policy, THREAD_PRECEDENCE_POLICY_COUNT ); + if (base_priority > THREAD_BASE_PRIORITY_LOWRT) + { + /* For realtime threads we are requesting from the scheduler to be moved + * into the Mach realtime band (96-127) above the kernel. + * The scheduler will bump us back into the application band though if we + * lie too much about our computation constraints... + * The maximum available amount of resources granted in that band is using + * half of the available bus cycles, and computation (nominally 1/10 of + * the time constraint) is a hint to the scheduler where to place our + * realtime threads relative to each other. + * If someone is violating the time contraint policy, they will be moved + * back where they were (non-timeshare application band with very high + * importance), which is on XNU equivalent to setting SCHED_RR with the + * pthread API. */ + struct thread_time_constraint_policy thread_time_constraint_policy; + int realtime_priority = base_priority - THREAD_BASE_PRIORITY_LOWRT; + unsigned int max_constraint = mach_ticks_per_second / 2; + unsigned int max_computation = max_constraint / 10; + /* unfortunately we can't give a hint for the periodicity of calculations */ + thread_time_constraint_policy.period = 0; + thread_time_constraint_policy.constraint = max_constraint; + thread_time_constraint_policy.computation = realtime_priority * max_computation / 16; + /* although it is not possible to disable preemption in NT, this is probably + * nice to have for the maximum possible effective thread priority */ + thread_time_constraint_policy.preemptible = thread->priority == THREAD_PRIORITY_TIME_CRITICAL ? 0 : 1; + thread_policy_set( thread_port, THREAD_TIME_CONSTRAINT_POLICY, + (thread_policy_t)&thread_time_constraint_policy, + THREAD_TIME_CONSTRAINT_POLICY_COUNT ); + } mach_port_deallocate( mach_task_self(), thread_port ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7317
From: Marc-Aurel Zent <mzent(a)codeweavers.com> This is needed for Mach based thread priorities to take effect, since before that the process port was not known. --- server/process.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/process.c b/server/process.c index 4993b8682b0..c7361c91467 100644 --- a/server/process.c +++ b/server/process.c @@ -1411,6 +1411,8 @@ DECL_HANDLER(get_startup_info) info->data_size = 0; } +static void set_process_priority( struct process *process, int priority ); + /* signal the end of the process initialization */ DECL_HANDLER(init_process_done) { @@ -1429,6 +1431,8 @@ DECL_HANDLER(init_process_done) process->start_time = current_time; init_process_tracing( process ); + /* Re-apply all thread priorities here, after process tracing is initialized */ + set_process_priority( process, process->priority ); generate_startup_debug_events( process ); set_process_startup_state( process, STARTUP_DONE ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7317
Rémi Bernon (@rbernon) commented about server/process.c:
info->data_size = 0; }
+static void set_process_priority( struct process *process, int priority );
You should declare the function here or above the requests, I believe we avoid using forward declarations if possible. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7317#note_94875
Rémi Bernon (@rbernon) commented about server/thread.c:
+ int throughput_qos, latency_qos; + struct thread_extended_policy thread_extended_policy; + struct thread_precedence_policy thread_precedence_policy; + mach_port_t thread_port, process_port = thread->process->trace_data; + + if (!process_port) return; + kr = mach_port_extract_right( process_port, thread->unix_tid, + MACH_MSG_TYPE_COPY_SEND, &thread_port, &type ); + if (kr != KERN_SUCCESS) return; + /* base priority 15 is for time-critical threads, so not compute-bound */ + thread_extended_policy.timeshare = base_priority > 14 ? 0 : 1; + thread_precedence_policy.importance = get_mach_importance( base_priority ); + /* adapted from the QoS table at xnu/osfmk/kern/thread_policy.c */ + switch (thread->priority) + { + case THREAD_PRIORITY_IDLE: /* THREAD_QOS_MAINTENANCE */ Nit: wineserver style doesn't indent cases.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7317#note_94877
Rémi Bernon (@rbernon) commented about server/thread.c:
THREAD_EXTENDED_POLICY_COUNT ); thread_policy_set( thread_port, THREAD_PRECEDENCE_POLICY, (thread_policy_t)&thread_precedence_policy, THREAD_PRECEDENCE_POLICY_COUNT ); + if (base_priority > THREAD_BASE_PRIORITY_LOWRT) + {
I don't really know about macOS, but I don't think implementing realtime priorities is a good idea. At least on Linux I would advise against it (and it needs specific permission anyway). It's IMO putting the system at a higher risk of becoming unresponsive in case of bogus or rogue application. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7317#note_94876
Fwiw I don't really know anything about macOS and can't really say anything useful about it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7317#note_94878
participants (3)
-
Marc-Aurel Zent -
Marc-Aurel Zent (＠mzent)
-
Rémi Bernon