Skip to content

Commit

Permalink
ntdll, server: Revert to old implementation of hung queue detection.
Browse files Browse the repository at this point in the history
By manually notifying the server every time we enter and exit a message wait.

The hung queue logic keeps breaking. In the case of bug ValveSoftware#9 it was breaking
because we were waiting for more than 5 seconds on our queue and then someone
sent us a message with SMTO_ABORTIFHUNG. Just stop fighting against the
server and try to coöperate with it instead. It takes two extra server calls,
but ideally the GUI thread isn't going to be in the same sort of performance-
critical code that this patchset was written for.
  • Loading branch information
zfigura authored and aeikum committed Jun 24, 2019
1 parent 739ee09 commit b94a304
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 23 deletions.
53 changes: 40 additions & 13 deletions dlls/ntdll/esync.c
Original file line number Diff line number Diff line change
Expand Up @@ -812,8 +812,8 @@ static void update_grabbed_object( struct esync *obj )

/* A value of STATUS_NOT_IMPLEMENTED returned from this function means that we
* need to delegate to server_select(). */
NTSTATUS esync_wait_objects( DWORD count, const HANDLE *handles, BOOLEAN wait_any,
BOOLEAN alertable, const LARGE_INTEGER *timeout )
static NTSTATUS __esync_wait_objects( DWORD count, const HANDLE *handles,
BOOLEAN wait_any, BOOLEAN alertable, const LARGE_INTEGER *timeout )
{
static const LARGE_INTEGER zero = {0};

Expand Down Expand Up @@ -876,22 +876,11 @@ NTSTATUS esync_wait_objects( DWORD count, const HANDLE *handles, BOOLEAN wait_an

if (objs[count - 1] && objs[count - 1]->type == ESYNC_QUEUE)
{
select_op_t select_op;

/* Last object in the list is a queue, which means someone is using
* MsgWaitForMultipleObjects(). We have to wait not only for the server
* fd (signaled on send_message, etc.) but also the USER driver's fd
* (signaled on e.g. X11 events.) */
msgwait = TRUE;

/* We need to let the server know we are doing a message wait, for two
* reasons. First one is WaitForInputIdle(). Second one is checking for
* hung queues. Do it like this. */
select_op.wait.op = SELECT_WAIT;
select_op.wait.handles[0] = wine_server_obj_handle( handles[count - 1] );
ret = server_select( &select_op, offsetof( select_op_t, wait.handles[1] ), 0, &zero );
if (ret != STATUS_WAIT_0 && ret != STATUS_TIMEOUT)
ERR("Unexpected ret %#x\n", ret);
}

if (has_esync && has_server)
Expand Down Expand Up @@ -1263,6 +1252,44 @@ NTSTATUS esync_wait_objects( DWORD count, const HANDLE *handles, BOOLEAN wait_an
return ret;
}

/* We need to let the server know when we are doing a message wait, and when we
* are done with one, so that all of the code surrounding hung queues works.
* We also need this for WaitForInputIdle(). */
static void server_set_msgwait( int in_msgwait )
{
SERVER_START_REQ( esync_msgwait )
{
req->in_msgwait = in_msgwait;
wine_server_call( req );
}
SERVER_END_REQ;
}

/* This is a very thin wrapper around the proper implementation above. The
* purpose is to make sure the server knows when we are doing a message wait.
* This is separated into a wrapper function since there are at least a dozen
* exit paths from esync_wait_objects(). */
NTSTATUS esync_wait_objects( DWORD count, const HANDLE *handles, BOOLEAN wait_any,
BOOLEAN alertable, const LARGE_INTEGER *timeout )
{
BOOL msgwait = FALSE;
struct esync *obj;
NTSTATUS ret;

if (!get_object( handles[count - 1], &obj ) && obj->type == ESYNC_QUEUE)
{
msgwait = TRUE;
server_set_msgwait( 1 );
}

ret = __esync_wait_objects( count, handles, wait_any, alertable, timeout );

if (msgwait)
server_set_msgwait( 0 );

return ret;
}

NTSTATUS esync_signal_and_wait( HANDLE signal, HANDLE wait, BOOLEAN alertable,
const LARGE_INTEGER *timeout )
{
Expand Down
16 changes: 15 additions & 1 deletion include/wine/server_protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -5855,6 +5855,17 @@ struct get_esync_apc_fd_reply
struct reply_header __header;
};


struct esync_msgwait_request
{
struct request_header __header;
int in_msgwait;
};
struct esync_msgwait_reply
{
struct reply_header __header;
};

enum esync_type
{
ESYNC_SEMAPHORE = 1,
Expand Down Expand Up @@ -6171,6 +6182,7 @@ enum request
REQ_open_esync,
REQ_get_esync_fd,
REQ_get_esync_apc_fd,
REQ_esync_msgwait,
REQ_NB_REQUESTS
};

Expand Down Expand Up @@ -6480,6 +6492,7 @@ union generic_request
struct open_esync_request open_esync_request;
struct get_esync_fd_request get_esync_fd_request;
struct get_esync_apc_fd_request get_esync_apc_fd_request;
struct esync_msgwait_request esync_msgwait_request;
};
union generic_reply
{
Expand Down Expand Up @@ -6787,8 +6800,9 @@ union generic_reply
struct open_esync_reply open_esync_reply;
struct get_esync_fd_reply get_esync_fd_reply;
struct get_esync_apc_fd_reply get_esync_apc_fd_reply;
struct esync_msgwait_reply esync_msgwait_reply;
};

#define SERVER_PROTOCOL_VERSION 597
#define SERVER_PROTOCOL_VERSION 598

#endif /* __WINE_WINE_SERVER_PROTOCOL_H */
6 changes: 5 additions & 1 deletion server/protocol.def
Original file line number Diff line number Diff line change
Expand Up @@ -3983,7 +3983,11 @@ struct handle_info

/* Retrieve the fd to wait on for user APCs. */
@REQ(get_esync_apc_fd)
@REPLY
@END

/* Notify the server that we are doing a message wait (or done with one). */
@REQ(esync_msgwait)
int in_msgwait; /* are we in a message wait? */
@END

enum esync_type
Expand Down
35 changes: 28 additions & 7 deletions server/queue.c
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ struct msg_queue
struct hook_table *hooks; /* hook table */
timeout_t last_get_msg; /* time of last get message call */
int esync_fd; /* esync file descriptor (signalled on message) */
int esync_in_msgwait; /* our thread is currently waiting on us */
};

