[PATCH 0/2] MR9014: server: Create an inproc sync for user APC signaling.
Unlike the message queue for which I believe we can get rid of any special handling, I don't see a way to avoid a dedicated inproc sync for user APCs. This will make sure that the sync gets created on thread creation. It could arguably be lazily allocated instead, but I don't see a reason to delay the failure to a later time and end up with successfully created but unalertable thread when they are supposed to be. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9014
From: Elizabeth Figura <zfigura(a)codeweavers.com> --- server/thread.c | 11 +++++++++++ server/thread.h | 1 + 2 files changed, 12 insertions(+) diff --git a/server/thread.c b/server/thread.c index fba1f69d284..853aff4cc84 100644 --- a/server/thread.c +++ b/server/thread.c @@ -397,6 +397,7 @@ static inline void init_thread_structure( struct thread *thread ) int i; thread->sync = NULL; + thread->alert_sync = NULL; thread->unix_pid = -1; /* not known yet */ thread->unix_tid = -1; /* not known yet */ thread->context = NULL; @@ -560,6 +561,7 @@ struct thread *create_thread( int fd, struct process *process, const struct secu } if (!(thread->request_fd = create_anonymous_fd( &thread_fd_ops, fd, &thread->obj, 0 ))) goto error; if (!(thread->sync = create_event_sync( 1, 0 ))) goto error; + if (get_inproc_device_fd() >= 0 && !(thread->alert_sync = create_inproc_internal_sync( 1, 0 ))) goto error; if (process->desktop) { @@ -654,6 +656,7 @@ static void destroy_thread( struct object *obj ) release_object( thread->process ); if (thread->id) free_ptid( thread->id ); if (thread->token) release_object( thread->token ); + if (thread->alert_sync) release_object( thread->alert_sync ); if (thread->sync) release_object( thread->sync ); } @@ -1440,7 +1443,11 @@ static int queue_apc( struct process *process, struct thread *thread, struct thr grab_object( apc ); list_add_tail( queue, &apc->entry ); if (!list_prev( queue, &apc->entry )) /* first one */ + { + if (apc->call.type == APC_USER && thread->alert_sync) + signal_inproc_sync( thread->alert_sync ); wake_thread( thread ); + } return 1; } @@ -1472,6 +1479,8 @@ void thread_cancel_apc( struct thread *thread, struct object *owner, enum apc_ty apc->executed = 1; signal_sync( apc->sync ); release_object( apc ); + if (list_empty( &thread->user_apc ) && thread->alert_sync) + reset_inproc_sync( thread->alert_sync ); return; } } @@ -1486,6 +1495,8 @@ static struct thread_apc *thread_dequeue_apc( struct thread *thread, int system { apc = LIST_ENTRY( ptr, struct thread_apc, entry ); list_remove( ptr ); + if (list_empty( &thread->user_apc ) && thread->alert_sync) + reset_inproc_sync( thread->alert_sync ); } return apc; } diff --git a/server/thread.h b/server/thread.h index 1418e4c5039..9c552a88ed2 100644 --- a/server/thread.h +++ b/server/thread.h @@ -51,6 +51,7 @@ struct thread { struct object obj; /* object header */ struct event_sync *sync; /* sync object for wait/signal */ + struct inproc_sync *alert_sync; /* inproc sync for user apc alerts */ struct list entry; /* entry in system-wide thread list */ struct list proc_entry; /* entry in per-process thread list */ struct list desktop_entry; /* entry in per-desktop thread list */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9014
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/server.c | 5 +++++ dlls/ntdll/unix/thread.c | 19 ++++++++++++++++--- dlls/ntdll/unix/unix_private.h | 1 + dlls/ntdll/unix/virtual.c | 9 +++++---- server/inproc_sync.c | 17 ++++++++++++++--- server/object.h | 1 + server/protocol.def | 2 ++ server/thread.c | 15 +++++++++++++-- 8 files changed, 57 insertions(+), 12 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index d44b4b90f3d..2f1773c017f 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -1662,6 +1662,11 @@ size_t server_init_process(void) inproc_device_fd = wine_server_receive_fd( &handle ); assert( handle == reply->inproc_device ); } + if (reply->alert_handle) + { + data->alert_sync_fd = wine_server_receive_fd( &handle ); + assert( handle == reply->alert_handle ); + } } } SERVER_END_REQ; diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index 65e7fbcb6bf..331a4eab08f 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -1104,6 +1104,7 @@ static void contexts_from_server( CONTEXT *context, struct context_data server_c */ static DECLSPEC_NORETURN void pthread_exit_wrapper( int status ) { + close( ntdll_get_thread_data()->alert_sync_fd ); close( ntdll_get_thread_data()->wait_fd[0] ); close( ntdll_get_thread_data()->wait_fd[1] ); close( ntdll_get_thread_data()->reply_fd ); @@ -1324,10 +1325,11 @@ NTSTATUS WINAPI NtCreateThreadEx( HANDLE *handle, ACCESS_MASK access, OBJECT_ATT struct object_attributes *objattr; struct ntdll_thread_data *thread_data; DWORD tid = 0; - int request_pipe[2]; + int request_pipe[2], alert_sync_fd = -1; TEB *teb; WOW_TEB *wow_teb; unsigned int status; + obj_handle_t token; if (flags & ~supported_flags) FIXME( "Unsupported flags %#x.\n", flags ); @@ -1377,6 +1379,8 @@ NTSTATUS WINAPI NtCreateThreadEx( HANDLE *handle, ACCESS_MASK access, OBJECT_ATT if (!access) access = THREAD_ALL_ACCESS; + server_enter_uninterrupted_section( &fd_cache_mutex, &sigset ); + SERVER_START_REQ( new_thread ) { req->process = wine_server_obj_handle( process ); @@ -1384,8 +1388,13 @@ NTSTATUS WINAPI NtCreateThreadEx( HANDLE *handle, ACCESS_MASK access, OBJECT_ATT req->flags = flags; req->request_fd = request_pipe[0]; wine_server_add_data( req, objattr, len ); - if (!(status = wine_server_call( req ))) + if (!(status = server_call_unlocked( req ))) { + if (reply->alert_handle) + { + alert_sync_fd = wine_server_receive_fd( &token ); + assert( token == reply->alert_handle ); + } *handle = wine_server_ptr_handle( reply->handle ); tid = reply->tid; } @@ -1393,6 +1402,8 @@ NTSTATUS WINAPI NtCreateThreadEx( HANDLE *handle, ACCESS_MASK access, OBJECT_ATT } SERVER_END_REQ; + server_leave_uninterrupted_section( &fd_cache_mutex, &sigset ); + free( objattr ); if (status) { @@ -1417,7 +1428,8 @@ NTSTATUS WINAPI NtCreateThreadEx( HANDLE *handle, ACCESS_MASK access, OBJECT_ATT if (wow_teb) wow_teb->SkipThreadAttach = teb->SkipThreadAttach; thread_data = (struct ntdll_thread_data *)&teb->GdiTebBatch; - thread_data->request_fd = request_pipe[1]; + thread_data->request_fd = request_pipe[1]; + thread_data->alert_sync_fd = alert_sync_fd; thread_data->start = start; thread_data->param = param; @@ -1439,6 +1451,7 @@ done: if (status) { NtClose( *handle ); + close( alert_sync_fd ); close( request_pipe[1] ); return status; } diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index f514f61dd60..b757fe5c3d1 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -108,6 +108,7 @@ struct ntdll_thread_data int request_fd; /* fd for sending server requests */ int reply_fd; /* fd for receiving server replies */ int wait_fd[2]; /* fd for sleeping server requests */ + int alert_sync_fd; /* inproc sync fd for user apc alerts */ BOOL allow_writes; /* ThreadAllowWrites flags */ pthread_t pthread_id; /* pthread thread id */ void *kernel_stack; /* stack for thread startup and kernel syscalls */ diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 994f76fb72a..a6854adfe36 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -4022,10 +4022,11 @@ static TEB *init_teb( void *ptr, BOOL is_wow ) teb->StaticUnicodeString.Buffer = teb->StaticUnicodeBuffer; teb->StaticUnicodeString.MaximumLength = sizeof(teb->StaticUnicodeBuffer); thread_data = (struct ntdll_thread_data *)&teb->GdiTebBatch; - thread_data->request_fd = -1; - thread_data->reply_fd = -1; - thread_data->wait_fd[0] = -1; - thread_data->wait_fd[1] = -1; + thread_data->request_fd = -1; + thread_data->reply_fd = -1; + thread_data->wait_fd[0] = -1; + thread_data->wait_fd[1] = -1; + thread_data->alert_sync_fd = -1; list_add_head( &teb_list, &thread_data->entry ); return teb; } diff --git a/server/inproc_sync.c b/server/inproc_sync.c index 41a9c2770a7..f364129b35a 100644 --- a/server/inproc_sync.c +++ b/server/inproc_sync.c @@ -87,6 +87,12 @@ static const struct object_ops inproc_sync_ops = inproc_sync_destroy, /* destroy */ }; +int get_inproc_sync_fd( struct inproc_sync *sync ) +{ + if (!sync) return -1; + return sync->fd; +} + struct inproc_sync *create_inproc_internal_sync( int manual, int signaled ) { struct ntsync_event_args args = {.signaled = signaled, .manual = manual}; @@ -133,7 +139,7 @@ static void inproc_sync_destroy( struct object *obj ) close( sync->fd ); } -static int get_inproc_sync_fd( struct object *obj, int *type ) +static int get_obj_inproc_sync( struct object *obj, int *type ) { struct object *sync; int fd = -1; @@ -160,6 +166,11 @@ int get_inproc_device_fd(void) return -1; } +int get_inproc_sync_fd( struct inproc_sync *sync ) +{ + return -1; +} + struct inproc_sync *create_inproc_internal_sync( int manual, int signaled ) { return NULL; @@ -173,7 +184,7 @@ void reset_inproc_sync( struct inproc_sync *sync ) { } -static int get_inproc_sync_fd( struct object *obj, int *type ) +static int get_obj_inproc_sync( struct object *obj, int *type ) { return -1; } @@ -189,7 +200,7 @@ DECL_HANDLER(get_inproc_sync_fd) reply->access = get_handle_access( current->process, req->handle ); - if ((fd = get_inproc_sync_fd( obj, &reply->type )) < 0) set_error( STATUS_NOT_IMPLEMENTED ); + if ((fd = get_obj_inproc_sync( obj, &reply->type )) < 0) set_error( STATUS_NOT_IMPLEMENTED ); else send_client_fd( current->process, fd, req->handle ); release_object( obj ); diff --git a/server/object.h b/server/object.h index 203734a565f..995041c2dcf 100644 --- a/server/object.h +++ b/server/object.h @@ -244,6 +244,7 @@ extern void abandon_mutexes( struct thread *thread ); struct inproc_sync; extern int get_inproc_device_fd(void); +extern int get_inproc_sync_fd( struct inproc_sync *sync ); extern struct inproc_sync *create_inproc_internal_sync( int manual, int signaled ); extern void signal_inproc_sync( struct inproc_sync *sync ); extern void reset_inproc_sync( struct inproc_sync *sync ); diff --git a/server/protocol.def b/server/protocol.def index 0c8be12314a..262efd1d56e 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1102,6 +1102,7 @@ typedef volatile struct @REPLY thread_id_t tid; /* thread id */ obj_handle_t handle; /* thread handle (in the current process) */ + obj_handle_t alert_handle; /* alert fd is in flight with this handle */ @END @@ -1137,6 +1138,7 @@ typedef volatile struct timeout_t server_start; /* server start time */ unsigned int session_id; /* process session id */ obj_handle_t inproc_device;/* inproc device fd in flight with this handle */ + obj_handle_t alert_handle; /* alert fd is in flight with this handle */ data_size_t info_size; /* total size of startup info */ VARARG(machines,ushorts); /* array of supported machines */ @END diff --git a/server/thread.c b/server/thread.c index 853aff4cc84..72817681e25 100644 --- a/server/thread.c +++ b/server/thread.c @@ -1631,7 +1631,7 @@ DECL_HANDLER(new_thread) struct unicode_str name; const struct security_descriptor *sd; const struct object_attributes *objattr = get_req_object_attributes( &sd, &name, NULL ); - int request_fd = thread_get_inflight_fd( current, req->request_fd ); + int fd, request_fd = thread_get_inflight_fd( current, req->request_fd ); if (!(process = get_process_from_handle( req->process, 0 ))) { @@ -1676,6 +1676,12 @@ DECL_HANDLER(new_thread) if ((reply->handle = alloc_handle_no_access_check( current->process, thread, req->access, objattr->attributes ))) { + /* first thread fd will be sent in init_first_thread */ + if (request_fd != -1 && (fd = get_inproc_sync_fd( thread->alert_sync )) >= 0) + { + reply->alert_handle = get_thread_id( thread ) | 1; /* arbitrary token */ + send_client_fd( thread->process, fd, reply->alert_handle ); + } /* thread object will be released when the thread gets killed */ goto done; } @@ -1746,9 +1752,14 @@ DECL_HANDLER(init_first_thread) if ((fd = get_inproc_device_fd()) >= 0) { - reply->inproc_device = get_process_id( process ) | 1; + reply->inproc_device = get_process_id( process ) | 1; /* arbitrary token */ send_client_fd( process, fd, reply->inproc_device ); } + if ((fd = get_inproc_sync_fd( current->alert_sync )) >= 0) + { + reply->alert_handle = get_thread_id( current ) | 1; /* arbitrary token */ + send_client_fd( process, fd, reply->alert_handle ); + } } /* initialize a new thread */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9014
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9014
Is there something wrong with doing that? The ntsync functions take a separate alert fd to implement the alerted flag, so I think there has to be a separate object. Do you have an idea how to do it differently? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9014#note_116715
@zfigura I asked about this a bit at WineConf, I understand it should be alright for now to have a dedicated fd for user alerts, but it would be nice to investigate other means to notify waiting threads, using signals for instance. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9014#note_116972
@zfigura I asked about this a bit at WineConf, I understand it should be alright for now to have a dedicated fd for user alerts, but it would be nice to investigate other means to notify waiting threads, using signals for instance.
That's frustrating to hear, because I actually initially designed the kernel pathces that way, explicitly asked Alexandre about that early on in development, and got the feedback that it would be better to make it an fd just like all the others. Not to mention that using signals would require some ppoll-like blocking logic which would have made the ntsync driver no longer fully self-contained. So that I don't go down a wrong path again, Alexandre, can you please explain what's wrong with the current approach? Why would signals be better? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9014#note_117055
I'm not suggesting to use signals to call the APC function, just that what we need here is a simple "APC pending" flag. I understand that it makes sense to map this to an fd in your design, it just seems rather heavyweight to have to allocate and manage yet another fd for every thread just for this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9014#note_117177
Alexandre Julliard (@julliard) commented about server/protocol.def:
timeout_t server_start; /* server start time */ unsigned int session_id; /* process session id */ obj_handle_t inproc_device;/* inproc device fd in flight with this handle */ + obj_handle_t alert_handle; /* alert fd is in flight with this handle */ data_size_t info_size; /* total size of startup info */
I'd suggest to add a separate request instead of trying to cram everything into the initial request. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9014#note_117178
participants (4)
-
Alexandre Julliard (＠julliard) -
Elizabeth Figura
-
Elizabeth Figura (＠zfigura)
-
Rémi Bernon