[PATCH 0/1] MR9001: ntdll: Introduce a helper to wait on a server-side sync object.
Some objects like async or completion waits are internal only, and their sync logic is still a bit complicated as they have various server side effects. They are only waited on alone, we can keep using server-side syncs for them. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001
From: Elizabeth Figura <zfigura(a)codeweavers.com> Some objects like async or completion waits are internal only, and their sync logic is still a bit complicated as they have various server side effects. They are only waited on alone, we can keep using server-side syncs for them. --- dlls/ntdll/unix/server.c | 15 +++++++++++++++ dlls/ntdll/unix/sync.c | 4 ++-- dlls/ntdll/unix/unix_private.h | 3 ++- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index d44b4b90f3d..bbca17aee8f 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -818,6 +818,21 @@ unsigned int server_wait( const union select_op *select_op, data_size_t size, UI } +/* helper function to perform a server-side wait on an internal handle without + * using the fast synchronization path */ +unsigned int server_wait_for_object( HANDLE handle, BOOL alertable, const LARGE_INTEGER *timeout ) +{ + union select_op select_op; + UINT flags = SELECT_INTERRUPTIBLE; + + if (alertable) flags |= SELECT_ALERTABLE; + + select_op.wait.op = SELECT_WAIT; + select_op.wait.handles[0] = wine_server_obj_handle( handle ); + return server_wait( &select_op, offsetof( union select_op, wait.handles[1] ), flags, timeout ); +} + + /*********************************************************************** * NtContinue (NTDLL.@) */ diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 9ba56490c33..dd18c948d22 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -2328,7 +2328,7 @@ NTSTATUS WINAPI NtRemoveIoCompletion( HANDLE handle, ULONG_PTR *key, ULONG_PTR * } SERVER_END_REQ; if (status != STATUS_PENDING) return status; - if (!timeout || timeout->QuadPart) status = NtWaitForSingleObject( wait_handle, FALSE, timeout ); + if (!timeout || timeout->QuadPart) status = server_wait_for_object( wait_handle, FALSE, timeout ); else status = STATUS_TIMEOUT; if (status != WAIT_OBJECT_0) return status; @@ -2392,7 +2392,7 @@ NTSTATUS WINAPI NtRemoveIoCompletionEx( HANDLE handle, FILE_IO_COMPLETION_INFORM assert( status == STATUS_USER_APC ); goto done; } - if (!timeout || timeout->QuadPart) status = NtWaitForSingleObject( wait_handle, alertable, timeout ); + if (!timeout || timeout->QuadPart) status = server_wait_for_object( wait_handle, alertable, timeout ); else status = STATUS_TIMEOUT; if (status != WAIT_OBJECT_0) goto done; diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index f514f61dd60..75c04e5ec15 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -227,6 +227,7 @@ extern unsigned int server_select( const union select_op *select_op, data_size_t timeout_t abs_timeout, struct context_data *context, struct user_apc *user_apc ); extern unsigned int server_wait( const union select_op *select_op, data_size_t size, UINT flags, const LARGE_INTEGER *timeout ); +extern unsigned int server_wait_for_object( HANDLE handle, BOOL alertable, const LARGE_INTEGER *timeout ); extern unsigned int server_queue_process_apc( HANDLE process, const union apc_call *call, union apc_result *result ); extern int server_get_unix_fd( HANDLE handle, unsigned int wanted_access, int *unix_fd, @@ -467,7 +468,7 @@ static inline struct async_data server_async( HANDLE handle, struct async_fileio static inline NTSTATUS wait_async( HANDLE handle, BOOL alertable ) { - return NtWaitForSingleObject( handle, alertable, NULL ); + return server_wait_for_object( handle, alertable, NULL ); } static inline BOOL in_wow64_call(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9001
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001
Do we need to change completion wait and async logic to make them work with inproc syncs too? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001#note_116714
I am afraid changing completion wait logic this way is not possible without compromising its correctness (or additional logic implementation on the kernel side). Completion ports have their own wake queue logic and satisfied wait handling. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001#note_116716
The wake logic should probably be easy, it's just a matter of signaling the sync at specific times. The satisfied logic on the other hand is perhaps more complicated, but I think there's always a `get_thread_completion` call after the wait so maybe it could be moved there? Just an idea though, as I haven't really investigated. The async looks much more difficult to me to change, the waits are spread out a bit everywhere on the client side. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001#note_116717
Completion wait can timeout and go away without extra server call. Waiting thread can also be killed. With generic event the server has no way of knowing which threads are waiting but it has to know and assign completion to thread transactionally with completing wait. Without breaking this logic badly (only slightly) it is maybe possible to do somehow but it is going to be hard and complicated. Why would we need to rush this approach, given we are not even going to get rid of server calls for completion wait? IMO the only good way is yo implement completion queue in keenel ntsync first. But before that we’d probably want to test and implement thread count limit for completion ports in wineserver. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001#note_116718
Note that for a start to do the queue at least somehow on top of events it will need a separate event for each thread assigned to port. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001#note_116719
After discussing about this a bit with @julliard I understand it is alright to have some object types still use server syncs for now, but it would be nice to eventually have them implemented through inproc syncs as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9001#note_116971
participants (3)
-
Elizabeth Figura -
Paul Gofman (＠gofman)
-
Rémi Bernon