From 19b334286b26872f74b33f7076a05182e26d697a Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 17 Oct 2017 09:25:34 -0700 Subject: Turn benchmark failures into test failures --- build.yaml | 28 ------------------ tools/run_tests/generated/tests.json | 56 +++++++++--------------------------- 2 files changed, 14 insertions(+), 70 deletions(-) diff --git a/build.yaml b/build.yaml index fd4b4c3c77..465f17318d 100644 --- a/build.yaml +++ b/build.yaml @@ -3553,8 +3553,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3576,8 +3574,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3599,8 +3595,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3622,8 +3616,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3644,8 +3636,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3666,8 +3656,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3688,8 +3676,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=4 benchmark: true defaults: benchmark platforms: @@ -3710,8 +3696,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3735,8 +3719,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark excluded_poll_engines: @@ -3763,8 +3745,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark excluded_poll_engines: @@ -3790,8 +3770,6 @@ targets: - gpr_test_util - gpr - grpc++_test_config - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark excluded_poll_engines: @@ -3818,8 +3796,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark excluded_poll_engines: @@ -3844,8 +3820,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: @@ -3867,8 +3841,6 @@ targets: - grpc_unsecure - gpr_test_util - gpr - args: - - --benchmark_min_time=0 benchmark: true defaults: benchmark platforms: diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index d62f063b80..d690433d5e 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -3084,9 +3084,7 @@ "uses_polling": false }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3108,9 +3106,7 @@ "uses_polling": false }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3132,9 +3128,7 @@ "uses_polling": false }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3156,9 +3150,7 @@ "uses_polling": false }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3180,9 +3172,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3204,9 +3194,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3228,9 +3216,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=4" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3252,9 +3238,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3276,9 +3260,7 @@ "uses_polling": false }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3305,9 +3287,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3334,9 +3314,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3363,9 +3341,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3392,9 +3368,7 @@ "uses_polling": true }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", @@ -3416,9 +3390,7 @@ "uses_polling": false }, { - "args": [ - "--benchmark_min_time=0" - ], + "args": [], "benchmark": true, "ci_platforms": [ "linux", -- cgit v1.2.3 From 04f404922dc2a98472d4299b973e7a81e7f19886 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Sat, 21 Oct 2017 16:06:18 -0700 Subject: Class-ify lockfree event --- src/core/lib/iomgr/lockfree_event.cc | 111 ++++++++++++++++------------------- src/core/lib/iomgr/lockfree_event.h | 47 ++++++++------- 2 files changed, 75 insertions(+), 83 deletions(-) diff --git a/src/core/lib/iomgr/lockfree_event.cc b/src/core/lib/iomgr/lockfree_event.cc index f967b22ba9..549747b34b 100644 --- a/src/core/lib/iomgr/lockfree_event.cc +++ b/src/core/lib/iomgr/lockfree_event.cc @@ -26,92 +26,79 @@ extern grpc_tracer_flag grpc_polling_trace; /* 'state' holds the to call when the fd is readable or writable respectively. It can contain one of the following values: - CLOSURE_READY : The fd has an I/O event of interest but there is no + kClosureReady : The fd has an I/O event of interest but there is no closure yet to execute - CLOSURE_NOT_READY : The fd has no I/O event of interest + kClosureNotReady : The fd has no I/O event of interest closure ptr : The closure to be executed when the fd has an I/O event of interest - shutdown_error | FD_SHUTDOWN_BIT : - 'shutdown_error' field ORed with FD_SHUTDOWN_BIT. + shutdown_error | kShutdownBit : + 'shutdown_error' field ORed with kShutdownBit. This indicates that the fd is shutdown. Since all memory allocations are word-aligned, the lower two bits of the shutdown_error pointer are always 0. So - it is safe to OR these with FD_SHUTDOWN_BIT + it is safe to OR these with kShutdownBit Valid state transitions: - <-----3------ CLOSURE_NOT_READY ----1----> CLOSURE_READY + <-----3------ kClosureNotReady -----1-------> kClosureReady | | ^ | ^ | | | | | | | | | | +--------------4----------+ 6 +---------2---------------+ | | | | | v | - +-----5-------> [shutdown_error | FD_SHUTDOWN_BIT] <----7---------+ + +-----5-------> [shutdown_error | kShutdownBit] <-------7---------+ - For 1, 4 : See grpc_lfev_set_ready() function - For 2, 3 : See grpc_lfev_notify_on() function - For 5,6,7: See grpc_lfev_set_shutdown() function */ + For 1, 4 : See SetReady() function + For 2, 3 : See NotifyOn() function + For 5,6,7: See SetShutdown() function */ -#define CLOSURE_NOT_READY ((gpr_atm)0) -#define CLOSURE_READY ((gpr_atm)2) +namespace grpc_core { -#define FD_SHUTDOWN_BIT ((gpr_atm)1) - -void grpc_lfev_init(gpr_atm *state) { - gpr_atm_no_barrier_store(state, CLOSURE_NOT_READY); -} - -void grpc_lfev_destroy(gpr_atm *state) { - gpr_atm curr = gpr_atm_no_barrier_load(state); - if (curr & FD_SHUTDOWN_BIT) { - GRPC_ERROR_UNREF((grpc_error *)(curr & ~FD_SHUTDOWN_BIT)); +LockfreeEvent::~LockfreeEvent() { + gpr_atm curr = gpr_atm_no_barrier_load(&state_); + if (curr & kShutdownBit) { + GRPC_ERROR_UNREF((grpc_error *)(curr & ~kShutdownBit)); } else { - GPR_ASSERT(curr == CLOSURE_NOT_READY || curr == CLOSURE_READY); + GPR_ASSERT(curr == kClosureNotReady || curr == kClosureReady); } } -bool grpc_lfev_is_shutdown(gpr_atm *state) { - gpr_atm curr = gpr_atm_no_barrier_load(state); - return (curr & FD_SHUTDOWN_BIT) != 0; -} - -void grpc_lfev_notify_on(grpc_exec_ctx *exec_ctx, gpr_atm *state, - grpc_closure *closure, const char *variable) { +void LockfreeEvent::NotifyOn(grpc_exec_ctx *exec_ctx, grpc_closure *closure) { while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(state); + gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_notify_on[%s]: %p curr=%p closure=%p", variable, - state, (void *)curr, closure); + gpr_log(GPR_ERROR, "lfev_notify_on: %p curr=%p closure=%p", this, + (void *)curr, closure); } switch (curr) { - case CLOSURE_NOT_READY: { - /* CLOSURE_NOT_READY -> . + case kClosureNotReady: { + /* kClosureNotReady -> . We're guaranteed by API that there's an acquire barrier before here, so there's no need to double-dip and this can be a release-only. The release itself pairs with the acquire half of a set_ready full barrier. */ - if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { + if (gpr_atm_rel_cas(&state_, kClosureNotReady, (gpr_atm)closure)) { return; /* Successful. Return */ } break; /* retry */ } - case CLOSURE_READY: { - /* Change the state to CLOSURE_NOT_READY. Schedule the closure if + case kClosureReady: { + /* Change the state to kClosureNotReady. Schedule the closure if successful. If not, the state most likely transitioned to shutdown. We should retry. This can be a no-barrier cas since the state is being transitioned to - CLOSURE_NOT_READY; set_ready and set_shutdown do not schedule any + kClosureNotReady; set_ready and set_shutdown do not schedule any closure when transitioning out of CLOSURE_NO_READY state (i.e there is no other code that needs to 'happen-after' this) */ - if (gpr_atm_no_barrier_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { + if (gpr_atm_no_barrier_cas(&state_, kClosureReady, kClosureNotReady)) { GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_NONE); return; /* Successful. Return */ } @@ -123,8 +110,8 @@ void grpc_lfev_notify_on(grpc_exec_ctx *exec_ctx, gpr_atm *state, /* 'curr' is either a closure or the fd is shutdown(in which case 'curr' contains a pointer to the shutdown-error). If the fd is shutdown, schedule the closure with the shutdown error */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { - grpc_error *shutdown_err = (grpc_error *)(curr & ~FD_SHUTDOWN_BIT); + if ((curr & kShutdownBit) > 0) { + grpc_error *shutdown_err = (grpc_error *)(curr & ~kShutdownBit); GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "FD Shutdown", &shutdown_err, 1)); @@ -142,22 +129,22 @@ void grpc_lfev_notify_on(grpc_exec_ctx *exec_ctx, gpr_atm *state, GPR_UNREACHABLE_CODE(return ); } -bool grpc_lfev_set_shutdown(grpc_exec_ctx *exec_ctx, gpr_atm *state, - grpc_error *shutdown_err) { - gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; +bool LockfreeEvent::SetShutdown(grpc_exec_ctx *exec_ctx, + grpc_error *shutdown_err) { + gpr_atm new_state = (gpr_atm)shutdown_err | kShutdownBit; while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(state); + gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_set_shutdown: %p curr=%p err=%s", state, + gpr_log(GPR_ERROR, "lfev_set_shutdown: %p curr=%p err=%s", &state_, (void *)curr, grpc_error_string(shutdown_err)); } switch (curr) { - case CLOSURE_READY: - case CLOSURE_NOT_READY: + case kClosureReady: + case kClosureNotReady: /* Need a full barrier here so that the initial load in notify_on doesn't need a barrier */ - if (gpr_atm_full_cas(state, curr, new_state)) { + if (gpr_atm_full_cas(&state_, curr, new_state)) { return true; /* early out */ } break; /* retry */ @@ -166,7 +153,7 @@ bool grpc_lfev_set_shutdown(grpc_exec_ctx *exec_ctx, gpr_atm *state, /* 'curr' is either a closure or the fd is already shutdown */ /* If fd is already shutdown, we are done */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { + if ((curr & kShutdownBit) > 0) { GRPC_ERROR_UNREF(shutdown_err); return false; } @@ -176,7 +163,7 @@ bool grpc_lfev_set_shutdown(grpc_exec_ctx *exec_ctx, gpr_atm *state, Needs an acquire to pair with setting the closure (and get a happens-after on that edge), and a release to pair with anything loading the shutdown state. */ - if (gpr_atm_full_cas(state, curr, new_state)) { + if (gpr_atm_full_cas(&state_, curr, new_state)) { GRPC_CLOSURE_SCHED(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "FD Shutdown", &shutdown_err, 1)); @@ -193,26 +180,24 @@ bool grpc_lfev_set_shutdown(grpc_exec_ctx *exec_ctx, gpr_atm *state, GPR_UNREACHABLE_CODE(return false); } -void grpc_lfev_set_ready(grpc_exec_ctx *exec_ctx, gpr_atm *state, - const char *variable) { +void LockfreeEvent::SetReady(grpc_exec_ctx *exec_ctx) { while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(state); + gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_set_ready[%s]: %p curr=%p", variable, state, - (void *)curr); + gpr_log(GPR_ERROR, "lfev_set_ready: %p curr=%p", &state_, (void *)curr); } switch (curr) { - case CLOSURE_READY: { + case kClosureReady: { /* Already ready. We are done here */ return; } - case CLOSURE_NOT_READY: { + case kClosureNotReady: { /* No barrier required as we're transitioning to a state that does not involve a closure */ - if (gpr_atm_no_barrier_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { + if (gpr_atm_no_barrier_cas(&state_, kClosureNotReady, kClosureReady)) { return; /* early out */ } break; /* retry */ @@ -220,14 +205,14 @@ void grpc_lfev_set_ready(grpc_exec_ctx *exec_ctx, gpr_atm *state, default: { /* 'curr' is either a closure or the fd is shutdown */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { + if ((curr & kShutdownBit) > 0) { /* The fd is shutdown. Do nothing */ return; } /* Full cas: acquire pairs with this cas' release in the event of a spurious set_ready; release pairs with this or the acquire in notify_on (or set_shutdown) */ - else if (gpr_atm_full_cas(state, curr, CLOSURE_NOT_READY)) { + else if (gpr_atm_full_cas(&state_, curr, kClosureNotReady)) { GRPC_CLOSURE_SCHED(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_NONE); return; } @@ -239,3 +224,5 @@ void grpc_lfev_set_ready(grpc_exec_ctx *exec_ctx, gpr_atm *state, } } } + +} // namespace grpc_core diff --git a/src/core/lib/iomgr/lockfree_event.h b/src/core/lib/iomgr/lockfree_event.h index 925f004945..d843c340a1 100644 --- a/src/core/lib/iomgr/lockfree_event.h +++ b/src/core/lib/iomgr/lockfree_event.h @@ -25,24 +25,29 @@ #include "src/core/lib/iomgr/exec_ctx.h" -#ifdef __cplusplus -extern "C" { -#endif - -void grpc_lfev_init(gpr_atm *state); -void grpc_lfev_destroy(gpr_atm *state); -bool grpc_lfev_is_shutdown(gpr_atm *state); - -void grpc_lfev_notify_on(grpc_exec_ctx *exec_ctx, gpr_atm *state, - grpc_closure *closure, const char *variable); -/* Returns true on first successful shutdown */ -bool grpc_lfev_set_shutdown(grpc_exec_ctx *exec_ctx, gpr_atm *state, - grpc_error *shutdown_err); -void grpc_lfev_set_ready(grpc_exec_ctx *exec_ctx, gpr_atm *state, - const char *variable); - -#ifdef __cplusplus -} -#endif - -#endif /* GRPC_CORE_LIB_IOMGR_LOCKFREE_EVENT_H */ \ No newline at end of file +namespace grpc_core { + +class LockfreeEvent { + public: + ~LockfreeEvent(); + + LockfreeEvent(const LockfreeEvent &) = delete; + LockfreeEvent &operator=(const LockfreeEvent &) = delete; + + bool IsShutdown() const { + return (gpr_atm_no_barrier_load(&state_) & kShutdownBit) != 0; + } + + void NotifyOn(grpc_exec_ctx *exec_ctx, grpc_closure *closure); + bool SetShutdown(grpc_exec_ctx *exec_ctx, grpc_error *error); + void SetReady(grpc_exec_ctx *exec_ctx); + + private: + enum State { kClosureNotReady = 0, kClosureReady = 2, kShutdownBit = 1 }; + + gpr_atm state_ = kClosureNotReady; +}; + +} // namespace grpc_core + +#endif /* GRPC_CORE_LIB_IOMGR_LOCKFREE_EVENT_H */ -- cgit v1.2.3 From fb222de58fe47e79bfb184ead2483b41d8b9e62e Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Sun, 22 Oct 2017 04:27:56 -0700 Subject: Convert epollexclusive --- src/core/lib/iomgr/ev_epollex_linux.cc | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index fa6d79cbfc..d7d5f219aa 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -48,6 +48,7 @@ #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/manual_constructor.h" #include "src/core/lib/support/spinlock.h" // debug aid: create workers on the heap (allows asan to spot @@ -153,8 +154,8 @@ struct grpc_fd { gpr_mu pollable_mu; pollable *pollable_obj; - gpr_atm read_closure; - gpr_atm write_closure; + grpc_core::ManualConstructor read_closure; + grpc_core::ManualConstructor write_closure; struct grpc_fd *freelist_next; grpc_closure *on_done_closure; @@ -286,8 +287,8 @@ static void fd_destroy(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { fd->freelist_next = fd_freelist; fd_freelist = fd; - grpc_lfev_destroy(&fd->read_closure); - grpc_lfev_destroy(&fd->write_closure); + fd->read_closure.Destroy(); + fd->write_closure.Destroy(); gpr_mu_unlock(&fd_freelist_mu); } @@ -346,8 +347,8 @@ static grpc_fd *fd_create(int fd, const char *name) { new_fd->pollable_obj = NULL; gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; - grpc_lfev_init(&new_fd->read_closure); - grpc_lfev_init(&new_fd->write_closure); + new_fd->read_closure.Init(); + new_fd->write_closure.Init(); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -410,27 +411,26 @@ static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, } static bool fd_is_shutdown(grpc_fd *fd) { - return grpc_lfev_is_shutdown(&fd->read_closure); + return fd->read_closure->IsShutdown(); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { - if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, - GRPC_ERROR_REF(why))) { + if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { shutdown(fd->fd, SHUT_RDWR); - grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); + fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } static void fd_notify_on_read(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); + fd->read_closure->NotifyOn(exec_ctx, closure); } static void fd_notify_on_write(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); + fd->write_closure->NotifyOn(exec_ctx, closure); } /******************************************************************************* @@ -701,7 +701,7 @@ static int poll_deadline_to_millis_timeout(grpc_exec_ctx *exec_ctx, static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_pollset *notifier) { - grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); + fd->read_closure->SetReady(exec_ctx); /* Note, it is possible that fd_become_readable might be called twice with different 'notifier's when an fd becomes readable and it is in two epoll @@ -713,7 +713,7 @@ static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, } static void fd_become_writable(grpc_exec_ctx *exec_ctx, grpc_fd *fd) { - grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); + fd->write_closure->SetReady(exec_ctx); } static grpc_error *fd_get_or_become_pollable(grpc_fd *fd, pollable **p) { -- cgit v1.2.3 From e098d2eb5bfa58376a60b49989cb66c59b420755 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 1 Nov 2017 15:34:46 -0700 Subject: Finish conversion --- src/core/lib/iomgr/ev_epoll1_linux.cc | 30 +++++++++++++++--------------- src/core/lib/iomgr/ev_epollsig_linux.cc | 28 ++++++++++++++-------------- src/core/lib/iomgr/lockfree_event.h | 1 + 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/core/lib/iomgr/ev_epoll1_linux.cc b/src/core/lib/iomgr/ev_epoll1_linux.cc index 6126e2771c..9fc560c542 100644 --- a/src/core/lib/iomgr/ev_epoll1_linux.cc +++ b/src/core/lib/iomgr/ev_epoll1_linux.cc @@ -46,6 +46,7 @@ #include "src/core/lib/iomgr/lockfree_event.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/manual_constructor.h" #include "src/core/lib/support/string.h" static grpc_wakeup_fd global_wakeup_fd; @@ -111,8 +112,8 @@ static void epoll_set_shutdown() { struct grpc_fd { int fd; - gpr_atm read_closure; - gpr_atm write_closure; + grpc_core::ManualConstructor read_closure; + grpc_core::ManualConstructor write_closure; struct grpc_fd *freelist_next; @@ -264,8 +265,8 @@ static grpc_fd *fd_create(int fd, const char *name) { } new_fd->fd = fd; - grpc_lfev_init(&new_fd->read_closure); - grpc_lfev_init(&new_fd->write_closure); + new_fd->read_closure.Init(); + new_fd->write_closure.Init(); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -297,12 +298,11 @@ static int fd_wrapped_fd(grpc_fd *fd) { return fd->fd; } * shutdown() syscall on that fd) */ static void fd_shutdown_internal(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why, bool releasing_fd) { - if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, - GRPC_ERROR_REF(why))) { + if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { if (!releasing_fd) { shutdown(fd->fd, SHUT_RDWR); } - grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); + fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } @@ -318,7 +318,7 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *error = GRPC_ERROR_NONE; bool is_release_fd = (release_fd != NULL); - if (!grpc_lfev_is_shutdown(&fd->read_closure)) { + if (!fd->read_closure->IsShutdown()) { fd_shutdown_internal(exec_ctx, fd, GRPC_ERROR_CREATE_FROM_COPIED_STRING(reason), is_release_fd); @@ -335,8 +335,8 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, GRPC_CLOSURE_SCHED(exec_ctx, on_done, GRPC_ERROR_REF(error)); grpc_iomgr_unregister_object(&fd->iomgr_object); - grpc_lfev_destroy(&fd->read_closure); - grpc_lfev_destroy(&fd->write_closure); + fd->read_closure.Destroy(); + fd->write_closure.Destroy(); gpr_mu_lock(&fd_freelist_mu); fd->freelist_next = fd_freelist; @@ -351,28 +351,28 @@ static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, } static bool fd_is_shutdown(grpc_fd *fd) { - return grpc_lfev_is_shutdown(&fd->read_closure); + return fd->read_closure->IsShutdown(); } static void fd_notify_on_read(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); + fd->read_closure->NotifyOn(exec_ctx, closure); } static void fd_notify_on_write(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); + fd->write_closure->NotifyOn(exec_ctx, closure); } static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_pollset *notifier) { - grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); + fd->read_closure->SetReady(exec_ctx); /* Use release store to match with acquire load in fd_get_read_notifier */ gpr_atm_rel_store(&fd->read_notifier_pollset, (gpr_atm)notifier); } static void fd_become_writable(grpc_exec_ctx *exec_ctx, grpc_fd *fd) { - grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); + fd->write_closure->SetReady(exec_ctx); } /******************************************************************************* diff --git a/src/core/lib/iomgr/ev_epollsig_linux.cc b/src/core/lib/iomgr/ev_epollsig_linux.cc index 035bdc4cb5..46333569c3 100644 --- a/src/core/lib/iomgr/ev_epollsig_linux.cc +++ b/src/core/lib/iomgr/ev_epollsig_linux.cc @@ -50,6 +50,7 @@ #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/manual_constructor.h" #define GRPC_POLLSET_KICK_BROADCAST ((grpc_pollset_worker *)1) @@ -127,8 +128,8 @@ struct grpc_fd { valid */ bool orphaned; - gpr_atm read_closure; - gpr_atm write_closure; + grpc_core::ManualConstructor read_closure; + grpc_core::ManualConstructor write_closure; struct grpc_fd *freelist_next; grpc_closure *on_done_closure; @@ -764,8 +765,8 @@ static void unref_by(grpc_fd *fd, int n) { fd_freelist = fd; grpc_iomgr_unregister_object(&fd->iomgr_object); - grpc_lfev_destroy(&fd->read_closure); - grpc_lfev_destroy(&fd->write_closure); + fd->read_closure.Destroy(); + fd->write_closure.Destroy(); gpr_mu_unlock(&fd_freelist_mu); } else { @@ -830,8 +831,8 @@ static grpc_fd *fd_create(int fd, const char *name) { gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; new_fd->orphaned = false; - grpc_lfev_init(&new_fd->read_closure); - grpc_lfev_init(&new_fd->write_closure); + new_fd->read_closure.Init(); + new_fd->write_closure.Init(); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -922,27 +923,26 @@ static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, } static bool fd_is_shutdown(grpc_fd *fd) { - return grpc_lfev_is_shutdown(&fd->read_closure); + return fd->read_closure->IsShutdown(); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { - if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, - GRPC_ERROR_REF(why))) { + if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { shutdown(fd->fd, SHUT_RDWR); - grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); + fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } static void fd_notify_on_read(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); + fd->read_closure->NotifyOn(exec_ctx, closure); } static void fd_notify_on_write(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); + fd->write_closure->NotifyOn(exec_ctx, closure); } /******************************************************************************* @@ -1106,7 +1106,7 @@ static int poll_deadline_to_millis_timeout(grpc_exec_ctx *exec_ctx, static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_pollset *notifier) { - grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); + fd->read_closure->SetReady(exec_ctx); /* Note, it is possible that fd_become_readable might be called twice with different 'notifier's when an fd becomes readable and it is in two epoll @@ -1118,7 +1118,7 @@ static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, } static void fd_become_writable(grpc_exec_ctx *exec_ctx, grpc_fd *fd) { - grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); + fd->write_closure->SetReady(exec_ctx); } static void pollset_release_polling_island(grpc_exec_ctx *exec_ctx, diff --git a/src/core/lib/iomgr/lockfree_event.h b/src/core/lib/iomgr/lockfree_event.h index d843c340a1..c985feef89 100644 --- a/src/core/lib/iomgr/lockfree_event.h +++ b/src/core/lib/iomgr/lockfree_event.h @@ -29,6 +29,7 @@ namespace grpc_core { class LockfreeEvent { public: + LockfreeEvent() = default; ~LockfreeEvent(); LockfreeEvent(const LockfreeEvent &) = delete; -- cgit v1.2.3 From c60659ad023d01f8d0f6ad0f87fefbd3740002d0 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 1 Nov 2017 15:36:13 -0700 Subject: Review feedback --- src/core/lib/iomgr/lockfree_event.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/core/lib/iomgr/lockfree_event.cc b/src/core/lib/iomgr/lockfree_event.cc index 549747b34b..d1273beee3 100644 --- a/src/core/lib/iomgr/lockfree_event.cc +++ b/src/core/lib/iomgr/lockfree_event.cc @@ -70,7 +70,7 @@ void LockfreeEvent::NotifyOn(grpc_exec_ctx *exec_ctx, grpc_closure *closure) { while (true) { gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_notify_on: %p curr=%p closure=%p", this, + gpr_log(GPR_ERROR, "LockfreeEvent::NotifyOn: %p curr=%p closure=%p", this, (void *)curr, closure); } switch (curr) { @@ -120,7 +120,8 @@ void LockfreeEvent::NotifyOn(grpc_exec_ctx *exec_ctx, grpc_closure *closure) { /* There is already a closure!. This indicates a bug in the code */ gpr_log(GPR_ERROR, - "notify_on called with a previous callback still pending"); + "LockfreeEvent::NotifyOn: notify_on called with a previous " + "callback still pending"); abort(); } } @@ -136,8 +137,8 @@ bool LockfreeEvent::SetShutdown(grpc_exec_ctx *exec_ctx, while (true) { gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_set_shutdown: %p curr=%p err=%s", &state_, - (void *)curr, grpc_error_string(shutdown_err)); + gpr_log(GPR_ERROR, "LockfreeEvent::SetShutdown: %p curr=%p err=%s", + &state_, (void *)curr, grpc_error_string(shutdown_err)); } switch (curr) { case kClosureReady: @@ -185,7 +186,8 @@ void LockfreeEvent::SetReady(grpc_exec_ctx *exec_ctx) { gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_set_ready: %p curr=%p", &state_, (void *)curr); + gpr_log(GPR_ERROR, "LockfreeEvent::SetReady: %p curr=%p", &state_, + (void *)curr); } switch (curr) { -- cgit v1.2.3 From 7f3f30f33304db3c58210af0f355364c0b39fbc5 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sat, 4 Nov 2017 00:24:12 -0700 Subject: Fix resource_quota_server bug --- src/core/ext/transport/chttp2/transport/chttp2_transport.cc | 11 ++++++++++- src/core/ext/transport/chttp2/transport/internal.h | 8 +++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 02fc53122d..42664a753e 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1121,6 +1121,7 @@ void grpc_chttp2_add_incoming_goaway(grpc_exec_ctx *exec_ctx, // GRPC_CHTTP2_IF_TRACING( // gpr_log(GPR_DEBUG, "got goaway [%d]: %s", goaway_error, msg)); t->seen_goaway = 1; + t->goaway_error = goaway_error; /* When a client receives a GOAWAY with error code ENHANCE_YOUR_CALM and debug * data equal to "too_many_pings", it should log the occurrence at a log level @@ -2074,7 +2075,6 @@ void grpc_chttp2_fake_status(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_status_code status; grpc_slice slice; grpc_error_get_status(exec_ctx, error, s->deadline, &status, &slice, NULL); - if (status != GRPC_STATUS_OK) { s->seen_error = true; } @@ -2542,6 +2542,15 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, "Transport closed", &t->closed_with_error, 1); } if (error != GRPC_ERROR_NONE) { + /* If a goaway frame was received, this might be the reason why the read + * failed. Add this info to the error */ + if (t->seen_goaway) { + error = grpc_error_add_child( + error, grpc_error_set_int( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), + GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)t->goaway_error)); + } + close_transport_locked(exec_ctx, t, GRPC_ERROR_REF(error)); t->endpoint_reading = 0; } else if (t->closed_with_error == GRPC_ERROR_NONE) { diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 9e0796e820..bd25e51ec0 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -310,6 +310,9 @@ struct grpc_chttp2_transport { bool seen_goaway; /** have we sent a goaway */ grpc_chttp2_sent_goaway_state sent_goaway_state; + /** http2 error code received in the GOAWAY frame. Only relevant if + * seen_goaway is true */ + uint32_t goaway_error; /** are the local settings dirty and need to be sent? */ bool dirtied_local_settings; @@ -376,11 +379,6 @@ struct grpc_chttp2_transport { grpc_chttp2_transport *t, grpc_chttp2_stream *s, grpc_slice slice, int is_last); - /* goaway data */ - grpc_status_code goaway_error; - uint32_t goaway_last_stream_index; - grpc_slice goaway_text; - grpc_chttp2_write_cb *write_cb_pool; /* bdp estimator */ -- cgit v1.2.3 From 0c1d8e2086b055b6e322e00e52223ec5359fca4b Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Mon, 6 Nov 2017 10:25:28 -0800 Subject: Review comment: merge seen_goaway and goaway_error fields on transport --- .../transport/chttp2/transport/chttp2_transport.cc | 32 ++++++++++++---------- src/core/ext/transport/chttp2/transport/internal.h | 10 +++---- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 42664a753e..2a156f5f7e 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -205,6 +205,8 @@ static void destruct_transport(grpc_exec_ctx *exec_ctx, GPR_ASSERT(t->lists[i].tail == NULL); } + GRPC_ERROR_UNREF(t->goaway_error); + GPR_ASSERT(grpc_chttp2_stream_map_size(&t->stream_map) == 0); grpc_chttp2_stream_map_destroy(&t->stream_map); @@ -320,6 +322,7 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, keepalive_watchdog_fired_locked, t, grpc_combiner_scheduler(t->combiner)); + t->goaway_error = GRPC_ERROR_NONE; grpc_chttp2_goaway_parser_init(&t->goaway_parser); grpc_chttp2_hpack_parser_init(exec_ctx, &t->hpack_parser); @@ -1120,8 +1123,16 @@ void grpc_chttp2_add_incoming_goaway(grpc_exec_ctx *exec_ctx, grpc_slice goaway_text) { // GRPC_CHTTP2_IF_TRACING( // gpr_log(GPR_DEBUG, "got goaway [%d]: %s", goaway_error, msg)); - t->seen_goaway = 1; - t->goaway_error = goaway_error; + + // Discard the error from a previous goaway frame (if any) + if (t->goaway_error != GRPC_ERROR_NONE) { + GRPC_ERROR_UNREF(t->goaway_error); + } + t->goaway_error = grpc_error_set_str( + grpc_error_set_int( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), + GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)goaway_error), + GRPC_ERROR_STR_RAW_BYTES, goaway_text); /* When a client receives a GOAWAY with error code ENHANCE_YOUR_CALM and debug * data equal to "too_many_pings", it should log the occurrence at a log level @@ -1142,14 +1153,8 @@ void grpc_chttp2_add_incoming_goaway(grpc_exec_ctx *exec_ctx, /* lie: use transient failure from the transport to indicate goaway has been * received */ - connectivity_state_set( - exec_ctx, t, GRPC_CHANNEL_TRANSIENT_FAILURE, - grpc_error_set_str( - grpc_error_set_int( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), - GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)goaway_error), - GRPC_ERROR_STR_RAW_BYTES, goaway_text), - "got_goaway"); + connectivity_state_set(exec_ctx, t, GRPC_CHANNEL_TRANSIENT_FAILURE, + GRPC_ERROR_REF(t->goaway_error), "got_goaway"); } static void maybe_start_some_streams(grpc_exec_ctx *exec_ctx, @@ -2544,11 +2549,8 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, if (error != GRPC_ERROR_NONE) { /* If a goaway frame was received, this might be the reason why the read * failed. Add this info to the error */ - if (t->seen_goaway) { - error = grpc_error_add_child( - error, grpc_error_set_int( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), - GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)t->goaway_error)); + if (t->goaway_error != GRPC_ERROR_NONE) { + error = grpc_error_add_child(error, GRPC_ERROR_REF(t->goaway_error)); } close_transport_locked(exec_ctx, t, GRPC_ERROR_REF(error)); diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index bd25e51ec0..3a20b653a4 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -306,13 +306,11 @@ struct grpc_chttp2_transport { */ uint32_t write_buffer_size; - /** have we seen a goaway */ - bool seen_goaway; - /** have we sent a goaway */ + /** Set to a grpc_error object if a goaway frame is received. By default, set + * to GRPC_ERROR_NONE */ + grpc_error *goaway_error; + grpc_chttp2_sent_goaway_state sent_goaway_state; - /** http2 error code received in the GOAWAY frame. Only relevant if - * seen_goaway is true */ - uint32_t goaway_error; /** are the local settings dirty and need to be sent? */ bool dirtied_local_settings; -- cgit v1.2.3 From 30101b05c6cb9ad2bb90bbb1ef2d7a4b9c145ca5 Mon Sep 17 00:00:00 2001 From: yang-g Date: Mon, 6 Nov 2017 14:35:30 -0800 Subject: log GRPC_POLL_STRATEGY when fail to find polling engine also log reason to skip a polling engine --- src/core/lib/iomgr/ev_epoll1_linux.cc | 3 +++ src/core/lib/iomgr/ev_epollex_linux.cc | 4 ++++ src/core/lib/iomgr/ev_epollsig_linux.cc | 8 ++++++++ src/core/lib/iomgr/ev_poll_posix.cc | 1 + src/core/lib/iomgr/ev_posix.cc | 4 ++-- 5 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/core/lib/iomgr/ev_epoll1_linux.cc b/src/core/lib/iomgr/ev_epoll1_linux.cc index 6126e2771c..435b6f0f37 100644 --- a/src/core/lib/iomgr/ev_epoll1_linux.cc +++ b/src/core/lib/iomgr/ev_epoll1_linux.cc @@ -1230,6 +1230,7 @@ static const grpc_event_engine_vtable vtable = { * support is available */ const grpc_event_engine_vtable *grpc_init_epoll1_linux(bool explicit_request) { if (!grpc_has_wakeup_fd()) { + gpr_log(GPR_ERROR, "Skipping epoll1 because of no wakeup fd."); return NULL; } @@ -1254,6 +1255,8 @@ const grpc_event_engine_vtable *grpc_init_epoll1_linux(bool explicit_request) { /* If GRPC_LINUX_EPOLL is not defined, it means epoll is not available. Return * NULL */ const grpc_event_engine_vtable *grpc_init_epoll1_linux(bool explicit_request) { + gpr_log(GPR_ERROR, + "Skipping epollsig becuase GRPC_LINUX_EPOLL is not defined."); return NULL; } #endif /* defined(GRPC_POSIX_SOCKET) */ diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 0809d574a9..9cafb1c61e 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -1447,10 +1447,12 @@ const grpc_event_engine_vtable *grpc_init_epollex_linux( } if (!grpc_has_wakeup_fd()) { + gpr_log(GPR_ERROR, "Skipping epollex because of no wakeup fd."); return NULL; } if (!grpc_is_epollexclusive_available()) { + gpr_log(GPR_INFO, "Skipping epollex because it is not supported."); return NULL; } @@ -1476,6 +1478,8 @@ const grpc_event_engine_vtable *grpc_init_epollex_linux( * NULL */ const grpc_event_engine_vtable *grpc_init_epollex_linux( bool explicitly_requested) { + gpr_log(GPR_ERROR, + "Skipping epollex becuase GRPC_LINUX_EPOLL is not defined."); return NULL; } #endif /* defined(GRPC_POSIX_SOCKET) */ diff --git a/src/core/lib/iomgr/ev_epollsig_linux.cc b/src/core/lib/iomgr/ev_epollsig_linux.cc index 035bdc4cb5..0473db9b95 100644 --- a/src/core/lib/iomgr/ev_epollsig_linux.cc +++ b/src/core/lib/iomgr/ev_epollsig_linux.cc @@ -1708,16 +1708,20 @@ static bool is_epoll_available() { const grpc_event_engine_vtable *grpc_init_epollsig_linux( bool explicit_request) { + const char *error_msg = NULL; /* If use of signals is disabled, we cannot use epoll engine*/ if (is_grpc_wakeup_signal_initialized && grpc_wakeup_signal < 0) { + gpr_log(GPR_ERROR, "Skipping epollsig because use of signals is disabled."); return NULL; } if (!grpc_has_wakeup_fd()) { + gpr_log(GPR_ERROR, "Skipping epollsig because of no wakeup fd."); return NULL; } if (!is_epoll_available()) { + gpr_log(GPR_ERROR, "Skipping epollsig because epoll is unavailable."); return NULL; } @@ -1725,6 +1729,8 @@ const grpc_event_engine_vtable *grpc_init_epollsig_linux( if (explicit_request) { grpc_use_signal(SIGRTMIN + 6); } else { + gpr_log(GPR_ERROR, + "Skipping epollsig because uninitialized wakeup signal."); return NULL; } } @@ -1750,6 +1756,8 @@ const grpc_event_engine_vtable *grpc_init_epollsig_linux( * NULL */ const grpc_event_engine_vtable *grpc_init_epollsig_linux( bool explicit_request) { + gpr_log(GPR_ERROR, + "Skipping epollsig becuase GRPC_LINUX_EPOLL is not defined."); return NULL; } #endif /* defined(GRPC_POSIX_SOCKET) */ diff --git a/src/core/lib/iomgr/ev_poll_posix.cc b/src/core/lib/iomgr/ev_poll_posix.cc index 036a35690c..c3d174d097 100644 --- a/src/core/lib/iomgr/ev_poll_posix.cc +++ b/src/core/lib/iomgr/ev_poll_posix.cc @@ -1713,6 +1713,7 @@ static const grpc_event_engine_vtable vtable = { const grpc_event_engine_vtable *grpc_init_poll_posix(bool explicit_request) { if (!grpc_has_wakeup_fd()) { + gpr_log(GPR_ERROR, "Skipping poll because of no wakeup fd."); return NULL; } if (!GRPC_LOG_IF_ERROR("pollset_global_init", pollset_global_init())) { diff --git a/src/core/lib/iomgr/ev_posix.cc b/src/core/lib/iomgr/ev_posix.cc index 677ee675a6..9fd598b65e 100644 --- a/src/core/lib/iomgr/ev_posix.cc +++ b/src/core/lib/iomgr/ev_posix.cc @@ -172,12 +172,12 @@ void grpc_event_engine_init(void) { gpr_free(strings[i]); } gpr_free(strings); - gpr_free(s); if (g_event_engine == NULL) { - gpr_log(GPR_ERROR, "No event engine could be initialized"); + gpr_log(GPR_ERROR, "No event engine could be initialized from %s", s); abort(); } + gpr_free(s); } void grpc_event_engine_shutdown(void) { -- cgit v1.2.3 From 9a0db888855e0905946ab476892ba697a6a9200d Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Mon, 6 Nov 2017 16:47:33 -0800 Subject: Fix TSAN issue in backup poller --- src/core/ext/filters/client_channel/backup_poller.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/core/ext/filters/client_channel/backup_poller.cc b/src/core/ext/filters/client_channel/backup_poller.cc index 466bf86bc0..45b2464e27 100644 --- a/src/core/ext/filters/client_channel/backup_poller.cc +++ b/src/core/ext/filters/client_channel/backup_poller.cc @@ -143,9 +143,16 @@ void grpc_client_channel_start_backup_polling( grpc_exec_ctx_now(exec_ctx) + g_poll_interval_ms, &g_poller->run_poller_closure); } + gpr_ref(&g_poller->refs); + /* Get a reference to g_poller->pollset before releasing g_poller_mu to make + * TSAN happy. Otherwise, reading from g_poller (i.e g_poller->pollset) after + * releasing the lock and setting g_poller to NULL in g_poller_unref() is + * being flagged as a data-race by TSAN */ + grpc_pollset *pollset = g_poller->pollset; gpr_mu_unlock(&g_poller_mu); - grpc_pollset_set_add_pollset(exec_ctx, interested_parties, g_poller->pollset); + + grpc_pollset_set_add_pollset(exec_ctx, interested_parties, pollset); } void grpc_client_channel_stop_backup_polling( -- cgit v1.2.3 From 802f762f17b8729ef4d72f55b22aa21fa15f992b Mon Sep 17 00:00:00 2001 From: yang-g Date: Tue, 7 Nov 2017 11:35:18 -0800 Subject: remove unused variable --- src/core/lib/iomgr/ev_epollsig_linux.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/lib/iomgr/ev_epollsig_linux.cc b/src/core/lib/iomgr/ev_epollsig_linux.cc index 91336df58f..7005539b39 100644 --- a/src/core/lib/iomgr/ev_epollsig_linux.cc +++ b/src/core/lib/iomgr/ev_epollsig_linux.cc @@ -1709,7 +1709,6 @@ static bool is_epoll_available() { const grpc_event_engine_vtable* grpc_init_epollsig_linux( bool explicit_request) { - const char *error_msg = NULL; /* If use of signals is disabled, we cannot use epoll engine*/ if (is_grpc_wakeup_signal_initialized && grpc_wakeup_signal < 0) { gpr_log(GPR_ERROR, "Skipping epollsig because use of signals is disabled."); -- cgit v1.2.3 From ceb24750e74d37288686d2a5601c1fa846b7df31 Mon Sep 17 00:00:00 2001 From: yang-g Date: Tue, 7 Nov 2017 12:06:37 -0800 Subject: include log.h for mac --- src/core/lib/iomgr/ev_epoll1_linux.cc | 3 ++- src/core/lib/iomgr/ev_epollex_linux.cc | 3 ++- src/core/lib/iomgr/ev_epollsig_linux.cc | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/core/lib/iomgr/ev_epoll1_linux.cc b/src/core/lib/iomgr/ev_epoll1_linux.cc index f519cc086c..28981e8ca6 100644 --- a/src/core/lib/iomgr/ev_epoll1_linux.cc +++ b/src/core/lib/iomgr/ev_epoll1_linux.cc @@ -18,6 +18,8 @@ #include "src/core/lib/iomgr/port.h" +#include + /* This polling engine is only relevant on linux kernels supporting epoll() */ #ifdef GRPC_LINUX_EPOLL #include "src/core/lib/iomgr/ev_epoll1_linux.h" @@ -34,7 +36,6 @@ #include #include -#include #include #include #include diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 4276effa3f..29eeccc1ef 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -18,6 +18,8 @@ #include "src/core/lib/iomgr/port.h" +#include + /* This polling engine is only relevant on linux kernels supporting epoll() */ #ifdef GRPC_LINUX_EPOLL @@ -34,7 +36,6 @@ #include #include -#include #include #include #include diff --git a/src/core/lib/iomgr/ev_epollsig_linux.cc b/src/core/lib/iomgr/ev_epollsig_linux.cc index 7005539b39..61f2a57062 100644 --- a/src/core/lib/iomgr/ev_epollsig_linux.cc +++ b/src/core/lib/iomgr/ev_epollsig_linux.cc @@ -19,6 +19,7 @@ #include "src/core/lib/iomgr/port.h" #include +#include /* This polling engine is only relevant on linux kernels supporting epoll() */ #ifdef GRPC_LINUX_EPOLL @@ -37,7 +38,6 @@ #include #include -#include #include #include #include -- cgit v1.2.3 From 5dd0d6fadb20f297ec04753a19f1e2a2fd03735d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 7 Nov 2017 17:18:55 -0800 Subject: Fix internal UBSAN failure --- src/core/ext/filters/client_channel/uri_parser.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/ext/filters/client_channel/uri_parser.cc b/src/core/ext/filters/client_channel/uri_parser.cc index 917e65342b..1cc52dec12 100644 --- a/src/core/ext/filters/client_channel/uri_parser.cc +++ b/src/core/ext/filters/client_channel/uri_parser.cc @@ -59,7 +59,9 @@ static grpc_uri* bad_uri(const char* uri_text, size_t pos, const char* section, static char* decode_and_copy_component(grpc_exec_ctx* exec_ctx, const char* src, size_t begin, size_t end) { grpc_slice component = - grpc_slice_from_copied_buffer(src + begin, end - begin); + (begin == NOT_SET || end == NOT_SET) + ? grpc_empty_slice() + : grpc_slice_from_copied_buffer(src + begin, end - begin); grpc_slice decoded_component = grpc_permissive_percent_decode_slice(component); char* out = grpc_dump_slice(decoded_component, GPR_DUMP_ASCII); -- cgit v1.2.3 From c284139d2f331d2d3a8f7314107e410a5d6f408f Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 7 Nov 2017 17:58:00 -0800 Subject: Remove include from end2end data files and add extern C to each definition instead --- test/core/end2end/data/client_certs.cc | 10 ++++------ test/core/end2end/data/server1_cert.cc | 4 +--- test/core/end2end/data/server1_key.cc | 4 +--- test/core/end2end/data/test_root_cert.cc | 4 +--- 4 files changed, 7 insertions(+), 15 deletions(-) diff --git a/test/core/end2end/data/client_certs.cc b/test/core/end2end/data/client_certs.cc index 6e61501234..7e0b10da5c 100644 --- a/test/core/end2end/data/client_certs.cc +++ b/test/core/end2end/data/client_certs.cc @@ -16,9 +16,7 @@ * */ -#include "test/core/end2end/data/ssl_test_data.h" - -const char test_self_signed_client_cert[] = { +extern "C" const char test_self_signed_client_cert[] = { 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x42, 0x45, 0x47, 0x49, 0x4e, 0x20, 0x43, 0x45, 0x52, 0x54, 0x49, 0x46, 0x49, 0x43, 0x41, 0x54, 0x45, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x4d, 0x49, 0x49, 0x43, 0x6f, 0x44, 0x43, 0x43, @@ -102,7 +100,7 @@ const char test_self_signed_client_cert[] = { 0x49, 0x46, 0x49, 0x43, 0x41, 0x54, 0x45, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x00}; -const char test_self_signed_client_key[] = { +extern "C" const char test_self_signed_client_key[] = { 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x42, 0x45, 0x47, 0x49, 0x4e, 0x20, 0x50, 0x52, 0x49, 0x56, 0x41, 0x54, 0x45, 0x20, 0x4b, 0x45, 0x59, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x4d, 0x49, 0x49, 0x43, 0x64, 0x77, 0x49, 0x42, @@ -181,7 +179,7 @@ const char test_self_signed_client_key[] = { 0x52, 0x49, 0x56, 0x41, 0x54, 0x45, 0x20, 0x4b, 0x45, 0x59, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x00}; -const char test_signed_client_cert[] = { +extern "C" const char test_signed_client_cert[] = { 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x42, 0x45, 0x47, 0x49, 0x4e, 0x20, 0x43, 0x45, 0x52, 0x54, 0x49, 0x46, 0x49, 0x43, 0x41, 0x54, 0x45, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x4d, 0x49, 0x49, 0x43, 0x48, 0x7a, 0x43, 0x43, @@ -250,7 +248,7 @@ const char test_signed_client_cert[] = { 0x20, 0x43, 0x45, 0x52, 0x54, 0x49, 0x46, 0x49, 0x43, 0x41, 0x54, 0x45, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x00}; -const char test_signed_client_key[] = { +extern "C" const char test_signed_client_key[] = { 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x42, 0x45, 0x47, 0x49, 0x4e, 0x20, 0x50, 0x52, 0x49, 0x56, 0x41, 0x54, 0x45, 0x20, 0x4b, 0x45, 0x59, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x4d, 0x49, 0x49, 0x43, 0x65, 0x51, 0x49, 0x42, diff --git a/test/core/end2end/data/server1_cert.cc b/test/core/end2end/data/server1_cert.cc index 5e017c4da7..dd09810732 100644 --- a/test/core/end2end/data/server1_cert.cc +++ b/test/core/end2end/data/server1_cert.cc @@ -16,9 +16,7 @@ * */ -#include "test/core/end2end/data/ssl_test_data.h" - -const char test_server1_cert[] = { +extern "C" const char test_server1_cert[] = { 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x42, 0x45, 0x47, 0x49, 0x4e, 0x20, 0x43, 0x45, 0x52, 0x54, 0x49, 0x46, 0x49, 0x43, 0x41, 0x54, 0x45, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x4d, 0x49, 0x49, 0x43, 0x6e, 0x44, 0x43, 0x43, diff --git a/test/core/end2end/data/server1_key.cc b/test/core/end2end/data/server1_key.cc index 92a77aa21f..59dcaf6334 100644 --- a/test/core/end2end/data/server1_key.cc +++ b/test/core/end2end/data/server1_key.cc @@ -16,9 +16,7 @@ * */ -#include "test/core/end2end/data/ssl_test_data.h" - -const char test_server1_key[] = { +extern "C" const char test_server1_key[] = { 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x42, 0x45, 0x47, 0x49, 0x4e, 0x20, 0x52, 0x53, 0x41, 0x20, 0x50, 0x52, 0x49, 0x56, 0x41, 0x54, 0x45, 0x20, 0x4b, 0x45, 0x59, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x4d, 0x49, 0x49, 0x43, diff --git a/test/core/end2end/data/test_root_cert.cc b/test/core/end2end/data/test_root_cert.cc index 81ca410e14..36fca2e45f 100644 --- a/test/core/end2end/data/test_root_cert.cc +++ b/test/core/end2end/data/test_root_cert.cc @@ -16,9 +16,7 @@ * */ -#include "test/core/end2end/data/ssl_test_data.h" - -const char test_root_cert[] = { +extern "C" const char test_root_cert[] = { 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x42, 0x45, 0x47, 0x49, 0x4e, 0x20, 0x43, 0x45, 0x52, 0x54, 0x49, 0x46, 0x49, 0x43, 0x41, 0x54, 0x45, 0x2d, 0x2d, 0x2d, 0x2d, 0x2d, 0x0a, 0x4d, 0x49, 0x49, 0x43, 0x49, 0x7a, 0x43, 0x43, -- cgit v1.2.3 From 332c7e402ab5324d4d8c26aa58780cddc98c63dc Mon Sep 17 00:00:00 2001 From: "David G. Quintas" Date: Wed, 8 Nov 2017 11:01:59 -0800 Subject: Revert "Class-ify lockfree event" --- src/core/lib/iomgr/ev_epoll1_linux.cc | 30 ++++----- src/core/lib/iomgr/ev_epollex_linux.cc | 28 ++++---- src/core/lib/iomgr/ev_epollsig_linux.cc | 28 ++++---- src/core/lib/iomgr/lockfree_event.cc | 115 +++++++++++++++++--------------- src/core/lib/iomgr/lockfree_event.h | 44 ++++++------ 5 files changed, 125 insertions(+), 120 deletions(-) diff --git a/src/core/lib/iomgr/ev_epoll1_linux.cc b/src/core/lib/iomgr/ev_epoll1_linux.cc index 9e3643fa28..504c659874 100644 --- a/src/core/lib/iomgr/ev_epoll1_linux.cc +++ b/src/core/lib/iomgr/ev_epoll1_linux.cc @@ -46,7 +46,6 @@ #include "src/core/lib/iomgr/lockfree_event.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" -#include "src/core/lib/support/manual_constructor.h" #include "src/core/lib/support/string.h" static grpc_wakeup_fd global_wakeup_fd; @@ -112,8 +111,8 @@ static void epoll_set_shutdown() { struct grpc_fd { int fd; - grpc_core::ManualConstructor read_closure; - grpc_core::ManualConstructor write_closure; + gpr_atm read_closure; + gpr_atm write_closure; struct grpc_fd* freelist_next; @@ -265,8 +264,8 @@ static grpc_fd* fd_create(int fd, const char* name) { } new_fd->fd = fd; - new_fd->read_closure.Init(); - new_fd->write_closure.Init(); + grpc_lfev_init(&new_fd->read_closure); + grpc_lfev_init(&new_fd->write_closure); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -298,11 +297,12 @@ static int fd_wrapped_fd(grpc_fd* fd) { return fd->fd; } * shutdown() syscall on that fd) */ static void fd_shutdown_internal(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* why, bool releasing_fd) { - if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { + if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, + GRPC_ERROR_REF(why))) { if (!releasing_fd) { shutdown(fd->fd, SHUT_RDWR); } - fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); + grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } @@ -318,7 +318,7 @@ static void fd_orphan(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* error = GRPC_ERROR_NONE; bool is_release_fd = (release_fd != NULL); - if (!fd->read_closure->IsShutdown()) { + if (!grpc_lfev_is_shutdown(&fd->read_closure)) { fd_shutdown_internal(exec_ctx, fd, GRPC_ERROR_CREATE_FROM_COPIED_STRING(reason), is_release_fd); @@ -335,8 +335,8 @@ static void fd_orphan(grpc_exec_ctx* exec_ctx, grpc_fd* fd, GRPC_CLOSURE_SCHED(exec_ctx, on_done, GRPC_ERROR_REF(error)); grpc_iomgr_unregister_object(&fd->iomgr_object); - fd->read_closure.Destroy(); - fd->write_closure.Destroy(); + grpc_lfev_destroy(&fd->read_closure); + grpc_lfev_destroy(&fd->write_closure); gpr_mu_lock(&fd_freelist_mu); fd->freelist_next = fd_freelist; @@ -351,28 +351,28 @@ static grpc_pollset* fd_get_read_notifier_pollset(grpc_exec_ctx* exec_ctx, } static bool fd_is_shutdown(grpc_fd* fd) { - return fd->read_closure->IsShutdown(); + return grpc_lfev_is_shutdown(&fd->read_closure); } static void fd_notify_on_read(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - fd->read_closure->NotifyOn(exec_ctx, closure); + grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); } static void fd_notify_on_write(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - fd->write_closure->NotifyOn(exec_ctx, closure); + grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); } static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_pollset* notifier) { - fd->read_closure->SetReady(exec_ctx); + grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); /* Use release store to match with acquire load in fd_get_read_notifier */ gpr_atm_rel_store(&fd->read_notifier_pollset, (gpr_atm)notifier); } static void fd_become_writable(grpc_exec_ctx* exec_ctx, grpc_fd* fd) { - fd->write_closure->SetReady(exec_ctx); + grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); } /******************************************************************************* diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 26ed1f6747..aafdd690c7 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -48,7 +48,6 @@ #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" -#include "src/core/lib/support/manual_constructor.h" #include "src/core/lib/support/spinlock.h" // debug aid: create workers on the heap (allows asan to spot @@ -154,8 +153,8 @@ struct grpc_fd { gpr_mu pollable_mu; pollable* pollable_obj; - grpc_core::ManualConstructor read_closure; - grpc_core::ManualConstructor write_closure; + gpr_atm read_closure; + gpr_atm write_closure; struct grpc_fd* freelist_next; grpc_closure* on_done_closure; @@ -287,8 +286,8 @@ static void fd_destroy(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { fd->freelist_next = fd_freelist; fd_freelist = fd; - fd->read_closure.Destroy(); - fd->write_closure.Destroy(); + grpc_lfev_destroy(&fd->read_closure); + grpc_lfev_destroy(&fd->write_closure); gpr_mu_unlock(&fd_freelist_mu); } @@ -348,8 +347,8 @@ static grpc_fd* fd_create(int fd, const char* name) { new_fd->pollable_obj = NULL; gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; - new_fd->read_closure.Init(); - new_fd->write_closure.Init(); + grpc_lfev_init(&new_fd->read_closure); + grpc_lfev_init(&new_fd->write_closure); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -412,26 +411,27 @@ static grpc_pollset* fd_get_read_notifier_pollset(grpc_exec_ctx* exec_ctx, } static bool fd_is_shutdown(grpc_fd* fd) { - return fd->read_closure->IsShutdown(); + return grpc_lfev_is_shutdown(&fd->read_closure); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* why) { - if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { + if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, + GRPC_ERROR_REF(why))) { shutdown(fd->fd, SHUT_RDWR); - fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); + grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } static void fd_notify_on_read(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - fd->read_closure->NotifyOn(exec_ctx, closure); + grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); } static void fd_notify_on_write(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - fd->write_closure->NotifyOn(exec_ctx, closure); + grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); } /******************************************************************************* @@ -702,7 +702,7 @@ static int poll_deadline_to_millis_timeout(grpc_exec_ctx* exec_ctx, static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_pollset* notifier) { - fd->read_closure->SetReady(exec_ctx); + grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); /* Note, it is possible that fd_become_readable might be called twice with different 'notifier's when an fd becomes readable and it is in two epoll @@ -714,7 +714,7 @@ static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, } static void fd_become_writable(grpc_exec_ctx* exec_ctx, grpc_fd* fd) { - fd->write_closure->SetReady(exec_ctx); + grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); } static grpc_error* fd_get_or_become_pollable(grpc_fd* fd, pollable** p) { diff --git a/src/core/lib/iomgr/ev_epollsig_linux.cc b/src/core/lib/iomgr/ev_epollsig_linux.cc index 9a127806fa..d5f3122abc 100644 --- a/src/core/lib/iomgr/ev_epollsig_linux.cc +++ b/src/core/lib/iomgr/ev_epollsig_linux.cc @@ -50,7 +50,6 @@ #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" -#include "src/core/lib/support/manual_constructor.h" #define GRPC_POLLSET_KICK_BROADCAST ((grpc_pollset_worker*)1) @@ -128,8 +127,8 @@ struct grpc_fd { valid */ bool orphaned; - grpc_core::ManualConstructor read_closure; - grpc_core::ManualConstructor write_closure; + gpr_atm read_closure; + gpr_atm write_closure; struct grpc_fd* freelist_next; grpc_closure* on_done_closure; @@ -767,8 +766,8 @@ static void unref_by(grpc_fd* fd, int n) { fd_freelist = fd; grpc_iomgr_unregister_object(&fd->iomgr_object); - fd->read_closure.Destroy(); - fd->write_closure.Destroy(); + grpc_lfev_destroy(&fd->read_closure); + grpc_lfev_destroy(&fd->write_closure); gpr_mu_unlock(&fd_freelist_mu); } else { @@ -833,8 +832,8 @@ static grpc_fd* fd_create(int fd, const char* name) { gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; new_fd->orphaned = false; - new_fd->read_closure.Init(); - new_fd->write_closure.Init(); + grpc_lfev_init(&new_fd->read_closure); + grpc_lfev_init(&new_fd->write_closure); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -925,26 +924,27 @@ static grpc_pollset* fd_get_read_notifier_pollset(grpc_exec_ctx* exec_ctx, } static bool fd_is_shutdown(grpc_fd* fd) { - return fd->read_closure->IsShutdown(); + return grpc_lfev_is_shutdown(&fd->read_closure); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* why) { - if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { + if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, + GRPC_ERROR_REF(why))) { shutdown(fd->fd, SHUT_RDWR); - fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); + grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } static void fd_notify_on_read(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - fd->read_closure->NotifyOn(exec_ctx, closure); + grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); } static void fd_notify_on_write(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - fd->write_closure->NotifyOn(exec_ctx, closure); + grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); } /******************************************************************************* @@ -1108,7 +1108,7 @@ static int poll_deadline_to_millis_timeout(grpc_exec_ctx* exec_ctx, static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_pollset* notifier) { - fd->read_closure->SetReady(exec_ctx); + grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); /* Note, it is possible that fd_become_readable might be called twice with different 'notifier's when an fd becomes readable and it is in two epoll @@ -1120,7 +1120,7 @@ static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, } static void fd_become_writable(grpc_exec_ctx* exec_ctx, grpc_fd* fd) { - fd->write_closure->SetReady(exec_ctx); + grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); } static void pollset_release_polling_island(grpc_exec_ctx* exec_ctx, diff --git a/src/core/lib/iomgr/lockfree_event.cc b/src/core/lib/iomgr/lockfree_event.cc index 98e19f89df..443a8375b2 100644 --- a/src/core/lib/iomgr/lockfree_event.cc +++ b/src/core/lib/iomgr/lockfree_event.cc @@ -26,79 +26,92 @@ extern grpc_tracer_flag grpc_polling_trace; /* 'state' holds the to call when the fd is readable or writable respectively. It can contain one of the following values: - kClosureReady : The fd has an I/O event of interest but there is no + CLOSURE_READY : The fd has an I/O event of interest but there is no closure yet to execute - kClosureNotReady : The fd has no I/O event of interest + CLOSURE_NOT_READY : The fd has no I/O event of interest closure ptr : The closure to be executed when the fd has an I/O event of interest - shutdown_error | kShutdownBit : - 'shutdown_error' field ORed with kShutdownBit. + shutdown_error | FD_SHUTDOWN_BIT : + 'shutdown_error' field ORed with FD_SHUTDOWN_BIT. This indicates that the fd is shutdown. Since all memory allocations are word-aligned, the lower two bits of the shutdown_error pointer are always 0. So - it is safe to OR these with kShutdownBit + it is safe to OR these with FD_SHUTDOWN_BIT Valid state transitions: - <-----3------ kClosureNotReady -----1-------> kClosureReady + <-----3------ CLOSURE_NOT_READY ----1----> CLOSURE_READY | | ^ | ^ | | | | | | | | | | +--------------4----------+ 6 +---------2---------------+ | | | | | v | - +-----5-------> [shutdown_error | kShutdownBit] <-------7---------+ + +-----5-------> [shutdown_error | FD_SHUTDOWN_BIT] <----7---------+ - For 1, 4 : See SetReady() function - For 2, 3 : See NotifyOn() function - For 5,6,7: See SetShutdown() function */ + For 1, 4 : See grpc_lfev_set_ready() function + For 2, 3 : See grpc_lfev_notify_on() function + For 5,6,7: See grpc_lfev_set_shutdown() function */ -namespace grpc_core { +#define CLOSURE_NOT_READY ((gpr_atm)0) +#define CLOSURE_READY ((gpr_atm)2) -LockfreeEvent::~LockfreeEvent() { - gpr_atm curr = gpr_atm_no_barrier_load(&state_); - if (curr & kShutdownBit) { - GRPC_ERROR_UNREF((grpc_error*)(curr & ~kShutdownBit)); +#define FD_SHUTDOWN_BIT ((gpr_atm)1) + +void grpc_lfev_init(gpr_atm* state) { + gpr_atm_no_barrier_store(state, CLOSURE_NOT_READY); +} + +void grpc_lfev_destroy(gpr_atm* state) { + gpr_atm curr = gpr_atm_no_barrier_load(state); + if (curr & FD_SHUTDOWN_BIT) { + GRPC_ERROR_UNREF((grpc_error*)(curr & ~FD_SHUTDOWN_BIT)); } else { - GPR_ASSERT(curr == kClosureNotReady || curr == kClosureReady); + GPR_ASSERT(curr == CLOSURE_NOT_READY || curr == CLOSURE_READY); } } -void LockfreeEvent::NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure) { +bool grpc_lfev_is_shutdown(gpr_atm* state) { + gpr_atm curr = gpr_atm_no_barrier_load(state); + return (curr & FD_SHUTDOWN_BIT) != 0; +} + +void grpc_lfev_notify_on(grpc_exec_ctx* exec_ctx, gpr_atm* state, + grpc_closure* closure, const char* variable) { while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(&state_); + gpr_atm curr = gpr_atm_no_barrier_load(state); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "LockfreeEvent::NotifyOn: %p curr=%p closure=%p", this, - (void*)curr, closure); + gpr_log(GPR_ERROR, "lfev_notify_on[%s]: %p curr=%p closure=%p", variable, + state, (void*)curr, closure); } switch (curr) { - case kClosureNotReady: { - /* kClosureNotReady -> . + case CLOSURE_NOT_READY: { + /* CLOSURE_NOT_READY -> . We're guaranteed by API that there's an acquire barrier before here, so there's no need to double-dip and this can be a release-only. The release itself pairs with the acquire half of a set_ready full barrier. */ - if (gpr_atm_rel_cas(&state_, kClosureNotReady, (gpr_atm)closure)) { + if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { return; /* Successful. Return */ } break; /* retry */ } - case kClosureReady: { - /* Change the state to kClosureNotReady. Schedule the closure if + case CLOSURE_READY: { + /* Change the state to CLOSURE_NOT_READY. Schedule the closure if successful. If not, the state most likely transitioned to shutdown. We should retry. This can be a no-barrier cas since the state is being transitioned to - kClosureNotReady; set_ready and set_shutdown do not schedule any + CLOSURE_NOT_READY; set_ready and set_shutdown do not schedule any closure when transitioning out of CLOSURE_NO_READY state (i.e there is no other code that needs to 'happen-after' this) */ - if (gpr_atm_no_barrier_cas(&state_, kClosureReady, kClosureNotReady)) { + if (gpr_atm_no_barrier_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_NONE); return; /* Successful. Return */ } @@ -110,8 +123,8 @@ void LockfreeEvent::NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure) { /* 'curr' is either a closure or the fd is shutdown(in which case 'curr' contains a pointer to the shutdown-error). If the fd is shutdown, schedule the closure with the shutdown error */ - if ((curr & kShutdownBit) > 0) { - grpc_error* shutdown_err = (grpc_error*)(curr & ~kShutdownBit); + if ((curr & FD_SHUTDOWN_BIT) > 0) { + grpc_error* shutdown_err = (grpc_error*)(curr & ~FD_SHUTDOWN_BIT); GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "FD Shutdown", &shutdown_err, 1)); @@ -120,8 +133,7 @@ void LockfreeEvent::NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure) { /* There is already a closure!. This indicates a bug in the code */ gpr_log(GPR_ERROR, - "LockfreeEvent::NotifyOn: notify_on called with a previous " - "callback still pending"); + "notify_on called with a previous callback still pending"); abort(); } } @@ -130,22 +142,22 @@ void LockfreeEvent::NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure) { GPR_UNREACHABLE_CODE(return ); } -bool LockfreeEvent::SetShutdown(grpc_exec_ctx* exec_ctx, - grpc_error* shutdown_err) { - gpr_atm new_state = (gpr_atm)shutdown_err | kShutdownBit; +bool grpc_lfev_set_shutdown(grpc_exec_ctx* exec_ctx, gpr_atm* state, + grpc_error* shutdown_err) { + gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(&state_); + gpr_atm curr = gpr_atm_no_barrier_load(state); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "LockfreeEvent::SetShutdown: %p curr=%p err=%s", - &state_, (void*)curr, grpc_error_string(shutdown_err)); + gpr_log(GPR_ERROR, "lfev_set_shutdown: %p curr=%p err=%s", state, + (void*)curr, grpc_error_string(shutdown_err)); } switch (curr) { - case kClosureReady: - case kClosureNotReady: + case CLOSURE_READY: + case CLOSURE_NOT_READY: /* Need a full barrier here so that the initial load in notify_on doesn't need a barrier */ - if (gpr_atm_full_cas(&state_, curr, new_state)) { + if (gpr_atm_full_cas(state, curr, new_state)) { return true; /* early out */ } break; /* retry */ @@ -154,7 +166,7 @@ bool LockfreeEvent::SetShutdown(grpc_exec_ctx* exec_ctx, /* 'curr' is either a closure or the fd is already shutdown */ /* If fd is already shutdown, we are done */ - if ((curr & kShutdownBit) > 0) { + if ((curr & FD_SHUTDOWN_BIT) > 0) { GRPC_ERROR_UNREF(shutdown_err); return false; } @@ -164,7 +176,7 @@ bool LockfreeEvent::SetShutdown(grpc_exec_ctx* exec_ctx, Needs an acquire to pair with setting the closure (and get a happens-after on that edge), and a release to pair with anything loading the shutdown state. */ - if (gpr_atm_full_cas(&state_, curr, new_state)) { + if (gpr_atm_full_cas(state, curr, new_state)) { GRPC_CLOSURE_SCHED(exec_ctx, (grpc_closure*)curr, GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "FD Shutdown", &shutdown_err, 1)); @@ -181,25 +193,26 @@ bool LockfreeEvent::SetShutdown(grpc_exec_ctx* exec_ctx, GPR_UNREACHABLE_CODE(return false); } -void LockfreeEvent::SetReady(grpc_exec_ctx* exec_ctx) { +void grpc_lfev_set_ready(grpc_exec_ctx* exec_ctx, gpr_atm* state, + const char* variable) { while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(&state_); + gpr_atm curr = gpr_atm_no_barrier_load(state); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "LockfreeEvent::SetReady: %p curr=%p", &state_, + gpr_log(GPR_ERROR, "lfev_set_ready[%s]: %p curr=%p", variable, state, (void*)curr); } switch (curr) { - case kClosureReady: { + case CLOSURE_READY: { /* Already ready. We are done here */ return; } - case kClosureNotReady: { + case CLOSURE_NOT_READY: { /* No barrier required as we're transitioning to a state that does not involve a closure */ - if (gpr_atm_no_barrier_cas(&state_, kClosureNotReady, kClosureReady)) { + if (gpr_atm_no_barrier_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { return; /* early out */ } break; /* retry */ @@ -207,14 +220,14 @@ void LockfreeEvent::SetReady(grpc_exec_ctx* exec_ctx) { default: { /* 'curr' is either a closure or the fd is shutdown */ - if ((curr & kShutdownBit) > 0) { + if ((curr & FD_SHUTDOWN_BIT) > 0) { /* The fd is shutdown. Do nothing */ return; } /* Full cas: acquire pairs with this cas' release in the event of a spurious set_ready; release pairs with this or the acquire in notify_on (or set_shutdown) */ - else if (gpr_atm_full_cas(&state_, curr, kClosureNotReady)) { + else if (gpr_atm_full_cas(state, curr, CLOSURE_NOT_READY)) { GRPC_CLOSURE_SCHED(exec_ctx, (grpc_closure*)curr, GRPC_ERROR_NONE); return; } @@ -226,5 +239,3 @@ void LockfreeEvent::SetReady(grpc_exec_ctx* exec_ctx) { } } } - -} // namespace grpc_core diff --git a/src/core/lib/iomgr/lockfree_event.h b/src/core/lib/iomgr/lockfree_event.h index 47d0089c01..75526d6b9f 100644 --- a/src/core/lib/iomgr/lockfree_event.h +++ b/src/core/lib/iomgr/lockfree_event.h @@ -25,30 +25,24 @@ #include "src/core/lib/iomgr/exec_ctx.h" -namespace grpc_core { - -class LockfreeEvent { - public: - LockfreeEvent() = default; - ~LockfreeEvent(); - - LockfreeEvent(const LockfreeEvent&) = delete; - LockfreeEvent& operator=(const LockfreeEvent&) = delete; - - bool IsShutdown() const { - return (gpr_atm_no_barrier_load(&state_) & kShutdownBit) != 0; - } - - void NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure); - bool SetShutdown(grpc_exec_ctx* exec_ctx, grpc_error* error); - void SetReady(grpc_exec_ctx* exec_ctx); - - private: - enum State { kClosureNotReady = 0, kClosureReady = 2, kShutdownBit = 1 }; - - gpr_atm state_ = kClosureNotReady; -}; - -} // namespace grpc_core +#ifdef __cplusplus +extern "C" { +#endif + +void grpc_lfev_init(gpr_atm* state); +void grpc_lfev_destroy(gpr_atm* state); +bool grpc_lfev_is_shutdown(gpr_atm* state); + +void grpc_lfev_notify_on(grpc_exec_ctx* exec_ctx, gpr_atm* state, + grpc_closure* closure, const char* variable); +/* Returns true on first successful shutdown */ +bool grpc_lfev_set_shutdown(grpc_exec_ctx* exec_ctx, gpr_atm* state, + grpc_error* shutdown_err); +void grpc_lfev_set_ready(grpc_exec_ctx* exec_ctx, gpr_atm* state, + const char* variable); + +#ifdef __cplusplus +} +#endif #endif /* GRPC_CORE_LIB_IOMGR_LOCKFREE_EVENT_H */ -- cgit v1.2.3 From f0901946667b05714d6dc99a905506e4715a3a80 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 8 Nov 2017 11:32:01 -0800 Subject: doc: Fully define GetState in connectivity state API --- doc/connectivity-semantics-and-api.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/doc/connectivity-semantics-and-api.md b/doc/connectivity-semantics-and-api.md index 6d39619d65..dc30fe5348 100644 --- a/doc/connectivity-semantics-and-api.md +++ b/doc/connectivity-semantics-and-api.md @@ -115,8 +115,14 @@ Channel State API ----------------- All gRPC libraries will expose a channel-level API method to poll the current -state of a channel. In C++, this method is called GetCurrentState and returns -an enum for one of the five legal states. +state of a channel. In C++, this method is called GetState and returns an enum +for one of the five legal states. It also accepts a boolean `try_to_connect` to +transition to CONNECTING if the channel is currently IDLE. The boolean should +act as if an RPC occurred, so it should also reset IDLE_TIMEOUT. + +```cpp +grpc_connectivity_state GetState(bool try_to_connect); +``` All libraries should also expose an API that enables the application (user of the gRPC API) to be notified when the channel state changes. Since state @@ -127,11 +133,11 @@ the user to poll the channel for the current state. The synchronous version of this API is: ```cpp -bool WaitForStateChange(gpr_timespec deadline, ChannelState source_state); +bool WaitForStateChange(grpc_connectivity_state source_state, gpr_timespec deadline); ``` -which returns true when the state changes to something other than the -source_state and false if the deadline expires. Asynchronous and futures based +which returns `true` when the state is something other than the +`source_state` and `false` if the deadline expires. Asynchronous- and futures-based APIs should have a corresponding method that allows the application to be notified when the state of a channel changes. -- cgit v1.2.3 From fbf61bbc1abe671d9e5b1a8177a6be2abf9d5e23 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 8 Nov 2017 11:50:14 -0800 Subject: Revert "Revert "Class-ify lockfree event"" --- src/core/lib/iomgr/ev_epoll1_linux.cc | 30 ++++----- src/core/lib/iomgr/ev_epollex_linux.cc | 28 ++++---- src/core/lib/iomgr/ev_epollsig_linux.cc | 28 ++++---- src/core/lib/iomgr/lockfree_event.cc | 115 +++++++++++++++----------------- src/core/lib/iomgr/lockfree_event.h | 44 ++++++------ 5 files changed, 120 insertions(+), 125 deletions(-) diff --git a/src/core/lib/iomgr/ev_epoll1_linux.cc b/src/core/lib/iomgr/ev_epoll1_linux.cc index 504c659874..9e3643fa28 100644 --- a/src/core/lib/iomgr/ev_epoll1_linux.cc +++ b/src/core/lib/iomgr/ev_epoll1_linux.cc @@ -46,6 +46,7 @@ #include "src/core/lib/iomgr/lockfree_event.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/manual_constructor.h" #include "src/core/lib/support/string.h" static grpc_wakeup_fd global_wakeup_fd; @@ -111,8 +112,8 @@ static void epoll_set_shutdown() { struct grpc_fd { int fd; - gpr_atm read_closure; - gpr_atm write_closure; + grpc_core::ManualConstructor read_closure; + grpc_core::ManualConstructor write_closure; struct grpc_fd* freelist_next; @@ -264,8 +265,8 @@ static grpc_fd* fd_create(int fd, const char* name) { } new_fd->fd = fd; - grpc_lfev_init(&new_fd->read_closure); - grpc_lfev_init(&new_fd->write_closure); + new_fd->read_closure.Init(); + new_fd->write_closure.Init(); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -297,12 +298,11 @@ static int fd_wrapped_fd(grpc_fd* fd) { return fd->fd; } * shutdown() syscall on that fd) */ static void fd_shutdown_internal(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* why, bool releasing_fd) { - if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, - GRPC_ERROR_REF(why))) { + if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { if (!releasing_fd) { shutdown(fd->fd, SHUT_RDWR); } - grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); + fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } @@ -318,7 +318,7 @@ static void fd_orphan(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* error = GRPC_ERROR_NONE; bool is_release_fd = (release_fd != NULL); - if (!grpc_lfev_is_shutdown(&fd->read_closure)) { + if (!fd->read_closure->IsShutdown()) { fd_shutdown_internal(exec_ctx, fd, GRPC_ERROR_CREATE_FROM_COPIED_STRING(reason), is_release_fd); @@ -335,8 +335,8 @@ static void fd_orphan(grpc_exec_ctx* exec_ctx, grpc_fd* fd, GRPC_CLOSURE_SCHED(exec_ctx, on_done, GRPC_ERROR_REF(error)); grpc_iomgr_unregister_object(&fd->iomgr_object); - grpc_lfev_destroy(&fd->read_closure); - grpc_lfev_destroy(&fd->write_closure); + fd->read_closure.Destroy(); + fd->write_closure.Destroy(); gpr_mu_lock(&fd_freelist_mu); fd->freelist_next = fd_freelist; @@ -351,28 +351,28 @@ static grpc_pollset* fd_get_read_notifier_pollset(grpc_exec_ctx* exec_ctx, } static bool fd_is_shutdown(grpc_fd* fd) { - return grpc_lfev_is_shutdown(&fd->read_closure); + return fd->read_closure->IsShutdown(); } static void fd_notify_on_read(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); + fd->read_closure->NotifyOn(exec_ctx, closure); } static void fd_notify_on_write(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); + fd->write_closure->NotifyOn(exec_ctx, closure); } static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_pollset* notifier) { - grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); + fd->read_closure->SetReady(exec_ctx); /* Use release store to match with acquire load in fd_get_read_notifier */ gpr_atm_rel_store(&fd->read_notifier_pollset, (gpr_atm)notifier); } static void fd_become_writable(grpc_exec_ctx* exec_ctx, grpc_fd* fd) { - grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); + fd->write_closure->SetReady(exec_ctx); } /******************************************************************************* diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index aafdd690c7..26ed1f6747 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -48,6 +48,7 @@ #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/manual_constructor.h" #include "src/core/lib/support/spinlock.h" // debug aid: create workers on the heap (allows asan to spot @@ -153,8 +154,8 @@ struct grpc_fd { gpr_mu pollable_mu; pollable* pollable_obj; - gpr_atm read_closure; - gpr_atm write_closure; + grpc_core::ManualConstructor read_closure; + grpc_core::ManualConstructor write_closure; struct grpc_fd* freelist_next; grpc_closure* on_done_closure; @@ -286,8 +287,8 @@ static void fd_destroy(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { fd->freelist_next = fd_freelist; fd_freelist = fd; - grpc_lfev_destroy(&fd->read_closure); - grpc_lfev_destroy(&fd->write_closure); + fd->read_closure.Destroy(); + fd->write_closure.Destroy(); gpr_mu_unlock(&fd_freelist_mu); } @@ -347,8 +348,8 @@ static grpc_fd* fd_create(int fd, const char* name) { new_fd->pollable_obj = NULL; gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; - grpc_lfev_init(&new_fd->read_closure); - grpc_lfev_init(&new_fd->write_closure); + new_fd->read_closure.Init(); + new_fd->write_closure.Init(); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -411,27 +412,26 @@ static grpc_pollset* fd_get_read_notifier_pollset(grpc_exec_ctx* exec_ctx, } static bool fd_is_shutdown(grpc_fd* fd) { - return grpc_lfev_is_shutdown(&fd->read_closure); + return fd->read_closure->IsShutdown(); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* why) { - if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, - GRPC_ERROR_REF(why))) { + if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { shutdown(fd->fd, SHUT_RDWR); - grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); + fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } static void fd_notify_on_read(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); + fd->read_closure->NotifyOn(exec_ctx, closure); } static void fd_notify_on_write(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); + fd->write_closure->NotifyOn(exec_ctx, closure); } /******************************************************************************* @@ -702,7 +702,7 @@ static int poll_deadline_to_millis_timeout(grpc_exec_ctx* exec_ctx, static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_pollset* notifier) { - grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); + fd->read_closure->SetReady(exec_ctx); /* Note, it is possible that fd_become_readable might be called twice with different 'notifier's when an fd becomes readable and it is in two epoll @@ -714,7 +714,7 @@ static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, } static void fd_become_writable(grpc_exec_ctx* exec_ctx, grpc_fd* fd) { - grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); + fd->write_closure->SetReady(exec_ctx); } static grpc_error* fd_get_or_become_pollable(grpc_fd* fd, pollable** p) { diff --git a/src/core/lib/iomgr/ev_epollsig_linux.cc b/src/core/lib/iomgr/ev_epollsig_linux.cc index d5f3122abc..9a127806fa 100644 --- a/src/core/lib/iomgr/ev_epollsig_linux.cc +++ b/src/core/lib/iomgr/ev_epollsig_linux.cc @@ -50,6 +50,7 @@ #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/iomgr/wakeup_fd_posix.h" #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/manual_constructor.h" #define GRPC_POLLSET_KICK_BROADCAST ((grpc_pollset_worker*)1) @@ -127,8 +128,8 @@ struct grpc_fd { valid */ bool orphaned; - gpr_atm read_closure; - gpr_atm write_closure; + grpc_core::ManualConstructor read_closure; + grpc_core::ManualConstructor write_closure; struct grpc_fd* freelist_next; grpc_closure* on_done_closure; @@ -766,8 +767,8 @@ static void unref_by(grpc_fd* fd, int n) { fd_freelist = fd; grpc_iomgr_unregister_object(&fd->iomgr_object); - grpc_lfev_destroy(&fd->read_closure); - grpc_lfev_destroy(&fd->write_closure); + fd->read_closure.Destroy(); + fd->write_closure.Destroy(); gpr_mu_unlock(&fd_freelist_mu); } else { @@ -832,8 +833,8 @@ static grpc_fd* fd_create(int fd, const char* name) { gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; new_fd->orphaned = false; - grpc_lfev_init(&new_fd->read_closure); - grpc_lfev_init(&new_fd->write_closure); + new_fd->read_closure.Init(); + new_fd->write_closure.Init(); gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; @@ -924,27 +925,26 @@ static grpc_pollset* fd_get_read_notifier_pollset(grpc_exec_ctx* exec_ctx, } static bool fd_is_shutdown(grpc_fd* fd) { - return grpc_lfev_is_shutdown(&fd->read_closure); + return fd->read_closure->IsShutdown(); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_error* why) { - if (grpc_lfev_set_shutdown(exec_ctx, &fd->read_closure, - GRPC_ERROR_REF(why))) { + if (fd->read_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why))) { shutdown(fd->fd, SHUT_RDWR); - grpc_lfev_set_shutdown(exec_ctx, &fd->write_closure, GRPC_ERROR_REF(why)); + fd->write_closure->SetShutdown(exec_ctx, GRPC_ERROR_REF(why)); } GRPC_ERROR_UNREF(why); } static void fd_notify_on_read(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - grpc_lfev_notify_on(exec_ctx, &fd->read_closure, closure, "read"); + fd->read_closure->NotifyOn(exec_ctx, closure); } static void fd_notify_on_write(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_closure* closure) { - grpc_lfev_notify_on(exec_ctx, &fd->write_closure, closure, "write"); + fd->write_closure->NotifyOn(exec_ctx, closure); } /******************************************************************************* @@ -1108,7 +1108,7 @@ static int poll_deadline_to_millis_timeout(grpc_exec_ctx* exec_ctx, static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, grpc_pollset* notifier) { - grpc_lfev_set_ready(exec_ctx, &fd->read_closure, "read"); + fd->read_closure->SetReady(exec_ctx); /* Note, it is possible that fd_become_readable might be called twice with different 'notifier's when an fd becomes readable and it is in two epoll @@ -1120,7 +1120,7 @@ static void fd_become_readable(grpc_exec_ctx* exec_ctx, grpc_fd* fd, } static void fd_become_writable(grpc_exec_ctx* exec_ctx, grpc_fd* fd) { - grpc_lfev_set_ready(exec_ctx, &fd->write_closure, "write"); + fd->write_closure->SetReady(exec_ctx); } static void pollset_release_polling_island(grpc_exec_ctx* exec_ctx, diff --git a/src/core/lib/iomgr/lockfree_event.cc b/src/core/lib/iomgr/lockfree_event.cc index 443a8375b2..98e19f89df 100644 --- a/src/core/lib/iomgr/lockfree_event.cc +++ b/src/core/lib/iomgr/lockfree_event.cc @@ -26,92 +26,79 @@ extern grpc_tracer_flag grpc_polling_trace; /* 'state' holds the to call when the fd is readable or writable respectively. It can contain one of the following values: - CLOSURE_READY : The fd has an I/O event of interest but there is no + kClosureReady : The fd has an I/O event of interest but there is no closure yet to execute - CLOSURE_NOT_READY : The fd has no I/O event of interest + kClosureNotReady : The fd has no I/O event of interest closure ptr : The closure to be executed when the fd has an I/O event of interest - shutdown_error | FD_SHUTDOWN_BIT : - 'shutdown_error' field ORed with FD_SHUTDOWN_BIT. + shutdown_error | kShutdownBit : + 'shutdown_error' field ORed with kShutdownBit. This indicates that the fd is shutdown. Since all memory allocations are word-aligned, the lower two bits of the shutdown_error pointer are always 0. So - it is safe to OR these with FD_SHUTDOWN_BIT + it is safe to OR these with kShutdownBit Valid state transitions: - <-----3------ CLOSURE_NOT_READY ----1----> CLOSURE_READY + <-----3------ kClosureNotReady -----1-------> kClosureReady | | ^ | ^ | | | | | | | | | | +--------------4----------+ 6 +---------2---------------+ | | | | | v | - +-----5-------> [shutdown_error | FD_SHUTDOWN_BIT] <----7---------+ + +-----5-------> [shutdown_error | kShutdownBit] <-------7---------+ - For 1, 4 : See grpc_lfev_set_ready() function - For 2, 3 : See grpc_lfev_notify_on() function - For 5,6,7: See grpc_lfev_set_shutdown() function */ + For 1, 4 : See SetReady() function + For 2, 3 : See NotifyOn() function + For 5,6,7: See SetShutdown() function */ -#define CLOSURE_NOT_READY ((gpr_atm)0) -#define CLOSURE_READY ((gpr_atm)2) +namespace grpc_core { -#define FD_SHUTDOWN_BIT ((gpr_atm)1) - -void grpc_lfev_init(gpr_atm* state) { - gpr_atm_no_barrier_store(state, CLOSURE_NOT_READY); -} - -void grpc_lfev_destroy(gpr_atm* state) { - gpr_atm curr = gpr_atm_no_barrier_load(state); - if (curr & FD_SHUTDOWN_BIT) { - GRPC_ERROR_UNREF((grpc_error*)(curr & ~FD_SHUTDOWN_BIT)); +LockfreeEvent::~LockfreeEvent() { + gpr_atm curr = gpr_atm_no_barrier_load(&state_); + if (curr & kShutdownBit) { + GRPC_ERROR_UNREF((grpc_error*)(curr & ~kShutdownBit)); } else { - GPR_ASSERT(curr == CLOSURE_NOT_READY || curr == CLOSURE_READY); + GPR_ASSERT(curr == kClosureNotReady || curr == kClosureReady); } } -bool grpc_lfev_is_shutdown(gpr_atm* state) { - gpr_atm curr = gpr_atm_no_barrier_load(state); - return (curr & FD_SHUTDOWN_BIT) != 0; -} - -void grpc_lfev_notify_on(grpc_exec_ctx* exec_ctx, gpr_atm* state, - grpc_closure* closure, const char* variable) { +void LockfreeEvent::NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure) { while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(state); + gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_notify_on[%s]: %p curr=%p closure=%p", variable, - state, (void*)curr, closure); + gpr_log(GPR_ERROR, "LockfreeEvent::NotifyOn: %p curr=%p closure=%p", this, + (void*)curr, closure); } switch (curr) { - case CLOSURE_NOT_READY: { - /* CLOSURE_NOT_READY -> . + case kClosureNotReady: { + /* kClosureNotReady -> . We're guaranteed by API that there's an acquire barrier before here, so there's no need to double-dip and this can be a release-only. The release itself pairs with the acquire half of a set_ready full barrier. */ - if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { + if (gpr_atm_rel_cas(&state_, kClosureNotReady, (gpr_atm)closure)) { return; /* Successful. Return */ } break; /* retry */ } - case CLOSURE_READY: { - /* Change the state to CLOSURE_NOT_READY. Schedule the closure if + case kClosureReady: { + /* Change the state to kClosureNotReady. Schedule the closure if successful. If not, the state most likely transitioned to shutdown. We should retry. This can be a no-barrier cas since the state is being transitioned to - CLOSURE_NOT_READY; set_ready and set_shutdown do not schedule any + kClosureNotReady; set_ready and set_shutdown do not schedule any closure when transitioning out of CLOSURE_NO_READY state (i.e there is no other code that needs to 'happen-after' this) */ - if (gpr_atm_no_barrier_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { + if (gpr_atm_no_barrier_cas(&state_, kClosureReady, kClosureNotReady)) { GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_NONE); return; /* Successful. Return */ } @@ -123,8 +110,8 @@ void grpc_lfev_notify_on(grpc_exec_ctx* exec_ctx, gpr_atm* state, /* 'curr' is either a closure or the fd is shutdown(in which case 'curr' contains a pointer to the shutdown-error). If the fd is shutdown, schedule the closure with the shutdown error */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { - grpc_error* shutdown_err = (grpc_error*)(curr & ~FD_SHUTDOWN_BIT); + if ((curr & kShutdownBit) > 0) { + grpc_error* shutdown_err = (grpc_error*)(curr & ~kShutdownBit); GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "FD Shutdown", &shutdown_err, 1)); @@ -133,7 +120,8 @@ void grpc_lfev_notify_on(grpc_exec_ctx* exec_ctx, gpr_atm* state, /* There is already a closure!. This indicates a bug in the code */ gpr_log(GPR_ERROR, - "notify_on called with a previous callback still pending"); + "LockfreeEvent::NotifyOn: notify_on called with a previous " + "callback still pending"); abort(); } } @@ -142,22 +130,22 @@ void grpc_lfev_notify_on(grpc_exec_ctx* exec_ctx, gpr_atm* state, GPR_UNREACHABLE_CODE(return ); } -bool grpc_lfev_set_shutdown(grpc_exec_ctx* exec_ctx, gpr_atm* state, - grpc_error* shutdown_err) { - gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; +bool LockfreeEvent::SetShutdown(grpc_exec_ctx* exec_ctx, + grpc_error* shutdown_err) { + gpr_atm new_state = (gpr_atm)shutdown_err | kShutdownBit; while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(state); + gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_set_shutdown: %p curr=%p err=%s", state, - (void*)curr, grpc_error_string(shutdown_err)); + gpr_log(GPR_ERROR, "LockfreeEvent::SetShutdown: %p curr=%p err=%s", + &state_, (void*)curr, grpc_error_string(shutdown_err)); } switch (curr) { - case CLOSURE_READY: - case CLOSURE_NOT_READY: + case kClosureReady: + case kClosureNotReady: /* Need a full barrier here so that the initial load in notify_on doesn't need a barrier */ - if (gpr_atm_full_cas(state, curr, new_state)) { + if (gpr_atm_full_cas(&state_, curr, new_state)) { return true; /* early out */ } break; /* retry */ @@ -166,7 +154,7 @@ bool grpc_lfev_set_shutdown(grpc_exec_ctx* exec_ctx, gpr_atm* state, /* 'curr' is either a closure or the fd is already shutdown */ /* If fd is already shutdown, we are done */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { + if ((curr & kShutdownBit) > 0) { GRPC_ERROR_UNREF(shutdown_err); return false; } @@ -176,7 +164,7 @@ bool grpc_lfev_set_shutdown(grpc_exec_ctx* exec_ctx, gpr_atm* state, Needs an acquire to pair with setting the closure (and get a happens-after on that edge), and a release to pair with anything loading the shutdown state. */ - if (gpr_atm_full_cas(state, curr, new_state)) { + if (gpr_atm_full_cas(&state_, curr, new_state)) { GRPC_CLOSURE_SCHED(exec_ctx, (grpc_closure*)curr, GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "FD Shutdown", &shutdown_err, 1)); @@ -193,26 +181,25 @@ bool grpc_lfev_set_shutdown(grpc_exec_ctx* exec_ctx, gpr_atm* state, GPR_UNREACHABLE_CODE(return false); } -void grpc_lfev_set_ready(grpc_exec_ctx* exec_ctx, gpr_atm* state, - const char* variable) { +void LockfreeEvent::SetReady(grpc_exec_ctx* exec_ctx) { while (true) { - gpr_atm curr = gpr_atm_no_barrier_load(state); + gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (GRPC_TRACER_ON(grpc_polling_trace)) { - gpr_log(GPR_ERROR, "lfev_set_ready[%s]: %p curr=%p", variable, state, + gpr_log(GPR_ERROR, "LockfreeEvent::SetReady: %p curr=%p", &state_, (void*)curr); } switch (curr) { - case CLOSURE_READY: { + case kClosureReady: { /* Already ready. We are done here */ return; } - case CLOSURE_NOT_READY: { + case kClosureNotReady: { /* No barrier required as we're transitioning to a state that does not involve a closure */ - if (gpr_atm_no_barrier_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { + if (gpr_atm_no_barrier_cas(&state_, kClosureNotReady, kClosureReady)) { return; /* early out */ } break; /* retry */ @@ -220,14 +207,14 @@ void grpc_lfev_set_ready(grpc_exec_ctx* exec_ctx, gpr_atm* state, default: { /* 'curr' is either a closure or the fd is shutdown */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { + if ((curr & kShutdownBit) > 0) { /* The fd is shutdown. Do nothing */ return; } /* Full cas: acquire pairs with this cas' release in the event of a spurious set_ready; release pairs with this or the acquire in notify_on (or set_shutdown) */ - else if (gpr_atm_full_cas(state, curr, CLOSURE_NOT_READY)) { + else if (gpr_atm_full_cas(&state_, curr, kClosureNotReady)) { GRPC_CLOSURE_SCHED(exec_ctx, (grpc_closure*)curr, GRPC_ERROR_NONE); return; } @@ -239,3 +226,5 @@ void grpc_lfev_set_ready(grpc_exec_ctx* exec_ctx, gpr_atm* state, } } } + +} // namespace grpc_core diff --git a/src/core/lib/iomgr/lockfree_event.h b/src/core/lib/iomgr/lockfree_event.h index 75526d6b9f..47d0089c01 100644 --- a/src/core/lib/iomgr/lockfree_event.h +++ b/src/core/lib/iomgr/lockfree_event.h @@ -25,24 +25,30 @@ #include "src/core/lib/iomgr/exec_ctx.h" -#ifdef __cplusplus -extern "C" { -#endif - -void grpc_lfev_init(gpr_atm* state); -void grpc_lfev_destroy(gpr_atm* state); -bool grpc_lfev_is_shutdown(gpr_atm* state); - -void grpc_lfev_notify_on(grpc_exec_ctx* exec_ctx, gpr_atm* state, - grpc_closure* closure, const char* variable); -/* Returns true on first successful shutdown */ -bool grpc_lfev_set_shutdown(grpc_exec_ctx* exec_ctx, gpr_atm* state, - grpc_error* shutdown_err); -void grpc_lfev_set_ready(grpc_exec_ctx* exec_ctx, gpr_atm* state, - const char* variable); - -#ifdef __cplusplus -} -#endif +namespace grpc_core { + +class LockfreeEvent { + public: + LockfreeEvent() = default; + ~LockfreeEvent(); + + LockfreeEvent(const LockfreeEvent&) = delete; + LockfreeEvent& operator=(const LockfreeEvent&) = delete; + + bool IsShutdown() const { + return (gpr_atm_no_barrier_load(&state_) & kShutdownBit) != 0; + } + + void NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure); + bool SetShutdown(grpc_exec_ctx* exec_ctx, grpc_error* error); + void SetReady(grpc_exec_ctx* exec_ctx); + + private: + enum State { kClosureNotReady = 0, kClosureReady = 2, kShutdownBit = 1 }; + + gpr_atm state_ = kClosureNotReady; +}; + +} // namespace grpc_core #endif /* GRPC_CORE_LIB_IOMGR_LOCKFREE_EVENT_H */ -- cgit v1.2.3 From 15626bb5274d4f0a65d9328af536b46f3a15e18b Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 8 Nov 2017 12:03:22 -0800 Subject: Fix data race --- src/core/lib/iomgr/lockfree_event.cc | 4 ++++ src/core/lib/iomgr/lockfree_event.h | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/core/lib/iomgr/lockfree_event.cc b/src/core/lib/iomgr/lockfree_event.cc index 98e19f89df..af1bee4694 100644 --- a/src/core/lib/iomgr/lockfree_event.cc +++ b/src/core/lib/iomgr/lockfree_event.cc @@ -57,6 +57,10 @@ extern grpc_tracer_flag grpc_polling_trace; namespace grpc_core { +LockfreeEvent::LockfreeEvent() { + gpr_atm_no_barrier_store(&state_, kClosureNotReady); +} + LockfreeEvent::~LockfreeEvent() { gpr_atm curr = gpr_atm_no_barrier_load(&state_); if (curr & kShutdownBit) { diff --git a/src/core/lib/iomgr/lockfree_event.h b/src/core/lib/iomgr/lockfree_event.h index 47d0089c01..c667dcd3bc 100644 --- a/src/core/lib/iomgr/lockfree_event.h +++ b/src/core/lib/iomgr/lockfree_event.h @@ -29,7 +29,7 @@ namespace grpc_core { class LockfreeEvent { public: - LockfreeEvent() = default; + LockfreeEvent(); ~LockfreeEvent(); LockfreeEvent(const LockfreeEvent&) = delete; @@ -46,7 +46,7 @@ class LockfreeEvent { private: enum State { kClosureNotReady = 0, kClosureReady = 2, kShutdownBit = 1 }; - gpr_atm state_ = kClosureNotReady; + gpr_atm state_; }; } // namespace grpc_core -- cgit v1.2.3 From 1ef989cd50cdfb172e99c552a67e3ed5f350e5cc Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 8 Nov 2017 12:08:33 -0800 Subject: Fixes, commentary --- src/core/lib/iomgr/lockfree_event.cc | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/core/lib/iomgr/lockfree_event.cc b/src/core/lib/iomgr/lockfree_event.cc index af1bee4694..40e2ed6219 100644 --- a/src/core/lib/iomgr/lockfree_event.cc +++ b/src/core/lib/iomgr/lockfree_event.cc @@ -58,16 +58,29 @@ extern grpc_tracer_flag grpc_polling_trace; namespace grpc_core { LockfreeEvent::LockfreeEvent() { + /* Perform an atomic store to start the state machine. + + Note carefully that LockfreeEvent *MAY* be used whilst in a destroyed + state, while a file descriptor is on a freelist. In such a state it may + be SetReady'd, and so we need to perform an atomic operation here to + ensure no races */ gpr_atm_no_barrier_store(&state_, kClosureNotReady); } LockfreeEvent::~LockfreeEvent() { - gpr_atm curr = gpr_atm_no_barrier_load(&state_); - if (curr & kShutdownBit) { - GRPC_ERROR_UNREF((grpc_error*)(curr & ~kShutdownBit)); - } else { - GPR_ASSERT(curr == kClosureNotReady || curr == kClosureReady); - } + gpr_atm curr; + do { + curr = gpr_atm_no_barrier_load(&state_); + if (curr & kShutdownBit) { + GRPC_ERROR_UNREF((grpc_error*)(curr & ~kShutdownBit)); + } else { + GPR_ASSERT(curr == kClosureNotReady || curr == kClosureReady); + } + /* we CAS in a shutdown, no error value here. If this event is interacted + with post-deletion (see the note in the constructor) we want the bit + pattern to prevent error retention in a deleted object */ + } while (!gpr_atm_no_barrier_cas(&state_, curr, + kShutdownBit /* shutdown, no error */)); } void LockfreeEvent::NotifyOn(grpc_exec_ctx* exec_ctx, grpc_closure* closure) { -- cgit v1.2.3 From c6f925d9224422e4dba02ed1dd00a5797c20f4cd Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 8 Nov 2017 13:39:19 -0800 Subject: Fix timer freed early in uv pollset --- src/core/lib/iomgr/pollset_uv.cc | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/core/lib/iomgr/pollset_uv.cc b/src/core/lib/iomgr/pollset_uv.cc index 6b9c53c01c..48d9d2ace0 100644 --- a/src/core/lib/iomgr/pollset_uv.cc +++ b/src/core/lib/iomgr/pollset_uv.cc @@ -40,7 +40,7 @@ grpc_tracer_flag grpc_trace_fd_refcount = #endif struct grpc_pollset { - uv_timer_t timer; + uv_timer_t *timer; int shutting_down; }; @@ -78,12 +78,16 @@ void grpc_pollset_global_shutdown(void) { static void timer_run_cb(uv_timer_t* timer) {} -static void timer_close_cb(uv_handle_t* handle) { handle->data = (void*)1; } +static void timer_close_cb(uv_handle_t* handle) { + handle->data = (void*)1; + gpr_free(handle); +} void grpc_pollset_init(grpc_pollset* pollset, gpr_mu** mu) { GRPC_UV_ASSERT_SAME_THREAD(); *mu = &grpc_polling_mu; - uv_timer_init(uv_default_loop(), &pollset->timer); + pollset->timer = (uv_timer_t*)gpr_malloc(sizeof(uv_timer_t)); + uv_timer_init(uv_default_loop(), pollset->timer); pollset->shutting_down = 0; } @@ -104,11 +108,11 @@ void grpc_pollset_shutdown(grpc_exec_ctx* exec_ctx, grpc_pollset* pollset, void grpc_pollset_destroy(grpc_exec_ctx* exec_ctx, grpc_pollset* pollset) { GRPC_UV_ASSERT_SAME_THREAD(); - uv_close((uv_handle_t*)&pollset->timer, timer_close_cb); + uv_close((uv_handle_t*)pollset->timer, timer_close_cb); // timer.data is a boolean indicating that the timer has finished closing - pollset->timer.data = (void*)0; + pollset->timer->data = (void*)0; if (grpc_pollset_work_run_loop) { - while (!pollset->timer.data) { + while (!pollset->timer->data) { uv_run(uv_default_loop(), UV_RUN_NOWAIT); } } @@ -130,11 +134,11 @@ grpc_error* grpc_pollset_work(grpc_exec_ctx* exec_ctx, grpc_pollset* pollset, /* We special-case timeout=0 so that we don't bother with the timer when the loop won't block anyway */ if (timeout > 0) { - uv_timer_start(&pollset->timer, timer_run_cb, timeout, 0); + uv_timer_start(pollset->timer, timer_run_cb, timeout, 0); /* Run until there is some I/O activity or the timer triggers. It doesn't matter which happens */ uv_run(uv_default_loop(), UV_RUN_ONCE); - uv_timer_stop(&pollset->timer); + uv_timer_stop(pollset->timer); } else { uv_run(uv_default_loop(), UV_RUN_NOWAIT); } -- cgit v1.2.3 From 096f883e5e4e2b76c2892d5c29a8ad48dc12dab1 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 8 Nov 2017 14:30:41 -0800 Subject: clang format --- src/core/ext/filters/client_channel/backup_poller.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/filters/client_channel/backup_poller.cc b/src/core/ext/filters/client_channel/backup_poller.cc index 45b2464e27..c3795c35c1 100644 --- a/src/core/ext/filters/client_channel/backup_poller.cc +++ b/src/core/ext/filters/client_channel/backup_poller.cc @@ -149,7 +149,7 @@ void grpc_client_channel_start_backup_polling( * TSAN happy. Otherwise, reading from g_poller (i.e g_poller->pollset) after * releasing the lock and setting g_poller to NULL in g_poller_unref() is * being flagged as a data-race by TSAN */ - grpc_pollset *pollset = g_poller->pollset; + grpc_pollset* pollset = g_poller->pollset; gpr_mu_unlock(&g_poller_mu); grpc_pollset_set_add_pollset(exec_ctx, interested_parties, pollset); -- cgit v1.2.3 From b6727b6f70c40b9f9362745650dd7e86ba74a7f8 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 8 Nov 2017 15:18:12 -0800 Subject: clang format --- src/core/ext/transport/chttp2/transport/internal.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 11c7bbec23..60cc280c43 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -308,7 +308,7 @@ struct grpc_chttp2_transport { /** Set to a grpc_error object if a goaway frame is received. By default, set * to GRPC_ERROR_NONE */ - grpc_error *goaway_error; + grpc_error* goaway_error; grpc_chttp2_sent_goaway_state sent_goaway_state; @@ -377,7 +377,7 @@ struct grpc_chttp2_transport { grpc_chttp2_transport* t, grpc_chttp2_stream* s, grpc_slice slice, int is_last); - grpc_chttp2_write_cb *write_cb_pool; + grpc_chttp2_write_cb* write_cb_pool; /* bdp estimator */ grpc_closure next_bdp_ping_timer_expired_locked; -- cgit v1.2.3 From 70e87c3dee2184c38426015e6c393f2d57a5154b Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 8 Nov 2017 15:37:13 -0800 Subject: Clang format --- src/core/lib/iomgr/pollset_uv.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lib/iomgr/pollset_uv.cc b/src/core/lib/iomgr/pollset_uv.cc index 48d9d2ace0..1d54942c1d 100644 --- a/src/core/lib/iomgr/pollset_uv.cc +++ b/src/core/lib/iomgr/pollset_uv.cc @@ -40,7 +40,7 @@ grpc_tracer_flag grpc_trace_fd_refcount = #endif struct grpc_pollset { - uv_timer_t *timer; + uv_timer_t* timer; int shutting_down; }; -- cgit v1.2.3 From dcd970d86bd8a7d6bcd33834087e7a04e3b57740 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Thu, 9 Nov 2017 14:05:00 +0100 Subject: artifact builder dockerfiles were removed by mistake --- .../dockerfile/grpc_artifact_linux_x64/Dockerfile | 88 ++++++++++++++++++++++ .../dockerfile/grpc_artifact_linux_x86/Dockerfile | 73 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 tools/dockerfile/grpc_artifact_linux_x64/Dockerfile create mode 100644 tools/dockerfile/grpc_artifact_linux_x86/Dockerfile diff --git a/tools/dockerfile/grpc_artifact_linux_x64/Dockerfile b/tools/dockerfile/grpc_artifact_linux_x64/Dockerfile new file mode 100644 index 0000000000..0251b2b392 --- /dev/null +++ b/tools/dockerfile/grpc_artifact_linux_x64/Dockerfile @@ -0,0 +1,88 @@ +# Copyright 2016 gRPC authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Docker file for building gRPC artifacts. + +FROM debian:jessie + +RUN apt-get update && apt-get install debian-keyring && apt-key update + +# Install Git and basic packages. +RUN apt-get update && apt-key update && apt-get install -y \ + autoconf \ + autotools-dev \ + build-essential \ + bzip2 \ + clang \ + curl \ + gcc \ + gcc-multilib \ + git \ + golang \ + libc6 \ + libc6-dbg \ + libc6-dev \ + libgtest-dev \ + libtool \ + make \ + perl \ + strace \ + python-dev \ + python-setuptools \ + python-yaml \ + telnet \ + unzip \ + wget \ + zip && apt-get clean + +# Install Node dependencies +RUN touch .profile +RUN curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.25.4/install.sh | bash +RUN /bin/bash -l -c "nvm install 8 && npm install -g node-pre-gyp" + + +################## +# Ruby dependencies + +# Install rvm +RUN gpg --keyserver hkp://keys.gnupg.net --recv-keys 409B6B1796C275462A1703113804BB82D39DC0E3 +RUN \curl -sSL https://get.rvm.io | bash -s stable + +# Install Ruby 2.1 +RUN /bin/bash -l -c "rvm install ruby-2.1" +RUN /bin/bash -l -c "rvm use --default ruby-2.1" +RUN /bin/bash -l -c "echo 'gem: --no-ri --no-rdoc' > ~/.gemrc" +RUN /bin/bash -l -c "echo 'export PATH=/usr/local/rvm/bin:$PATH' >> ~/.bashrc" +RUN /bin/bash -l -c "echo 'rvm --default use ruby-2.1' >> ~/.bashrc" +RUN /bin/bash -l -c "gem install bundler --no-ri --no-rdoc" + + +################## +# PHP dependencies + +RUN apt-get update && apt-get install -y \ + php5 php5-dev php-pear phpunit + +################## +# Install cross compiler for ARM + +RUN echo 'deb http://emdebian.org/tools/debian/ jessie main' | tee -a /etc/apt/sources.list.d/crosstools.list && \ + curl http://emdebian.org/tools/debian/emdebian-toolchain-archive.key | apt-key add - + +RUN dpkg --add-architecture armhf && apt-get update && apt-get install -y crossbuild-essential-armhf + +RUN mkdir /var/local/jenkins + +# Define the default command. +CMD ["bash"] diff --git a/tools/dockerfile/grpc_artifact_linux_x86/Dockerfile b/tools/dockerfile/grpc_artifact_linux_x86/Dockerfile new file mode 100644 index 0000000000..2d179c8c45 --- /dev/null +++ b/tools/dockerfile/grpc_artifact_linux_x86/Dockerfile @@ -0,0 +1,73 @@ +# Copyright 2016 gRPC authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Docker file for building gRPC artifacts. + +FROM 32bit/debian:jessie + +RUN apt-get update && apt-get install debian-keyring && apt-key update + +# Install Git and basic packages. +RUN apt-get update && apt-key update && apt-get install -y \ + autoconf \ + autotools-dev \ + build-essential \ + bzip2 \ + clang \ + curl \ + gcc \ + gcc-multilib \ + git \ + golang \ + libc6 \ + libc6-dbg \ + libc6-dev \ + libgtest-dev \ + libtool \ + make \ + perl \ + strace \ + python-dev \ + python-setuptools \ + python-yaml \ + telnet \ + unzip \ + wget \ + zip && apt-get clean + +# Install Node dependencies +RUN touch .profile +RUN curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.25.4/install.sh | bash +RUN /bin/bash -l -c "nvm install 8 && npm install -g node-pre-gyp" + + +################## +# Ruby dependencies + +# Install rvm +RUN gpg --keyserver hkp://keys.gnupg.net --recv-keys 409B6B1796C275462A1703113804BB82D39DC0E3 +RUN \curl -sSL https://get.rvm.io | bash -s stable + +# Install Ruby 2.1 +RUN /bin/bash -l -c "rvm install ruby-2.1" +RUN /bin/bash -l -c "rvm use --default ruby-2.1" +RUN /bin/bash -l -c "echo 'gem: --no-ri --no-rdoc' > ~/.gemrc" +RUN /bin/bash -l -c "echo 'export PATH=/usr/local/rvm/bin:$PATH' >> ~/.bashrc" +RUN /bin/bash -l -c "echo 'rvm --default use ruby-2.1' >> ~/.bashrc" +RUN /bin/bash -l -c "gem install bundler --no-ri --no-rdoc" + +RUN mkdir /var/local/jenkins + +# Define the default command. +CMD ["bash"] -- cgit v1.2.3 From c42e97800468659f2f9a88425df26e8eb8ad8aba Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Thu, 9 Nov 2017 08:35:21 -0800 Subject: Reduce # of arithmetic ops to reduce time spent in this test. --- test/core/support/cpu_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/support/cpu_test.cc b/test/core/support/cpu_test.cc index a76531ead9..c69ce55a73 100644 --- a/test/core/support/cpu_test.cc +++ b/test/core/support/cpu_test.cc @@ -68,7 +68,7 @@ static void worker_thread(void* arg) { unsigned i, j; /* Avoid repetitive division calculations */ int64_t max_i = 1000 / grpc_test_slowdown_factor(); - int64_t max_j = 1000000 / grpc_test_slowdown_factor(); + int64_t max_j = 1000 / grpc_test_slowdown_factor(); for (i = 0; i < max_i; i++) { /* run for a bit - just calculate something random. */ for (j = 0; j < max_j; j++) { -- cgit v1.2.3 From a6746340550ef6504ce1ecbc86256a6e2af4f21e Mon Sep 17 00:00:00 2001 From: Whitney Jackson Date: Mon, 30 Oct 2017 23:35:05 -0600 Subject: Handle frame with priority flag for canceled stream --- src/core/ext/transport/chttp2/transport/parsing.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 8a3774d688..6737c26e72 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -590,7 +590,11 @@ static grpc_error* init_header_frame_parser(grpc_exec_ctx* exec_ctx, GRPC_CHTTP2_IF_TRACING(gpr_log( GPR_ERROR, "ignoring new grpc_chttp2_stream creation on client")); } - return init_skip_frame_parser(exec_ctx, t, 1); + grpc_error* err = init_skip_frame_parser(exec_ctx, t, 1); + if (t->incoming_frame_flags & GRPC_CHTTP2_FLAG_HAS_PRIORITY) { + grpc_chttp2_hpack_parser_set_has_priority(&t->hpack_parser); + } + return err; } else if (t->last_new_stream_id >= t->incoming_stream_id) { GRPC_CHTTP2_IF_TRACING(gpr_log( GPR_ERROR, -- cgit v1.2.3