[PATCH v11 0/4] MR4551: Server-side thread priorities implementation (Part 1)
This is the current proton thread priority implementation by @rbernon rebased for upstream with a few `#ifdef`s added since AFAIK Linux is the only operating system where threads have a unique PID which can be used to set niceness on. ~~I also ran `./tools/make_requests` on https://gitlab.winehq.org/mzent/wine/-/commit/6705d3481be0409f7e971c1d2c7a36... as well and `autoconf` on https://gitlab.winehq.org/mzent/wine/-/commit/d7bafe40c411753662b2ad97148a6c... (which does blow up the line count a bit).~~ A few tiny changes ~~(with the ready variable for example)~~ are in anticipation for Part 2, which also adds Mach thread priorities and recalculates thread priorities on process priority change. Since this is a rather large MR, I hope the split here is appropriate ~~(with the second part being slightly smaller)~~, but I think logically it makes the most sense here. -- v11: server: Check wineserver privileges on init with -20 niceness. server: Use setpriority to update thread niceness when safe. ntdll: Set RLIMIT_NICE to its hard limit. server: Introduce new set_thread_priority helper. https://gitlab.winehq.org/wine/wine/-/merge_requests/4551
From: Rémi Bernon <rbernon(a)codeweavers.com> --- server/thread.c | 44 +++++++++++++++++++++++++++++++------------- server/thread.h | 1 + 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/server/thread.c b/server/thread.c index 56f57cefd8f..46ae8f5471d 100644 --- a/server/thread.c +++ b/server/thread.c @@ -606,25 +606,39 @@ affinity_t get_thread_affinity( struct thread *thread ) #define THREAD_PRIORITY_REALTIME_HIGHEST 6 #define THREAD_PRIORITY_REALTIME_LOWEST -7 +int set_thread_priority( struct thread *thread, int priority_class, int priority ) +{ + int max = THREAD_PRIORITY_HIGHEST; + int min = THREAD_PRIORITY_LOWEST; + if (priority_class == PROCESS_PRIOCLASS_REALTIME) + { + max = THREAD_PRIORITY_REALTIME_HIGHEST; + min = THREAD_PRIORITY_REALTIME_LOWEST; + } + if ((priority < min || priority > max) && + priority != THREAD_PRIORITY_IDLE && + priority != THREAD_PRIORITY_TIME_CRITICAL) + { + errno = EINVAL; + return -1; + } + + if (thread->process->priority == priority_class && + thread->priority == priority) + return 0; + thread->priority = priority; + + return 0; +} + /* set all information about a thread */ static void set_thread_info( struct thread *thread, const struct set_thread_info_request *req ) { if (req->mask & SET_THREAD_INFO_PRIORITY) { - int max = THREAD_PRIORITY_HIGHEST; - int min = THREAD_PRIORITY_LOWEST; - if (thread->process->priority == PROCESS_PRIOCLASS_REALTIME) - { - max = THREAD_PRIORITY_REALTIME_HIGHEST; - min = THREAD_PRIORITY_REALTIME_LOWEST; - } - if ((req->priority >= min && req->priority <= max) || - req->priority == THREAD_PRIORITY_IDLE || - req->priority == THREAD_PRIORITY_TIME_CRITICAL) - thread->priority = req->priority; - else - set_error( STATUS_INVALID_PARAMETER ); + if (set_thread_priority( thread, thread->process->priority, req->priority )) + file_set_error(); } if (req->mask & SET_THREAD_INFO_AFFINITY) { @@ -1413,7 +1427,10 @@ DECL_HANDLER(init_first_thread) if (!process->parent_id) process->affinity = current->affinity = get_thread_affinity( current ); else + { + set_thread_priority( current, current->process->priority, current->priority ); set_thread_affinity( current, current->affinity ); + } debug_level = max( debug_level, req->debug_level ); @@ -1444,6 +1461,7 @@ DECL_HANDLER(init_thread) init_thread_context( current ); generate_debug_event( current, DbgCreateThreadStateChange, &req->entry ); + set_thread_priority( current, current->process->priority, current->priority ); set_thread_affinity( current, current->affinity ); reply->suspend = (current->suspend || current->process->suspend || current->context != NULL); diff --git a/server/thread.h b/server/thread.h index 8dcf966a90a..b0237c3a80e 100644 --- a/server/thread.h +++ b/server/thread.h @@ -119,6 +119,7 @@ extern void thread_cancel_apc( struct thread *thread, struct object *owner, enum extern int thread_add_inflight_fd( struct thread *thread, int client, int server ); extern int thread_get_inflight_fd( struct thread *thread, int client ); extern struct token *thread_get_impersonation_token( struct thread *thread ); +extern int set_thread_priority( struct thread *thread, int priority_class, int priority ); extern int set_thread_affinity( struct thread *thread, affinity_t affinity ); extern int suspend_thread( struct thread *thread ); extern int resume_thread( struct thread *thread ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4551
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/loader.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/ntdll/unix/loader.c b/dlls/ntdll/unix/loader.c index 803d8079213..40c55d58f76 100644 --- a/dlls/ntdll/unix/loader.c +++ b/dlls/ntdll/unix/loader.c @@ -2151,6 +2151,9 @@ DECLSPEC_EXPORT void __wine_main( int argc, char *argv[], char *envp[] ) #ifdef RLIMIT_AS set_max_limit( RLIMIT_AS ); #endif +#ifdef RLIMIT_NICE + set_max_limit( RLIMIT_NICE ); +#endif virtual_init(); init_environment(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4551
From: Rémi Bernon <rbernon(a)codeweavers.com> --- configure.ac | 10 ++++++++ server/main.c | 1 + server/object.h | 4 +++ server/thread.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+) diff --git a/configure.ac b/configure.ac index 9e550559589..27fa7b2e93e 100644 --- a/configure.ac +++ b/configure.ac @@ -2118,6 +2118,16 @@ then AC_DEFINE(HAVE_SCHED_SETAFFINITY, 1, [Define to 1 if you have the `sched_setaffinity' function.]) fi +AC_CACHE_CHECK([for setpriority],wine_cv_have_setpriority, + AC_LINK_IFELSE([AC_LANG_PROGRAM( +[[#define _GNU_SOURCE +#include <sys/resource.h> +#include <sys/time.h>]], [[setpriority(0, 0, 0);]])],[wine_cv_have_setpriority=yes],[wine_cv_have_setpriority=no])) +if test "$wine_cv_have_setpriority" = "yes" +then + AC_DEFINE(HAVE_SETPRIORITY, 1, [Define to 1 if you have the `setpriority' function.]) +fi + dnl **** Check for types **** AC_C_INLINE diff --git a/server/main.c b/server/main.c index 1248b92f24d..e014ec535ff 100644 --- a/server/main.c +++ b/server/main.c @@ -234,6 +234,7 @@ int main( int argc, char *argv[] ) init_signals(); init_memory(); init_directories( load_intl_file() ); + init_threading(); init_registry(); main_loop(); return 0; diff --git a/server/object.h b/server/object.h index dfdd691601f..66012fbc4af 100644 --- a/server/object.h +++ b/server/object.h @@ -277,6 +277,10 @@ extern struct object *get_directory_obj( struct process *process, obj_handle_t h extern int directory_link_name( struct object *obj, struct object_name *name, struct object *parent ); extern void init_directories( struct fd *intl_fd ); +/* thread functions */ + +extern void init_threading(void); + /* symbolic link functions */ extern struct object *create_root_symlink( struct object *root, const struct unicode_str *name, diff --git a/server/thread.c b/server/thread.c index 46ae8f5471d..e59d8f96efd 100644 --- a/server/thread.c +++ b/server/thread.c @@ -37,6 +37,9 @@ #define _WITH_CPU_SET_T #include <sched.h> #endif +#ifdef HAVE_SYS_RESOURCE_H +#include <sys/resource.h> +#endif #include "ntstatus.h" #define WIN32_NO_STATUS @@ -215,6 +218,27 @@ static const struct fd_ops thread_fd_ops = }; static struct list thread_list = LIST_INIT(thread_list); +#ifdef __linux__ +static int nice_limit; +#endif + +void init_threading(void) +{ +#ifdef __linux__ +#ifdef RLIMIT_NICE + struct rlimit rlimit; + if (!getrlimit( RLIMIT_NICE, &rlimit )) + { + rlimit.rlim_cur = rlimit.rlim_max; + setrlimit( RLIMIT_NICE, &rlimit ); + if (rlimit.rlim_max <= 40) nice_limit = 20 - rlimit.rlim_max; + else if (rlimit.rlim_max == -1) nice_limit = -20; + if (nice_limit >= 0 && debug_level) fprintf(stderr, "wine: RLIMIT_NICE is <= 20, unable to use setpriority safely\n"); + } +#endif + if (nice_limit < 0 && debug_level) fprintf(stderr, "wine: Using setpriority to control niceness in the [%d,%d] range\n", nice_limit, -nice_limit ); +#endif +} /* initialize the structure for a newly allocated thread */ static inline void init_thread_structure( struct thread *thread ) @@ -603,9 +627,50 @@ affinity_t get_thread_affinity( struct thread *thread ) return mask; } +static int get_base_priority( int priority_class, int priority ) +{ + /* offsets taken from https://learn.microsoft.com/en-us/windows/win32/procthread/scheduling-priori... */ + static const int class_offsets[] = { 4, 8, 13, 24, 6, 10 }; + if (priority == THREAD_PRIORITY_IDLE) return (priority_class == PROCESS_PRIOCLASS_REALTIME ? 16 : 1); + if (priority == THREAD_PRIORITY_TIME_CRITICAL) return (priority_class == PROCESS_PRIOCLASS_REALTIME ? 31 : 15); + if (priority_class >= ARRAY_SIZE(class_offsets)) return 8; + return class_offsets[priority_class - 1] + priority; +} + +#ifdef __linux__ +/* maps an NT application band [1,15] base priority to [-nice_limit, nice_limit] */ +static int get_unix_niceness( int base_priority ) +{ + int min = -nice_limit, max = nice_limit, range = max - min; + return min + (base_priority - 1) * range / 14; +} +#endif + #define THREAD_PRIORITY_REALTIME_HIGHEST 6 #define THREAD_PRIORITY_REALTIME_LOWEST -7 +static void apply_thread_priority( struct thread *thread, int priority_class, int priority ) +{ + int base_priority = get_base_priority( priority_class, priority ); +#ifdef __linux__ + int niceness; + + if (thread->unix_tid == -1) return; + + /* FIXME: handle REALTIME class using SCHED_RR if possible, for now map it to highest non-realtime band */ + if (priority_class == PROCESS_PRIOCLASS_REALTIME) base_priority = 15; +#ifdef HAVE_SETPRIORITY + if (nice_limit < 0) + { + niceness = get_unix_niceness( base_priority ); + if (setpriority( PRIO_PROCESS, thread->unix_tid, niceness ) != 0) + fprintf( stderr, "wine: setpriority %d for pid %d failed: %d\n", niceness, thread->unix_tid, errno ); + return; + } +#endif +#endif +} + int set_thread_priority( struct thread *thread, int priority_class, int priority ) { int max = THREAD_PRIORITY_HIGHEST; @@ -628,6 +693,7 @@ int set_thread_priority( struct thread *thread, int priority_class, int priority return 0; thread->priority = priority; + apply_thread_priority( thread, priority_class, priority ); return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4551
From: Rémi Bernon <rbernon(a)codeweavers.com> --- server/thread.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/server/thread.c b/server/thread.c index e59d8f96efd..14911a3dace 100644 --- a/server/thread.c +++ b/server/thread.c @@ -227,7 +227,14 @@ void init_threading(void) #ifdef __linux__ #ifdef RLIMIT_NICE struct rlimit rlimit; - if (!getrlimit( RLIMIT_NICE, &rlimit )) +#endif +#ifdef HAVE_SETPRIORITY + /* if wineserver has cap_sys_nice we are unlimited, but leave -20 to the user */ + if (!setpriority( PRIO_PROCESS, getpid(), -20 )) nice_limit = -19; + setpriority( PRIO_PROCESS, getpid(), 0 ); +#endif +#ifdef RLIMIT_NICE + if (!nice_limit && !getrlimit( RLIMIT_NICE, &rlimit )) { rlimit.rlim_cur = rlimit.rlim_max; setrlimit( RLIMIT_NICE, &rlimit ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4551
On Mon Jan 15 14:24:07 2024 +0000, Marc-Aurel Zent wrote:
I think this can happen when the thread gets killed by the looks of it (or alternatively `get_unix_tid()` returned -1, but in that case I don't see how it could fix itself later). Should be guarded against now, `set_thread_affinity()` has a similar check
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_58780
Alexandre Julliard (@julliard) commented about server/thread.c:
};
static struct list thread_list = LIST_INIT(thread_list); +#ifdef __linux__ +static int nice_limit; +#endif + +void init_threading(void) +{ +#ifdef __linux__ +#ifdef RLIMIT_NICE Please avoid unnecessary #ifdefs.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_59326
Alexandre Julliard (@julliard) commented about server/thread.c:
+static void apply_thread_priority( struct thread *thread, int priority_class, int priority ) +{ + int base_priority = get_base_priority( priority_class, priority ); +#ifdef __linux__ + int niceness; + + if (thread->unix_tid == -1) return; + + /* FIXME: handle REALTIME class using SCHED_RR if possible, for now map it to highest non-realtime band */ + if (priority_class == PROCESS_PRIOCLASS_REALTIME) base_priority = 15; +#ifdef HAVE_SETPRIORITY + if (nice_limit < 0) + { + niceness = get_unix_niceness( base_priority ); + if (setpriority( PRIO_PROCESS, thread->unix_tid, niceness ) != 0) + fprintf( stderr, "wine: setpriority %d for pid %d failed: %d\n", niceness, thread->unix_tid, errno ); Errors should be handled and propagated to the caller, not printed on the console.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_59327
Alexandre Julliard (@julliard) commented about server/thread.c:
+ int max = THREAD_PRIORITY_HIGHEST; + int min = THREAD_PRIORITY_LOWEST; + if (priority_class == PROCESS_PRIOCLASS_REALTIME) + { + max = THREAD_PRIORITY_REALTIME_HIGHEST; + min = THREAD_PRIORITY_REALTIME_LOWEST; + } + if ((priority < min || priority > max) && + priority != THREAD_PRIORITY_IDLE && + priority != THREAD_PRIORITY_TIME_CRITICAL) + { + errno = EINVAL; + return -1; + } + + if (thread->process->priority == priority_class && Internal functions should not use errno for error reporting.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_59328
Alexandre Julliard (@julliard) commented about configure.ac:
AC_DEFINE(HAVE_SCHED_SETAFFINITY, 1, [Define to 1 if you have the `sched_setaffinity' function.]) fi
+AC_CACHE_CHECK([for setpriority],wine_cv_have_setpriority, + AC_LINK_IFELSE([AC_LANG_PROGRAM( +[[#define _GNU_SOURCE +#include <sys/resource.h> +#include <sys/time.h>]], [[setpriority(0, 0, 0);]])],[wine_cv_have_setpriority=yes],[wine_cv_have_setpriority=no])) +if test "$wine_cv_have_setpriority" = "yes" +then + AC_DEFINE(HAVE_SETPRIORITY, 1, [Define to 1 if you have the `setpriority' function.]) +fi + Why do you need a compile check? Is the prototype different on other platforms?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_59329
On Mon Jan 29 10:05:59 2024 +0000, Alexandre Julliard wrote:
Why do you need a compile check? Is the prototype different on other platforms? The prototype is slightly different on darwin with `int setpriority(int which, id_t who, int prio);`, where `id_t` is typedefed to `__darwin_id_t` and then `__uint32_t`.
In any case it would still pass the check, but still cannot be used for per-thread priorities there. This function seems fairly ubiquitous since BSD 4.2 and is currently used only in the linux context, so I guess this can be safely removed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_59335
participants (3)
-
Alexandre Julliard (＠julliard) -
Marc-Aurel Zent (＠mzent)
-
Rémi Bernon