From 22725f2433fd321c71d4392461baab24ffca2372 Mon Sep 17 00:00:00 2001 From: Michael Penick Date: Wed, 11 Feb 2015 14:47:20 -0700 Subject: [PATCH] unix: fix for uv_async data race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's a data race in the consuming side of uv_async. The "pending" flag could be trampled by producing thread causing an async send event to be missed. PR-URL: https://github.com/libuv/libuv/pull/197 Reviewed-By: Ben Noordhuis Reviewed-By: Saúl Ibarra Corretgé --- src/unix/async.c | 45 ++++++++++----------------------------------- src/unix/internal.h | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 35 deletions(-) diff --git a/src/unix/async.c b/src/unix/async.c index 43f54ed4..8adf046a 100644 --- a/src/unix/async.c +++ b/src/unix/async.c @@ -24,6 +24,7 @@ #include "uv.h" #include "internal.h" +#include "atomic-ops.h" #include #include @@ -34,7 +35,6 @@ static void uv__async_event(uv_loop_t* loop, struct uv__async* w, unsigned int nevents); -static int uv__async_make_pending(int* pending); static int uv__async_eventfd(void); @@ -54,7 +54,11 @@ int uv_async_init(uv_loop_t* loop, uv_async_t* handle, uv_async_cb async_cb) { int uv_async_send(uv_async_t* handle) { - if (uv__async_make_pending(&handle->pending) == 0) + /* Do a cheap read first. */ + if (ACCESS_ONCE(int, handle->pending) != 0) + return 0; + + if (cmpxchgi(&handle->pending, 0, 1) == 0) uv__async_send(&handle->loop->async_watcher); return 0; @@ -75,44 +79,15 @@ static void uv__async_event(uv_loop_t* loop, ngx_queue_foreach(q, &loop->async_handles) { h = ngx_queue_data(q, uv_async_t, queue); - if (!h->pending) continue; - h->pending = 0; + + if (cmpxchgi(&h->pending, 1, 0) == 0) + continue; + h->async_cb(h, 0); } } -static int uv__async_make_pending(int* pending) { - /* Do a cheap read first. */ - if (ACCESS_ONCE(int, *pending) != 0) - return 1; - - /* Micro-optimization: use atomic memory operations to detect if we've been - * preempted by another thread and don't have to make an expensive syscall. - * This speeds up the heavily contended case by about 1-2% and has little - * if any impact on the non-contended case. - * - * Use XCHG instead of the CMPXCHG that __sync_val_compare_and_swap() emits - * on x86, it's about 4x faster. It probably makes zero difference in the - * grand scheme of things but I'm OCD enough not to let this one pass. - */ -#if defined(__i386__) || defined(__x86_64__) - { - unsigned int val = 1; - __asm__ __volatile__ ("xchgl %0, %1" - : "+r" (val) - : "m" (*pending)); - return val != 0; - } -#elif defined(__GNUC__) && (__GNUC__ > 4 || __GNUC__ == 4 && __GNUC_MINOR__ > 0) - return __sync_val_compare_and_swap(pending, 0, 1) != 0; -#else - ACCESS_ONCE(int, *pending) = 1; - return 0; -#endif -} - - static void uv__async_io(uv_loop_t* loop, uv__io_t* w, unsigned int events) { struct uv__async* wa; char buf[1024]; diff --git a/src/unix/internal.h b/src/unix/internal.h index 003e1a50..af360539 100644 --- a/src/unix/internal.h +++ b/src/unix/internal.h @@ -66,6 +66,21 @@ } \ while (0) +/* The __clang__ and __INTEL_COMPILER checks are superfluous because they + * define __GNUC__. They are here to convey to you, dear reader, that these + * macros are enabled when compiling with clang or icc. + */ +#if defined(__clang__) || \ + defined(__GNUC__) || \ + defined(__INTEL_COMPILER) || \ + defined(__SUNPRO_C) +# define UV_DESTRUCTOR(declaration) __attribute__((destructor)) declaration +# define UV_UNUSED(declaration) __attribute__((unused)) declaration +#else +# define UV_DESTRUCTOR(declaration) declaration +# define UV_UNUSED(declaration) declaration +#endif + #if defined(__linux__) # define UV__POLLIN UV__EPOLLIN # define UV__POLLOUT UV__EPOLLOUT