From 78a1cd167fd97ebd7bb2f6aaf80dc093d54e2234 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C3=BAl=20Ibarra=20Corretg=C3=A9?= Date: Fri, 5 Aug 2016 15:44:39 +0100 Subject: [PATCH] win,async: refactor internal implementation - Use atomic ops both to set and reset async_sent flag - Remove the MinGW atomic ops, since Windows intrinsics are supported - Remone thread-unsafe checks from uv_async_send According to MSDN there are no alignment requirements. We could use InterlockedExchange8, but that's only available on Windows >= 8. This change is intended to make uv_async_send more resilient. It has to be thread-safe, and that means that the handle could just have been closed when uv_async_send was called. This case was previously not handles (there is an inherent race condition). The new model is inspired by the one used on the Unix side, which uses a single fd (or overlapped in this case) to wakeup the loop and then process all pending async handles. This makes handling those edge cases a lot simpler: when the handle is closed it's removed from the handle queue, and then it's not processed at all. As a result of this change, async benchmarks work on Windows where they previously failed with assertions. PR-URL: https://github.com/libuv/libuv/pull/980 Reviewed-By: Ben Noordhuis --- Makefile.am | 1 - Makefile.mingw | 1 - include/uv-win.h | 10 +++--- src/win/async.c | 67 ++++++++++++++++++----------------------- src/win/atomicops-inl.h | 56 ---------------------------------- src/win/core.c | 4 +++ src/win/internal.h | 3 +- src/win/req-inl.h | 2 +- uv.gyp | 1 - 9 files changed, 42 insertions(+), 103 deletions(-) delete mode 100644 src/win/atomicops-inl.h diff --git a/Makefile.am b/Makefile.am index 11019a2c7..66d92ae86 100644 --- a/Makefile.am +++ b/Makefile.am @@ -47,7 +47,6 @@ AM_CPPFLAGS += -I$(top_srcdir)/src/win \ -DWIN32_LEAN_AND_MEAN \ -D_WIN32_WINNT=0x0600 libuv_la_SOURCES += src/win/async.c \ - src/win/atomicops-inl.h \ src/win/core.c \ src/win/dl.c \ src/win/error.c \ diff --git a/Makefile.mingw b/Makefile.mingw index ffa705f3b..f7349b441 100644 --- a/Makefile.mingw +++ b/Makefile.mingw @@ -32,7 +32,6 @@ INCLUDES = include/tree.h \ src/heap-inl.h \ src/queue.h \ src/uv-common.h \ - src/win/atomicops-inl.h \ src/win/handle-inl.h \ src/win/internal.h \ src/win/req-inl.h \ diff --git a/include/uv-win.h b/include/uv-win.h index 966321c17..74bb31a4b 100644 --- a/include/uv-win.h +++ b/include/uv-win.h @@ -310,7 +310,10 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s); /* Threadpool */ \ void* wq[2]; \ uv_mutex_t wq_mutex; \ - uv_async_t wq_async; + uv_async_t wq_async; \ + /* Async handle */ \ + struct uv_req_s async_req; \ + void* async_handles[2]; #define UV_REQ_TYPE_PRIVATE \ /* TODO: remove the req suffix */ \ @@ -498,10 +501,9 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s); uv_timer_cb timer_cb; #define UV_ASYNC_PRIVATE_FIELDS \ - struct uv_req_s async_req; \ + void* queue[2]; \ uv_async_cb async_cb; \ - /* char to avoid alignment issues */ \ - char volatile async_sent; + LONG volatile async_sent; #define UV_PREPARE_PRIVATE_FIELDS \ void* queue[2]; \ diff --git a/src/win/async.c b/src/win/async.c index ad240ab89..16a1e1093 100644 --- a/src/win/async.c +++ b/src/win/async.c @@ -23,32 +23,23 @@ #include "uv.h" #include "internal.h" -#include "atomicops-inl.h" #include "handle-inl.h" #include "req-inl.h" void uv_async_endgame(uv_loop_t* loop, uv_async_t* handle) { - if (handle->flags & UV__HANDLE_CLOSING && - !handle->async_sent) { - assert(!(handle->flags & UV_HANDLE_CLOSED)); - uv__handle_close(handle); - } + assert(handle->flags & UV__HANDLE_CLOSING); + assert(!(handle->flags & UV_HANDLE_CLOSED)); + uv__handle_close(handle); } int uv_async_init(uv_loop_t* loop, uv_async_t* handle, uv_async_cb async_cb) { - uv_req_t* req; - uv__handle_init(loop, (uv_handle_t*) handle, UV_ASYNC); handle->async_sent = 0; handle->async_cb = async_cb; - req = &handle->async_req; - uv_req_init(loop, req); - req->type = UV_WAKEUP; - req->data = handle; - + QUEUE_INSERT_TAIL(&loop->async_handles, &handle->queue); uv__handle_start(handle); return 0; @@ -56,44 +47,46 @@ int uv_async_init(uv_loop_t* loop, uv_async_t* handle, uv_async_cb async_cb) { void uv_async_close(uv_loop_t* loop, uv_async_t* handle) { - if (!((uv_async_t*)handle)->async_sent) { - uv_want_endgame(loop, (uv_handle_t*) handle); - } - + QUEUE_REMOVE(&handle->queue); + uv_want_endgame(loop, (uv_handle_t*) handle); uv__handle_closing(handle); } int uv_async_send(uv_async_t* handle) { - uv_loop_t* loop = handle->loop; + /* First do a cheap read. */ + if (handle->async_sent != 0) + return 0; - if (handle->type != UV_ASYNC) { - /* Can't set errno because that's not thread-safe. */ - return -1; - } - - /* The user should make sure never to call uv_async_send to a closing */ - /* or closed handle. */ - assert(!(handle->flags & UV__HANDLE_CLOSING)); - - if (!uv__atomic_exchange_set(&handle->async_sent)) { - POST_COMPLETION_FOR_REQ(loop, &handle->async_req); + if (InterlockedExchange(&handle->async_sent, 1) == 0) { + uv_loop_t* loop = handle->loop; + POST_COMPLETION_FOR_REQ(loop, &loop->async_req); } return 0; } -void uv_process_async_wakeup_req(uv_loop_t* loop, uv_async_t* handle, - uv_req_t* req) { - assert(handle->type == UV_ASYNC); +void uv_process_async_wakeup_req(uv_loop_t* loop, + uv_req_t* req) { assert(req->type == UV_WAKEUP); - handle->async_sent = 0; + QUEUE queue; + QUEUE* q; + uv_async_t* h; - if (handle->flags & UV__HANDLE_CLOSING) { - uv_want_endgame(loop, (uv_handle_t*)handle); - } else if (handle->async_cb != NULL) { - handle->async_cb(handle); + QUEUE_MOVE(&loop->async_handles, &queue); + while (!QUEUE_EMPTY(&queue)) { + q = QUEUE_HEAD(&queue); + h = QUEUE_DATA(q, uv_async_t, queue); + + QUEUE_REMOVE(q); + QUEUE_INSERT_TAIL(&loop->async_handles, q); + + if (InterlockedExchange(&h->async_sent, 0) == 0) + continue; + + if (h->async_cb != NULL) + h->async_cb(h); } } diff --git a/src/win/atomicops-inl.h b/src/win/atomicops-inl.h deleted file mode 100644 index 61e006026..000000000 --- a/src/win/atomicops-inl.h +++ /dev/null @@ -1,56 +0,0 @@ -/* Copyright Joyent, Inc. and other Node contributors. All rights reserved. - * - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to - * deal in the Software without restriction, including without limitation the - * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or - * sell copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in - * all copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS - * IN THE SOFTWARE. - */ - -#ifndef UV_WIN_ATOMICOPS_INL_H_ -#define UV_WIN_ATOMICOPS_INL_H_ - -#include "uv.h" - - -/* Atomic set operation on char */ -#ifdef _MSC_VER /* MSVC */ - -/* _InterlockedOr8 is supported by MSVC on x32 and x64. It is slightly less */ -/* efficient than InterlockedExchange, but InterlockedExchange8 does not */ -/* exist, and interlocked operations on larger targets might require the */ -/* target to be aligned. */ -#pragma intrinsic(_InterlockedOr8) - -static char __declspec(inline) uv__atomic_exchange_set(char volatile* target) { - return _InterlockedOr8(target, 1); -} - -#else /* GCC */ - -/* Mingw-32 version, hopefully this works for 64-bit gcc as well. */ -static inline char uv__atomic_exchange_set(char volatile* target) { - const char one = 1; - char old_value; - __asm__ __volatile__ ("lock xchgb %0, %1\n\t" - : "=r"(old_value), "=m"(*target) - : "0"(one), "m"(*target) - : "memory"); - return old_value; -} - -#endif - -#endif /* UV_WIN_ATOMICOPS_INL_H_ */ diff --git a/src/win/core.c b/src/win/core.c index a8c18e837..b7bd049ed 100644 --- a/src/win/core.c +++ b/src/win/core.c @@ -153,6 +153,10 @@ int uv_loop_init(uv_loop_t* loop) { QUEUE_INIT(&loop->prepare_handles); QUEUE_INIT(&loop->idle_handles); + QUEUE_INIT(&loop->async_handles); + uv_req_init(loop, &loop->async_req); + loop->async_req.type = UV_WAKEUP; + memset(&loop->poll_peer_sockets, 0, sizeof loop->poll_peer_sockets); loop->timer_counter = 0; diff --git a/src/win/internal.h b/src/win/internal.h index 6bfe53613..6c6fd3a24 100644 --- a/src/win/internal.h +++ b/src/win/internal.h @@ -267,8 +267,7 @@ void uv__once_init(); void uv_async_close(uv_loop_t* loop, uv_async_t* handle); void uv_async_endgame(uv_loop_t* loop, uv_async_t* handle); -void uv_process_async_wakeup_req(uv_loop_t* loop, uv_async_t* handle, - uv_req_t* req); +void uv_process_async_wakeup_req(uv_loop_t* loop, uv_req_t* req); /* diff --git a/src/win/req-inl.h b/src/win/req-inl.h index b5e502eef..aca628083 100644 --- a/src/win/req-inl.h +++ b/src/win/req-inl.h @@ -194,7 +194,7 @@ INLINE static int uv_process_reqs(uv_loop_t* loop) { break; case UV_WAKEUP: - uv_process_async_wakeup_req(loop, (uv_async_t*) req->data, req); + uv_process_async_wakeup_req(loop, req); break; case UV_SIGNAL_REQ: diff --git a/uv.gyp b/uv.gyp index 280d96875..12a7ebde4 100644 --- a/uv.gyp +++ b/uv.gyp @@ -74,7 +74,6 @@ 'sources': [ 'include/uv-win.h', 'src/win/async.c', - 'src/win/atomicops-inl.h', 'src/win/core.c', 'src/win/dl.c', 'src/win/error.c',