struct hotkey
Expand Down Expand Up @@ -912,7 +913,21 @@ static void cleanup_results( struct msg_queue *queue )
/* check if the thread owning the queue is hung (not checking for messages) */
static int is_queue_hung( struct msg_queue *queue )
{
return (current_time - queue->last_get_msg > 5 * TICKS_PER_SEC);
struct wait_queue_entry *entry;

if (current_time - queue->last_get_msg <= 5 * TICKS_PER_SEC)
return 0; /* less than 5 seconds since last get message -> not hung */

LIST_FOR_EACH_ENTRY( entry, &queue->obj.wait_queue, struct wait_queue_entry, entry )
{
if (get_wait_queue_thread(entry)->queue == queue)
return 0; /* thread is waiting on queue -> not hung */
}

if (do_esync() && queue->esync_in_msgwait)
return 0; /* thread is waiting on queue in absentia -> not hung */

return 1;
}

static int msg_queue_add_queue( struct object *obj, struct wait_queue_entry *entry )
Expand All @@ -928,12 +943,6 @@ static int msg_queue_add_queue( struct object *obj, struct wait_queue_entry *ent
}
if (process->idle_event && !(queue->wake_mask & QS_SMRESULT)) set_event( process->idle_event );

/* On Windows, we are considered hung iff we have not somehow processed
* messages OR done a MsgWait call in the last 5 seconds. Note that in the
* latter case repeatedly waiting for 0 seconds is not hung, but waiting
* forever is hung, so this is correct. */
queue->last_get_msg = current_time;

