[PATCH v2 0/19] MR7516: Server-side thread priorities implementation (Part 3)

[Marc-Aurel Zent (＠mzent)]

This is the continuation of https://gitlab.winehq.org/wine/wine/-/merge_requests/7317.

There are a lot of smaller commits (some even no-op renames), because there are some inconsistencies in the codebase currently of what an NT thread priority vs a base thread priority should be and I hope that this clears that up a bit.

I tried making the commits as atomic as possible I hope this is fine... There are still a few tiny details missing like fixing up the `list_processes` request and `KeSetPriorityThread` and friends which are either stubs or not correctly working as well.

There are also probably a few tests that can be written here (after thread priority boosting has been disabled, cause that makes the behavior on windows very flaky and dynamic). In fact on windows there is some thread priority boost decay going on after the message has been processed (and probably other boosting mechanisms), which this MR does not properly capture, but are also probably not reasonable to implement.

This also reverts the commits of https://gitlab.winehq.org/wine/wine/-/merge_requests/1232, which are being effectively overwritten anyways.

The `get_thread_priority_info` request was added, because the reply in `get_thread_info` would be otherwise larger than 64 bytes, so it had to be split.

--
  v2: Revert "ntdll: Set the QoS class of the main Wine thread on macOS."
      Revert "ntdll: Fix runtime availability check for pthread_attr_set_qos_class_np."
 
This merge request has too many patches to be relayed via email.
Please visit the URL below to see the contents of the merge request.
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about include/winnt.h:
> #define THREAD_BASE_PRIORITY_MIN   -2
>  #define THREAD_BASE_PRIORITY_IDLE  -15
>
> +#define LOW_PRIORITY 0              /* Lowest thread priority level */
> +#define LOW_REALTIME_PRIORITY 16    /* Lowest realtime priority level */
> +#define HIGH_PRIORITY 31            /* Highest thread priority level */
> +#define MAXIMUM_PRIORITY 32         /* Number of thread priority levels */
> +
> This could be added first.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97073

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about server/thread.c:
> int min = -nice_limit, max = nice_limit, range = max - min, niceness;
>
> /* FIXME: handle realtime priorities using SCHED_RR if possible */
> -    if (effective_priority > THREAD_BASE_PRIORITY_LOWRT) effective_priority = THREAD_BASE_PRIORITY_LOWRT;
> +    if (effective_priority >= LOW_REALTIME_PRIORITY) effective_priority = LOW_REALTIME_PRIORITY - 1;

And this probably squashed with the `effective_priority` renames.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97074

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about server/thread.c:
> thread->wait_fd         = NULL;
>      thread->state           = RUNNING;
>      thread->exit_code       = 0;
> -    thread->priority        = 0;
> +    thread->base_priority   = 0;

I don't think `base_priority` makes anything clearer. https://learn.microsoft.com/en-us/windows/win32/procthread/scheduling-priori... uses that term for the absolute values, which is not what this is. Event though the ntdll constant cannot be changed, I think we should maybe use `priority_level` instead to describe the class-relative priority everywhere we can.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97072

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about server/thread.c:
> return mask;
>  }
>
> -static int get_base_priority( int priority_class, int priority )
> +static int calculate_thread_priority( int priority_class, int priority )

Can we squash this with previous commit and name it for instance `thread_priority_from_class_and_level`?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97071

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about server/thread.c:
> thread->exit_code       = 0;
>      thread->priority        = 0;
>      thread->base_priority   = 0;
> +    thread->disable_boost   = 0;

Let's split the boost handling to a separate MR. Fwiw I don't think adding fields to structure deserve separate commits.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97077

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/thread.c:
> }
>
> +    case ThreadPriority:
> +    {
> +        const DWORD *pprio = data;
> +        if (length != sizeof(DWORD)) return STATUS_INVALID_PARAMETER;
> +        SERVER_START_REQ( set_thread_info )
> +        {
> +            req->handle    = wine_server_obj_handle( handle );
> +            req->priority  = *pprio;
> +            req->mask      = SET_THREAD_INFO_PRIORITY;
> +            status = wine_server_call( req );
> +        }
> +        SERVER_END_REQ;
> +        return status;
> +    }

Would be nice to have tests for this addition.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97078

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about server/thread.h:
> client_ptr_t           teb;           /* TEB address (in client address space) */
>      client_ptr_t           entry_point;   /* entry point (in client address space) */
>      affinity_t             affinity;      /* affinity mask */
> -    int                    base_priority; /* priority level */
> +    int                    priority;      /* priority level */
> +    int                    base_priority; /* base priority class */

Same here, doesn't really deserve a commit and could be squashed with next change.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97079

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/thread.c:
> +        }
> +        SERVER_END_REQ;
> +        if (status == STATUS_SUCCESS)
> +        {
> +            SERVER_START_REQ( get_thread_priority_info )
> +            {
> +                req->handle = wine_server_obj_handle( handle );
> +                if (!(status = wine_server_call( req )))
> +                {
>                      info.Priority           = reply->priority;
> -                info.BasePriority           = reply->priority;  /* FIXME */
> +                    info.BasePriority       = reply->base_priority;
>                  }
>              }
>              SERVER_END_REQ;
> +        }
> This makes two requests instead of one, I don't think this is good. You should find a way to squeeze the information in, increase the fixed request size, or use varargs if the data doesn't fit in the fixed request part.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97075

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/kernelbase/thread.c:
> if (!set_ntstatus( NtQueryInformationThread( thread, ThreadBasicInformation,
>                                                   &info, sizeof(info), NULL )))
>          return THREAD_PRIORITY_ERROR_RETURN;
> -    return info.Priority;
> +    return info.BasePriority;

This could probably be squashed with previous change.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7516#note_97076