[PATCH v5 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. -- v5: server: Check wineserver privileges on init with -20 server: Use setpriority to update thread niceness when safe. 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 83ae381d5c5..5c039828912 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 +++ dlls/ntdll/unix/server.c | 14 ++++++++++++++ include/wine/server_protocol.h | 4 +++- server/process.h | 1 + server/protocol.def | 1 + server/request.h | 3 ++- server/thread.c | 7 +++++++ server/trace.c | 1 + 8 files changed, 32 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/loader.c b/dlls/ntdll/unix/loader.c index aa120ae38ec..9dd6c672336 100644 --- a/dlls/ntdll/unix/loader.c +++ b/dlls/ntdll/unix/loader.c @@ -2125,6 +2125,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( argc, argv, envp ); diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 7211457387c..bfb14173d60 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -53,6 +53,9 @@ # include <sys/prctl.h> #endif #include <sys/stat.h> +#ifdef HAVE_SYS_RESOURCE_H +# include <sys/resource.h> +#endif #ifdef HAVE_SYS_SYSCALL_H # include <sys/syscall.h> #endif @@ -1552,6 +1555,8 @@ size_t server_init_process(void) struct sigaction sig_act; size_t info_size; DWORD pid, tid; + struct rlimit rlimit; + int nice_limit = 0; server_pid = -1; if (env_socket) @@ -1613,10 +1618,19 @@ size_t server_init_process(void) reply_pipe = init_thread_pipe(); +#ifdef RLIMIT_NICE + if (!getrlimit( RLIMIT_NICE, &rlimit )) + { + if (rlimit.rlim_cur <= 40) nice_limit = 20 - rlimit.rlim_cur; + else if (rlimit.rlim_cur == -1 /* RLIMIT_INFINITY */) nice_limit = -20; + } +#endif + SERVER_START_REQ( init_first_thread ) { req->unix_pid = getpid(); req->unix_tid = get_unix_tid(); + req->nice_limit = nice_limit; req->reply_fd = reply_pipe; req->wait_fd = ntdll_get_thread_data()->wait_fd[1]; req->debug_level = (TRACE_ON(server) != 0); diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index a392b532f4e..f2fb0474803 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -988,6 +988,8 @@ struct init_first_thread_request int debug_level; int reply_fd; int wait_fd; + char nice_limit; + char __pad_33[7]; }; struct init_first_thread_reply { @@ -6504,7 +6506,7 @@ union generic_reply /* ### protocol_version begin ### */ -#define SERVER_PROTOCOL_VERSION 784 +#define SERVER_PROTOCOL_VERSION 785 /* ### protocol_version end ### */ diff --git a/server/process.h b/server/process.h index 97e0d455ece..203a97d6682 100644 --- a/server/process.h +++ b/server/process.h @@ -50,6 +50,7 @@ struct process timeout_t sigkill_delay; /* delay before final SIGKILL */ unsigned short machine; /* client machine type */ int unix_pid; /* Unix pid for final SIGKILL */ + int nice_limit; /* RLIMIT_NICE of the process */ int exit_code; /* process exit code */ int running_threads; /* number of threads running in this process */ timeout_t start_time; /* absolute time at process start */ diff --git a/server/protocol.def b/server/protocol.def index e9195df6b65..fb4aa32ad18 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -969,6 +969,7 @@ typedef struct int debug_level; /* new debug level */ int reply_fd; /* fd for reply pipe */ int wait_fd; /* fd for blocking calls pipe */ + char nice_limit; /* RLIMIT_NICE of new thread */ @REPLY process_id_t pid; /* process id of the new thread's process */ thread_id_t tid; /* thread id of the new thread */ diff --git a/server/request.h b/server/request.h index d6043c5fdc3..c2dc4a8ada4 100644 --- a/server/request.h +++ b/server/request.h @@ -786,7 +786,8 @@ C_ASSERT( FIELD_OFFSET(struct init_first_thread_request, unix_tid) == 16 ); C_ASSERT( FIELD_OFFSET(struct init_first_thread_request, debug_level) == 20 ); C_ASSERT( FIELD_OFFSET(struct init_first_thread_request, reply_fd) == 24 ); C_ASSERT( FIELD_OFFSET(struct init_first_thread_request, wait_fd) == 28 ); -C_ASSERT( sizeof(struct init_first_thread_request) == 32 ); +C_ASSERT( FIELD_OFFSET(struct init_first_thread_request, nice_limit) == 32 ); +C_ASSERT( sizeof(struct init_first_thread_request) == 40 ); C_ASSERT( FIELD_OFFSET(struct init_first_thread_reply, pid) == 8 ); C_ASSERT( FIELD_OFFSET(struct init_first_thread_reply, tid) == 12 ); C_ASSERT( FIELD_OFFSET(struct init_first_thread_reply, server_start) == 16 ); diff --git a/server/thread.c b/server/thread.c index 5c039828912..dcc90aded09 100644 --- a/server/thread.c +++ b/server/thread.c @@ -37,6 +37,12 @@ #define _WITH_CPU_SET_T #include <sched.h> #endif +#ifdef HAVE_SYS_TIME_H +#include <sys/time.h> +#endif +#ifdef HAVE_SYS_RESOURCE_H +#include <sys/resource.h> +#endif #include "ntstatus.h" #define WIN32_NO_STATUS @@ -1423,6 +1429,7 @@ DECL_HANDLER(init_first_thread) current->unix_pid = process->unix_pid = req->unix_pid; current->unix_tid = req->unix_tid; + process->nice_limit = req->nice_limit; if (!process->parent_id) process->affinity = current->affinity = get_thread_affinity( current ); diff --git a/server/trace.c b/server/trace.c index 55ccefa1746..5a099071752 100644 --- a/server/trace.c +++ b/server/trace.c @@ -1472,6 +1472,7 @@ static void dump_init_first_thread_request( const struct init_first_thread_reque fprintf( stderr, ", debug_level=%d", req->debug_level ); fprintf( stderr, ", reply_fd=%d", req->reply_fd ); fprintf( stderr, ", wait_fd=%d", req->wait_fd ); + fprintf( stderr, ", nice_limit=%c", req->nice_limit ); } static void dump_init_first_thread_reply( const struct init_first_thread_reply *req ) -- 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 | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+) diff --git a/configure.ac b/configure.ac index 074086c586b..b50f1d162aa 100644 --- a/configure.ac +++ b/configure.ac @@ -2090,6 +2090,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 dcc90aded09..2bac7457d10 100644 --- a/server/thread.c +++ b/server/thread.c @@ -221,6 +221,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 ) @@ -609,9 +630,46 @@ affinity_t get_thread_affinity( struct thread *thread ) return mask; } +static int get_base_priority( int priority_class, int priority ) +{ + static const int class_offsets[] = { 4, 8, 13, 24, 6, 10 }; + assert(priority_class <= ARRAY_SIZE(class_offsets)); + if (priority == THREAD_PRIORITY_IDLE) return (priority_class == PROCESS_PRIOCLASS_REALTIME ? 16 : 1); + else if (priority == THREAD_PRIORITY_TIME_CRITICAL) return (priority_class == PROCESS_PRIOCLASS_REALTIME ? 31 : 15); + else return class_offsets[priority_class - 1] + priority; +} + +#ifdef __linux__ +static int get_unix_niceness( int base_priority, int limit ) +{ + int min = -limit, max = 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, limit = min( nice_limit, thread->process->nice_limit ); + + /* 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 (limit < 0) + { + niceness = get_unix_niceness( base_priority, limit ); + 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; @@ -634,6 +692,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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/thread.c b/server/thread.c index 2bac7457d10..e3baefcf240 100644 --- a/server/thread.c +++ b/server/thread.c @@ -230,7 +230,13 @@ void init_threading(void) #ifdef __linux__ #ifdef RLIMIT_NICE struct rlimit rlimit; - if (!getrlimit( RLIMIT_NICE, &rlimit )) +#endif +#ifdef HAVE_SETPRIORITY + if (setpriority( PRIO_PROCESS, getpid(), -20 ) == 0) 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
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=140625 Your paranoid android. === debian11 (build log) === error: patch failed: include/wine/server_protocol.h:6504 Task: Patch failed to apply === debian11b (build log) === error: patch failed: include/wine/server_protocol.h:6504 Task: Patch failed to apply
On Fri Dec 1 16:01:50 2023 +0000, Marc-Aurel Zent wrote:
changed this line in [version 4 of the diff](/wine/wine/-/merge_requests/4551/diffs?diff_id=87408&start_sha=b8d702590c6094f4fcb70e9a5692e7929d536e31#e54a93e094a94cfc85d5c061decb6b27dbd311f1_689_665) Alright dropped that commit and the delayed parameter in `apply_thread_priority` (and removed the autoconf changes to configure while being at it).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54593
Rémi Bernon (@rbernon) commented about server/thread.c:
#ifdef __linux__ #ifdef RLIMIT_NICE struct rlimit rlimit; - if (!getrlimit( RLIMIT_NICE, &rlimit )) +#endif +#ifdef HAVE_SETPRIORITY + if (setpriority( PRIO_PROCESS, getpid(), -20 ) == 0) nice_limit = -19; + setpriority( PRIO_PROCESS, getpid(), 0 ); +#endif +#ifdef RLIMIT_NICE + if (!nice_limit && !getrlimit( RLIMIT_NICE, &rlimit )) Maybe this could be made clearer in either a comment or the commit title, but the idea is that some users wanted to be able to set privileges through `setcap 'cap_sys_nice=eip'` on wineserver alone, instead of configuring system rlimit.
The if could also be shortened, and the commit message is missing a dot. ```suggestion:-5+0 #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 )) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54607
Rémi Bernon (@rbernon) commented about server/thread.c:
+#ifdef __linux__ +static int get_unix_niceness( int base_priority, int limit ) +{ + int min = -limit, max = 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, limit = min( nice_limit, thread->process->nice_limit ); As nice_limit is usually negative, I think this should be max instead, as `wineserver` will be calling call `setpriority`?
```suggestion:-0+0 int niceness, limit = max( nice_limit, thread->process->nice_limit ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54608
Rémi Bernon (@rbernon) commented about server/thread.c:
return mask; }
+static int get_base_priority( int priority_class, int priority ) +{ + static const int class_offsets[] = { 4, 8, 13, 24, 6, 10 }; + assert(priority_class <= ARRAY_SIZE(class_offsets)); + if (priority == THREAD_PRIORITY_IDLE) return (priority_class == PROCESS_PRIOCLASS_REALTIME ? 16 : 1); + else if (priority == THREAD_PRIORITY_TIME_CRITICAL) return (priority_class == PROCESS_PRIOCLASS_REALTIME ? 31 : 15); + else return class_offsets[priority_class - 1] + priority;
I think asserts are not well appreciated on the server side, maybe just a check if it's out of bounds. Also elses are unnecessary: ```suggestion:-4+0 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 16; return class_offsets[priority_class - 1] + priority; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54609
On Fri Dec 1 18:33:19 2023 +0000, Rémi Bernon wrote:
As nice_limit is usually negative, I think this should be max instead, as `wineserver` will be calling call `setpriority`? ```suggestion:-0+0 int niceness, limit = max( nice_limit, thread->process->nice_limit ); ``` Hmm actually no, but still it makes me wonder if there's actually a point in passing the process nice limit. I'm not completely sure how setpriority works but if it only uses the caller limits and if wineserver is more limited than the process itself it won't succeed anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54610
On Fri Dec 1 18:37:13 2023 +0000, Rémi Bernon wrote:
Hmm actually no, but still it makes me wonder if there's actually a point in passing the process nice limit. I'm not completely sure how setpriority works but if it only uses the caller limits and if wineserver is more limited than the process itself it won't succeed anyway. I think you can also drop 896e234327a843665c4360252c36221bbcc92685, I don't see how it is useful.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54612
On Fri Dec 1 18:40:48 2023 +0000, Rémi Bernon wrote:
I think you can also drop 896e234327a843665c4360252c36221bbcc92685, I don't see how it is useful. Looking at the man page I think only the target limit matters and probably the reason for https://gitlab.winehq.org/wine/wine/-/merge_requests/4551/diffs?commit_id=89... (I think)
Since Linux 2.6.12, this error only occurs if the caller attempts to set a process priority outside the range of the RLIMIT_NICE soft resource limit of the target process; see getrlimit(2) for details.
So the server priority shouldn't matter at all if I understand this correctly? Will do a bit of testing on that -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54614
On Fri Dec 1 18:48:12 2023 +0000, Marc-Aurel Zent wrote:
Looking at the man page I think only the target limit matters and probably the reason for https://gitlab.winehq.org/wine/wine/-/merge_requests/4551/diffs?commit_id=89... (I think)
Since Linux 2.6.12, this error only occurs if the caller attempts to set a process priority outside the range of the RLIMIT_NICE soft resource limit of the target process; see getrlimit(2) for details. So the server priority shouldn't matter at all if I understand this correctly? Will do a bit of testing on that Ah yeah looks like you're right... Hmm. The server priority might matter in the case where it's priviledged where maybe it can succeed regardless of the target rlimit?
The `min` is probably somehow there tell between the two, but only if we assume the rlimit isn't changed between wineserver start and process start, so maybe a dedicated flag (or server nice limit value) would be better to handle the priviledged case. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54615
On Fri Dec 1 19:11:16 2023 +0000, Rémi Bernon wrote:
Ah yeah looks like you're right... Hmm. The server priority might matter in the case where it's privileged where maybe it can succeed regardless of the target rlimit? The `min` is probably somehow there tell between the two, but only if we assume the rlimit isn't changed between wineserver start and process start, so maybe a dedicated flag would be better to handle the privileged case. Hm it seems to me like every process can set its own niceness to [limit, 19]. Making the assumption that the hard limit for wineserver and wine processes are the same (which it probably always will be unless the user did something special), there probably is no need to send back the nice limit of the target process, only for each to raise its limit.
For CAP_SYS_NICE (or euid 0) though the target process limit does not matter at all, so taking the -20 limit of the server there should be fine as well. Would be a partial revert of https://gitlab.winehq.org/wine/wine/-/merge_requests/4551/diffs?commit_id=89... and adjusting the logic to go from [limit, -limit] to [limit, 19], unless they have to be symmetrical? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54618
On Fri Dec 1 19:43:32 2023 +0000, Marc-Aurel Zent wrote:
Hm it seems to me like every process can set its own niceness to [limit, 19]. Making the assumption that the hard limit for wineserver and wine processes are the same (which it probably always will be unless the user did something special), there probably is no need to send back the nice limit of the target process, only for each to raise its limit. For CAP_SYS_NICE (or euid 0) though the target process limit does not matter at all, so taking the -20 limit of the server there should be fine as well. Would be a partial revert of https://gitlab.winehq.org/wine/wine/-/merge_requests/4551/diffs?commit_id=89... and adjusting the logic to go from [limit, -limit] to [limit, 19], unless they have to be symmetrical? Yeah I think it's probably fine to assume that the rlimit for every process will be the same, and if it's not `SetThreadPriority` will just return an error.
The symmetry was there to make the range mapping easier, as well as to keep the symmetry of Windows thread priorities. I think it's best to keep it like this as it gives control over both the lowest and highest priority at the same time. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4551#note_54625
participants (3)
-
Marc-Aurel Zent (＠mzent) -
Marvin -
Rémi Bernon