if (queue->fd && list_empty( &obj->wait_queue )) /* first on the queue */
set_fd_events( queue->fd, POLLIN );
add_queue( obj, entry );
Expand Down Expand Up @@ -1579,6 +1588,7 @@ static int send_hook_ll_message( struct desktop *desktop, struct message *hardwa

if (!(hook_thread = get_first_global_hook( id ))) return 0;
if (!(queue = hook_thread->queue)) return 0;
if (is_queue_hung( queue )) return 0;

if (!(msg = mem_alloc( sizeof(*msg) ))) return 0;

Expand Down Expand Up @@ -3143,3 +3153,14 @@ DECL_HANDLER(update_rawinput_devices)
e = find_rawinput_device( 1, 6 );
current->process->rawinput_kbd = e ? &e->device : NULL;
}

DECL_HANDLER(esync_msgwait)
{
struct msg_queue *queue = get_current_queue();

if (!queue) return;
queue->esync_in_msgwait = req->in_msgwait;

if (current->process->idle_event && !(queue->wake_mask & QS_SMRESULT))
set_event( current->process->idle_event );
}
5 changes: 4 additions & 1 deletion server/request.h
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ DECL_HANDLER(create_esync);
DECL_HANDLER(open_esync);
DECL_HANDLER(get_esync_fd);
DECL_HANDLER(get_esync_apc_fd);
DECL_HANDLER(esync_msgwait);

#ifdef WANT_REQUEST_HANDLERS

Expand Down Expand Up @@ -722,6 +723,7 @@ static const req_handler req_handlers[REQ_NB_REQUESTS] =
(req_handler)req_open_esync,
(req_handler)req_get_esync_fd,
(req_handler)req_get_esync_apc_fd,
(req_handler)req_esync_msgwait,
};

C_ASSERT( sizeof(affinity_t) == 8 );
Expand Down Expand Up @@ -2475,7 +2477,8 @@ C_ASSERT( FIELD_OFFSET(struct get_esync_fd_reply, type) == 8 );
C_ASSERT( FIELD_OFFSET(struct get_esync_fd_reply, shm_idx) == 12 );
C_ASSERT( sizeof(struct get_esync_fd_reply) == 16 );
C_ASSERT( sizeof(struct get_esync_apc_fd_request) == 16 );
C_ASSERT( sizeof(struct get_esync_apc_fd_reply) == 8 );
C_ASSERT( FIELD_OFFSET(struct esync_msgwait_request, in_msgwait) == 12 );
C_ASSERT( sizeof(struct esync_msgwait_request) == 16 );

#endif /* WANT_REQUEST_HANDLERS */

Expand Down
8 changes: 8 additions & 0 deletions server/trace.c
Original file line number Diff line number Diff line change
Expand Up @@ -4647,6 +4647,11 @@ static void dump_get_esync_apc_fd_request( const struct get_esync_apc_fd_request
{
}

static void dump_esync_msgwait_request( const struct esync_msgwait_request *req )
{
fprintf( stderr, " in_msgwait=%d", req->in_msgwait );
}

static const dump_func req_dumpers[REQ_NB_REQUESTS] = {
(dump_func)dump_new_process_request,
(dump_func)dump_exec_process_request,
Expand Down Expand Up @@ -4950,6 +4955,7 @@ static const dump_func req_dumpers[REQ_NB_REQUESTS] = {
(dump_func)dump_open_esync_request,
(dump_func)dump_get_esync_fd_request,
(dump_func)dump_get_esync_apc_fd_request,
(dump_func)dump_esync_msgwait_request,
};

static const dump_func reply_dumpers[REQ_NB_REQUESTS] = {
Expand Down Expand Up @@ -5255,6 +5261,7 @@ static const dump_func reply_dumpers[REQ_NB_REQUESTS] = {
(dump_func)dump_open_esync_reply,
(dump_func)dump_get_esync_fd_reply,
NULL,
NULL,
};

static const char * const req_names[REQ_NB_REQUESTS] = {
Expand Down Expand Up @@ -5560,6 +5567,7 @@ static const char * const req_names[REQ_NB_REQUESTS] = {
"open_esync",
"get_esync_fd",
"get_esync_apc_fd",
"esync_msgwait",
};

static const struct
Expand Down

0 comments on commit b94a304

Please sign in to comment.