From c70dd705bc2adc488ddffcdc12f0c610d116e77b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 16 Jun 2020 11:42:46 +0200 Subject: [PATCH] unix: use relaxed loads/stores for feature checks Make ThreadSanitizer stop complaining about the static variables that libuv uses to record the presence (or lack) of system calls and other kernel features. Fixes: https://github.com/libuv/libuv/issues/2884 PR-URL: https://github.com/libuv/libuv/pull/2886 Reviewed-By: Anna Henningsen Reviewed-By: Jameson Nash --- src/unix/core.c | 30 +++++++++++++++++++----------- src/unix/fs.c | 12 ++++++------ src/unix/kqueue.c | 9 +++++---- src/unix/linux-core.c | 23 +++++++++++++++++++---- src/unix/os390-syscalls.c | 1 - src/unix/pthread-fixes.c | 6 ++++-- src/unix/tcp.c | 5 ++++- src/uv-common.c | 4 ++-- src/uv-common.h | 8 ++++++++ 9 files changed, 67 insertions(+), 31 deletions(-) diff --git a/src/unix/core.c b/src/unix/core.c index 3ba4e500..5b0b64dd 100644 --- a/src/unix/core.c +++ b/src/unix/core.c @@ -216,15 +216,23 @@ int uv__getiovmax(void) { #if defined(IOV_MAX) return IOV_MAX; #elif defined(_SC_IOV_MAX) - static int iovmax = -1; - if (iovmax == -1) { - iovmax = sysconf(_SC_IOV_MAX); - /* On some embedded devices (arm-linux-uclibc based ip camera), - * sysconf(_SC_IOV_MAX) can not get the correct value. The return - * value is -1 and the errno is EINPROGRESS. Degrade the value to 1. - */ - if (iovmax == -1) iovmax = 1; - } + static int iovmax_cached = -1; + int iovmax; + + iovmax = uv__load_relaxed(&iovmax_cached); + if (iovmax != -1) + return iovmax; + + /* On some embedded devices (arm-linux-uclibc based ip camera), + * sysconf(_SC_IOV_MAX) can not get the correct value. The return + * value is -1 and the errno is EINPROGRESS. Degrade the value to 1. + */ + iovmax = sysconf(_SC_IOV_MAX); + if (iovmax == -1) + iovmax = 1; + + uv__store_relaxed(&iovmax_cached, iovmax); + return iovmax; #else return 1024; @@ -658,7 +666,7 @@ ssize_t uv__recvmsg(int fd, struct msghdr* msg, int flags) { int* end; #if defined(__linux__) static int no_msg_cmsg_cloexec; - if (no_msg_cmsg_cloexec == 0) { + if (0 == uv__load_relaxed(&no_msg_cmsg_cloexec)) { rc = recvmsg(fd, msg, flags | 0x40000000); /* MSG_CMSG_CLOEXEC */ if (rc != -1) return rc; @@ -667,7 +675,7 @@ ssize_t uv__recvmsg(int fd, struct msghdr* msg, int flags) { rc = recvmsg(fd, msg, flags); if (rc == -1) return UV__ERR(errno); - no_msg_cmsg_cloexec = 1; + uv__store_relaxed(&no_msg_cmsg_cloexec, 1); } else { rc = recvmsg(fd, msg, flags); } diff --git a/src/unix/fs.c b/src/unix/fs.c index 61f39303..dd08ea54 100644 --- a/src/unix/fs.c +++ b/src/unix/fs.c @@ -312,7 +312,7 @@ static int uv__fs_mkstemp(uv_fs_t* req) { uv_once(&once, uv__mkostemp_initonce); #ifdef O_CLOEXEC - if (no_cloexec_support == 0 && uv__mkostemp != NULL) { + if (uv__load_relaxed(&no_cloexec_support) == 0 && uv__mkostemp != NULL) { r = uv__mkostemp(path, O_CLOEXEC); if (r >= 0) @@ -325,7 +325,7 @@ static int uv__fs_mkstemp(uv_fs_t* req) { /* We set the static variable so that next calls don't even try to use mkostemp. */ - no_cloexec_support = 1; + uv__store_relaxed(&no_cloexec_support, 1); } #endif /* O_CLOEXEC */ @@ -456,7 +456,7 @@ static ssize_t uv__fs_read(uv_fs_t* req) { result = preadv(req->file, (struct iovec*) req->bufs, req->nbufs, req->off); #else # if defined(__linux__) - if (no_preadv) retry: + if (uv__load_relaxed(&no_preadv)) retry: # endif { result = uv__fs_preadv(req->file, req->bufs, req->nbufs, req->off); @@ -468,7 +468,7 @@ static ssize_t uv__fs_read(uv_fs_t* req) { req->nbufs, req->off); if (result == -1 && errno == ENOSYS) { - no_preadv = 1; + uv__store_relaxed(&no_preadv, 1); goto retry; } } @@ -1351,7 +1351,7 @@ static int uv__fs_statx(int fd, int mode; int rc; - if (no_statx) + if (uv__load_relaxed(&no_statx)) return UV_ENOSYS; dirfd = AT_FDCWD; @@ -1384,7 +1384,7 @@ static int uv__fs_statx(int fd, * implemented, rc might return 1 with 0 set as the error code in which * case we return ENOSYS. */ - no_statx = 1; + uv__store_relaxed(&no_statx, 1); return UV_ENOSYS; } diff --git a/src/unix/kqueue.c b/src/unix/kqueue.c index ad09f403..27a0d3df 100644 --- a/src/unix/kqueue.c +++ b/src/unix/kqueue.c @@ -82,7 +82,7 @@ int uv__io_fork(uv_loop_t* loop) { process. So we sidestep the issue by pretending like we never started it in the first place. */ - uv__has_forked_with_cfrunloop = 1; + uv__store_relaxed(&uv__has_forked_with_cfrunloop, 1); uv__free(loop->cf_state); loop->cf_state = NULL; } @@ -487,7 +487,7 @@ int uv_fs_event_start(uv_fs_event_t* handle, if (!(statbuf.st_mode & S_IFDIR)) goto fallback; - if (!uv__has_forked_with_cfrunloop) { + if (0 == uv__load_relaxed(&uv__has_forked_with_cfrunloop)) { int r; /* The fallback fd is no longer needed */ uv__close_nocheckstdio(fd); @@ -522,8 +522,9 @@ int uv_fs_event_stop(uv_fs_event_t* handle) { uv__handle_stop(handle); #if defined(__APPLE__) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070 - if (!uv__has_forked_with_cfrunloop && handle->cf_cb != NULL) - r = uv__fsevents_close(handle); + if (0 == uv__load_relaxed(&uv__has_forked_with_cfrunloop)) + if (handle->cf_cb != NULL) + r = uv__fsevents_close(handle); #endif if (handle->event_watcher.fd != -1) { diff --git a/src/unix/linux-core.c b/src/unix/linux-core.c index 8863333a..80ca75ee 100644 --- a/src/unix/linux-core.c +++ b/src/unix/linux-core.c @@ -198,8 +198,10 @@ void uv__io_poll(uv_loop_t* loop, int timeout) { * that being the largest value I have seen in the wild (and only once.) */ static const int max_safe_timeout = 1789569; - static int no_epoll_pwait; - static int no_epoll_wait; + static int no_epoll_pwait_cached; + static int no_epoll_wait_cached; + int no_epoll_pwait; + int no_epoll_wait; struct epoll_event events[1024]; struct epoll_event* pe; struct epoll_event e; @@ -271,6 +273,15 @@ void uv__io_poll(uv_loop_t* loop, int timeout) { count = 48; /* Benchmarks suggest this gives the best throughput. */ real_timeout = timeout; + /* You could argue there is a dependency between these two but + * ultimately we don't care about their ordering with respect + * to one another. Worst case, we make a few system calls that + * could have been avoided because another thread already knows + * they fail with ENOSYS. Hardly the end of the world. + */ + no_epoll_pwait = uv__load_relaxed(&no_epoll_pwait_cached); + no_epoll_wait = uv__load_relaxed(&no_epoll_wait_cached); + for (;;) { /* See the comment for max_safe_timeout for an explanation of why * this is necessary. Executive summary: kernel bug workaround. @@ -288,15 +299,19 @@ void uv__io_poll(uv_loop_t* loop, int timeout) { ARRAY_SIZE(events), timeout, &sigset); - if (nfds == -1 && errno == ENOSYS) + if (nfds == -1 && errno == ENOSYS) { + uv__store_relaxed(&no_epoll_pwait_cached, 1); no_epoll_pwait = 1; + } } else { nfds = epoll_wait(loop->backend_fd, events, ARRAY_SIZE(events), timeout); - if (nfds == -1 && errno == ENOSYS) + if (nfds == -1 && errno == ENOSYS) { + uv__store_relaxed(&no_epoll_wait_cached, 1); no_epoll_wait = 1; + } } if (sigmask != 0 && no_epoll_pwait != 0) diff --git a/src/unix/os390-syscalls.c b/src/unix/os390-syscalls.c index 424cc486..491e950c 100644 --- a/src/unix/os390-syscalls.c +++ b/src/unix/os390-syscalls.c @@ -33,7 +33,6 @@ #pragma linkage(BPX4CTW, OS) #pragma linkage(BPX1CTW, OS) -static int number_of_epolls; static QUEUE global_epoll_queue; static uv_mutex_t global_epoll_lock; static uv_once_t once = UV_ONCE_INIT; diff --git a/src/unix/pthread-fixes.c b/src/unix/pthread-fixes.c index fb179958..022d79c4 100644 --- a/src/unix/pthread-fixes.c +++ b/src/unix/pthread-fixes.c @@ -30,6 +30,8 @@ */ /* Android versions < 4.1 have a broken pthread_sigmask. */ +#include "uv-common.h" + #include #include #include @@ -38,13 +40,13 @@ int uv__pthread_sigmask(int how, const sigset_t* set, sigset_t* oset) { static int workaround; int err; - if (workaround) { + if (uv__load_relaxed(&workaround)) { return sigprocmask(how, set, oset); } else { err = pthread_sigmask(how, set, oset); if (err) { if (err == EINVAL && sigprocmask(how, set, oset) == 0) { - workaround = 1; + uv__store_relaxed(&workaround, 1); return 0; } else { return -1; diff --git a/src/unix/tcp.c b/src/unix/tcp.c index d47e9433..18acd20d 100644 --- a/src/unix/tcp.c +++ b/src/unix/tcp.c @@ -326,16 +326,19 @@ int uv_tcp_close_reset(uv_tcp_t* handle, uv_close_cb close_cb) { int uv_tcp_listen(uv_tcp_t* tcp, int backlog, uv_connection_cb cb) { - static int single_accept = -1; + static int single_accept_cached = -1; unsigned long flags; + int single_accept; int err; if (tcp->delayed_error) return tcp->delayed_error; + single_accept = uv__load_relaxed(&single_accept_cached); if (single_accept == -1) { const char* val = getenv("UV_TCP_SINGLE_ACCEPT"); single_accept = (val != NULL && atoi(val) != 0); /* Off by default. */ + uv__store_relaxed(&single_accept_cached, single_accept); } if (single_accept) diff --git a/src/uv-common.c b/src/uv-common.c index a25d6aae..0cfb921e 100644 --- a/src/uv-common.c +++ b/src/uv-common.c @@ -859,11 +859,11 @@ __attribute__((destructor)) void uv_library_shutdown(void) { static int was_shutdown; - if (was_shutdown) + if (uv__load_relaxed(&was_shutdown)) return; uv__process_title_cleanup(); uv__signal_cleanup(); uv__threadpool_cleanup(); - was_shutdown = 1; + uv__store_relaxed(&was_shutdown, 1); } diff --git a/src/uv-common.h b/src/uv-common.h index 0b0f5f86..c7894148 100644 --- a/src/uv-common.h +++ b/src/uv-common.h @@ -60,6 +60,14 @@ extern int snprintf(char*, size_t, const char*, ...); #define STATIC_ASSERT(expr) \ void uv__static_assert(int static_assert_failed[1 - 2 * !(expr)]) +#ifdef __GNUC__ +#define uv__load_relaxed(p) __atomic_load_n(p, __ATOMIC_RELAXED) +#define uv__store_relaxed(p, v) __atomic_store_n(p, v, __ATOMIC_RELAXED) +#else +#define uv__load_relaxed(p) (*p) +#define uv__store_relaxed(p, v) do *p = v; while (0) +#endif + /* Handle flags. Some flags are specific to Windows or UNIX. */ enum { /* Used by all handles. */