From 82d73414e3acd6c8a1bb9908cf7c071d58a22497 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 9 Feb 2017 18:27:45 -0800 Subject: closures -> atomics --- src/core/lib/iomgr/ev_epoll_linux.c | 241 +++++++++++++++++++++++------------- 1 file changed, 155 insertions(+), 86 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 51842fc208..c60ff85898 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -68,7 +68,7 @@ static int grpc_polling_trace = 0; /* Disabled by default */ gpr_log(GPR_INFO, (fmt), __VA_ARGS__); \ } -/* Uncomment the following enable extra checks on poll_object operations */ +/* Uncomment the following to enable extra checks on poll_object operations */ /* #define PO_DEBUG */ static int grpc_wakeup_signal = -1; @@ -141,23 +141,26 @@ struct grpc_fd { gpr_atm refst; /* Indicates that the fd is shutdown and that any pending read/write closures - should fail */ - bool shutdown; - grpc_error *shutdown_error; /* reason for shutdown: set iff shutdown==true */ + should fail. */ + // TODO: sreek storing bool and grpc_error* + gpr_atm shutdown1; + gpr_atm shutdown_error1; /* reason for shutdown: set iff shutdown==true */ /* The fd is either closed or we relinquished control of it. In either cases, this indicates that the 'fd' on this structure is no longer valid */ bool orphaned; - /* TODO: sreek - Move this to a lockfree implementation */ - grpc_closure *read_closure; - grpc_closure *write_closure; + /* Closures to call when the fd is readable or writable. The actual type + stored in these is (grpc_closure *) */ + gpr_atm read_closure; + gpr_atm write_closure; struct grpc_fd *freelist_next; grpc_closure *on_done_closure; - /* The pollset that last noticed that the fd is readable */ - grpc_pollset *read_notifier_pollset; + /* The pollset that last noticed that the fd is readable. The actual type + * stored in this is (grpc_pollset *) */ + gpr_atm read_notifier_pollset; grpc_iomgr_object iomgr_object; }; @@ -180,8 +183,8 @@ static void fd_unref(grpc_fd *fd); static void fd_global_init(void); static void fd_global_shutdown(void); -#define CLOSURE_NOT_READY ((grpc_closure *)0) -#define CLOSURE_READY ((grpc_closure *)1) +#define CLOSURE_NOT_READY ((gpr_atm)0) +#define CLOSURE_READY ((gpr_atm)1) /******************************************************************************* * Polling island Declarations @@ -908,7 +911,12 @@ static void unref_by(grpc_fd *fd, int n) { fd->freelist_next = fd_freelist; fd_freelist = fd; grpc_iomgr_unregister_object(&fd->iomgr_object); - if (fd->shutdown) GRPC_ERROR_UNREF(fd->shutdown_error); + + if ((bool)gpr_atm_acq_load(&fd->shutdown1)) { + grpc_error *err = + (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); + GRPC_ERROR_UNREF(err); + } gpr_mu_unlock(&fd_freelist_mu); } else { @@ -972,13 +980,15 @@ 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->shutdown = false; + gpr_atm_rel_store(&new_fd->shutdown1, (gpr_atm) false); + gpr_atm_rel_store(&new_fd->shutdown_error1, (gpr_atm)GRPC_ERROR_NONE); new_fd->orphaned = false; - new_fd->read_closure = CLOSURE_NOT_READY; - new_fd->write_closure = CLOSURE_NOT_READY; + gpr_atm_rel_store(&new_fd->read_closure, CLOSURE_NOT_READY); + gpr_atm_rel_store(&new_fd->write_closure, CLOSURE_NOT_READY); + gpr_atm_rel_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); + new_fd->freelist_next = NULL; new_fd->on_done_closure = NULL; - new_fd->read_notifier_pollset = NULL; gpr_mu_unlock(&new_fd->po.mu); @@ -1061,100 +1071,159 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, } static grpc_error *fd_shutdown_error(grpc_fd *fd) { + grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); + if (err != GRPC_ERROR_NONE) { + err = GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &err, 1); + } + + return err; + + /* TODO sreek - delete this */ + /* if (!fd->shutdown) { return GRPC_ERROR_NONE; } else { return GRPC_ERROR_CREATE_REFERENCING("FD shutdown", &fd->shutdown_error, 1); } -} + */ +} + +static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, + grpc_closure *closure) { + bool is_done = false; + while (!is_done) { + is_done = true; + if (!gpr_atm_acq_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { + // CAS failed because the current value of 'state' is not + // 'CLOSURE_NOT_READY' + gpr_atm curr = gpr_atm_acq_load(state); + + switch (curr) { + case CLOSURE_NOT_READY: { + // The CAS above failed because the state was not 'CLOSURE_NOT_READY' + // but it seems to be back to 'CLOSURE_NOT_READY'. Lets retry CAS + // again + is_done = false; + break; + } -static void notify_on_locked(grpc_exec_ctx *exec_ctx, grpc_fd *fd, - grpc_closure **st, grpc_closure *closure) { - if (fd->shutdown) { - grpc_closure_sched(exec_ctx, closure, GRPC_ERROR_CREATE("FD shutdown")); - } else if (*st == CLOSURE_NOT_READY) { - /* not ready ==> switch to a waiting state by setting the closure */ - *st = closure; - } else if (*st == CLOSURE_READY) { - /* already ready ==> queue the closure to run immediately */ - *st = CLOSURE_NOT_READY; - grpc_closure_sched(exec_ctx, closure, fd_shutdown_error(fd)); - } else { - /* upcallptr was set to a different closure. This is an error! */ - gpr_log(GPR_ERROR, - "User called a notify_on function with a previous callback still " - "pending"); - abort(); + case CLOSURE_READY: { + // Change the state to CLOSURE_NOT_READY and if successful, schedule + // the closure + if (gpr_atm_rel_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { + grpc_closure_sched(exec_ctx, closure, fd_shutdown_error(fd)); + } else { + // Looks like the current state is not CLOSURE_READY anymore. Retry + // from the beginning + is_done = false; + } + } + + default: { + // The current state already contains a closure. This is a fatal error + gpr_log( + GPR_ERROR, + "User called notify_on function with a previous callback still " + "pending"); + abort(); + break; + } + } + } } } -/* returns 1 if state becomes not ready */ -static int set_ready_locked(grpc_exec_ctx *exec_ctx, grpc_fd *fd, - grpc_closure **st) { - if (*st == CLOSURE_READY) { - /* duplicate ready ==> ignore */ - return 0; - } else if (*st == CLOSURE_NOT_READY) { - /* not ready, and not waiting ==> flag ready */ - *st = CLOSURE_READY; - return 0; - } else { - /* waiting ==> queue closure */ - grpc_closure_sched(exec_ctx, *st, fd_shutdown_error(fd)); - *st = CLOSURE_NOT_READY; - return 1; - } +static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { + /* Try the fast-path first (i.e expect current value to be CLOSURE_NOT_READY + * and then try to change it to CLOSURE_READY) */ + if (!gpr_atm_acq_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { + /* CAS failed since the current state is not CLOSURE_NOT_READY. Find out + what is the current state */ + gpr_atm curr = gpr_atm_acq_load(state); + switch (curr) { + case CLOSURE_READY: { + /* Already ready. We are done here */ + break; + } + + case CLOSURE_NOT_READY: { + /* The state was not CLOSURE_NOT_READY when we checked initially at the + beginning of this function but now it is CLOSURE_NOT_READY. This is + only possible if the state transitioned out of CLOSURE_NOT_READY to + either CLOSURE_READY or and then back to + CLOSURE_NOT_READY again. So there is no need to make the state + CLOSURE_READY now */ + break; + } + + default: { + /* 'curr' is a closure. This closure should be enqueued and the current + state should be changed to CLOSURE_NOT_READY */ + if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { + grpc_closure_sched(exec_ctx, (grpc_closure *)*state, + fd_shutdown_error(fd)); + } /* else the state changed again. This can only happen due to another + racing set_ready function (which means, we do not have to do + anything else here */ + break; + } + } + } /* else fast-path succeeded. We are done */ } static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, grpc_fd *fd) { - grpc_pollset *notifier = NULL; - - gpr_mu_lock(&fd->po.mu); - notifier = fd->read_notifier_pollset; - gpr_mu_unlock(&fd->po.mu); - - return notifier; + gpr_atm notifier = gpr_atm_no_barrier_load(&fd->read_closure); + return (grpc_pollset *)notifier; } static bool fd_is_shutdown(grpc_fd *fd) { - gpr_mu_lock(&fd->po.mu); - const bool r = fd->shutdown; - gpr_mu_unlock(&fd->po.mu); - return r; + return (bool)gpr_atm_acq_load(&fd->shutdown1); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { - gpr_mu_lock(&fd->po.mu); - /* Do the actual shutdown only once */ - if (!fd->shutdown) { - fd->shutdown = true; - fd->shutdown_error = why; + if (gpr_atm_acq_cas(&fd->shutdown1, (gpr_atm) false, (gpr_atm) true)) { + gpr_atm_rel_store(&fd->shutdown_error1, (gpr_atm)why); shutdown(fd->fd, SHUT_RDWR); - /* Flush any pending read and write closures. Since fd->shutdown is 'true' - at this point, the closures would be called with 'success = false' */ - set_ready_locked(exec_ctx, fd, &fd->read_closure); - set_ready_locked(exec_ctx, fd, &fd->write_closure); + + /* Flush any pending read and write closures at this point. Since + fd->shutdown_error1 is set, both the closures would be called with + success = false */ + set_ready(exec_ctx, fd, &fd->read_closure); + set_ready(exec_ctx, fd, &fd->write_closure); + } else { + // Shutdown already called GRPC_ERROR_UNREF(why); } - gpr_mu_unlock(&fd->po.mu); + + // gpr_mu_lock(&fd->po.mu); + /* Do the actual shutdown only once */ + // if (!fd->shutdown) { + // fd->shutdown = true; + // fd->shutdown_error = why; + + // shutdown(fd->fd, SHUT_RDWR); + /* Flush any pending read and write closures. Since fd->shutdown is 'true' + at this point, the closures would be called with 'success = false' */ + // set_ready(exec_ctx, fd, &fd->read_closure); + // set_ready(exec_ctx, fd, &fd->write_closure); + // } else { + // GRPC_ERROR_UNREF(why); + // } + // gpr_mu_unlock(&fd->po.mu); } static void fd_notify_on_read(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - gpr_mu_lock(&fd->po.mu); - notify_on_locked(exec_ctx, fd, &fd->read_closure, closure); - gpr_mu_unlock(&fd->po.mu); + notify_on(exec_ctx, fd, &fd->read_closure, closure); } static void fd_notify_on_write(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *closure) { - gpr_mu_lock(&fd->po.mu); - notify_on_locked(exec_ctx, fd, &fd->write_closure, closure); - gpr_mu_unlock(&fd->po.mu); + notify_on(exec_ctx, fd, &fd->write_closure, closure); } static grpc_workqueue *fd_get_workqueue(grpc_fd *fd) { @@ -1343,18 +1412,18 @@ static int poll_deadline_to_millis_timeout(gpr_timespec deadline, static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_pollset *notifier) { - /* Need the fd->po.mu since we might be racing with fd_notify_on_read */ - gpr_mu_lock(&fd->po.mu); - set_ready_locked(exec_ctx, fd, &fd->read_closure); - fd->read_notifier_pollset = notifier; - gpr_mu_unlock(&fd->po.mu); + set_ready(exec_ctx, fd, &fd->read_closure); + + // 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 + // sets (This can happen briefly during polling island merges). In such cases + // it does not really matter which notifer is set as the read_notifier_pollset + // (They would both point to the same polling island anyway) + gpr_atm_no_barrier_store(&fd->read_notifier_pollset, (gpr_atm)notifier); } static void fd_become_writable(grpc_exec_ctx *exec_ctx, grpc_fd *fd) { - /* Need the fd->po.mu since we might be racing with fd_notify_on_write */ - gpr_mu_lock(&fd->po.mu); - set_ready_locked(exec_ctx, fd, &fd->write_closure); - gpr_mu_unlock(&fd->po.mu); + set_ready(exec_ctx, fd, &fd->write_closure); } static void pollset_release_polling_island(grpc_exec_ctx *exec_ctx, -- cgit v1.2.3 From 8b8cbed345ba1dccb17fef9f388a24fbcb5cdf96 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 9 Feb 2017 21:31:27 -0800 Subject: remove fd->shutdown --- src/core/lib/iomgr/error.c | 4 +++- src/core/lib/iomgr/error.h | 2 ++ src/core/lib/iomgr/ev_epoll_linux.c | 48 ++++++++++++++++--------------------- 3 files changed, 26 insertions(+), 28 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index dbe5b139f9..68cadb70d1 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -164,7 +164,7 @@ static const char *error_time_name(grpc_error_times key) { bool grpc_error_is_special(grpc_error *err) { return err == GRPC_ERROR_NONE || err == GRPC_ERROR_OOM || - err == GRPC_ERROR_CANCELLED; + err == GRPC_ERROR_CANCELLED || err == GRPC_ERROR_INTERNAL; } #ifdef GRPC_ERROR_REFCOUNT_DEBUG @@ -260,6 +260,8 @@ static grpc_error *copy_error_and_unref(grpc_error *in) { out = grpc_error_set_int(GRPC_ERROR_CREATE("cancelled"), GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_CANCELLED); + else if (in == GRPC_ERROR_INTERNAL) + out = GRPC_ERROR_CREATE("internal"); else out = GRPC_ERROR_CREATE("unknown"); } else { diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index ffacdac393..87d7344596 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -136,6 +136,7 @@ typedef enum { GRPC_ERROR_TIME_CREATED, } grpc_error_times; + /// The following "special" errors can be propagated without allocating memory. /// They are always even so that other code (particularly combiner locks) can /// safely use the lower bit for themselves. @@ -143,6 +144,7 @@ typedef enum { #define GRPC_ERROR_NONE ((grpc_error *)NULL) #define GRPC_ERROR_OOM ((grpc_error *)2) #define GRPC_ERROR_CANCELLED ((grpc_error *)4) +#define GRPC_ERROR_INTERNAL ((grpc_error *)8) const char *grpc_error_string(grpc_error *error); diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index c60ff85898..1b3607c255 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -140,11 +140,11 @@ struct grpc_fd { Ref/Unref by two to avoid altering the orphaned bit */ gpr_atm refst; - /* Indicates that the fd is shutdown and that any pending read/write closures - should fail. */ - // TODO: sreek storing bool and grpc_error* - gpr_atm shutdown1; - gpr_atm shutdown_error1; /* reason for shutdown: set iff shutdown==true */ + /* Internally stores data of type (grpc_error *). If the value is anything + other than GRPC_ERROR_NONE, it indicates that the fd is shutdown and this + contains the reason for shutdown. Once an fd is shutdown, any pending or + future read/write closures on the fd should fail */ + gpr_atm shutdown_error1; /* The fd is either closed or we relinquished control of it. In either cases, this indicates that the 'fd' on this structure is no longer valid */ @@ -185,6 +185,7 @@ static void fd_global_shutdown(void); #define CLOSURE_NOT_READY ((gpr_atm)0) #define CLOSURE_READY ((gpr_atm)1) +#define CLOSURE_SHUTDOWN ((gpr_atm)2) /******************************************************************************* * Polling island Declarations @@ -912,11 +913,8 @@ static void unref_by(grpc_fd *fd, int n) { fd_freelist = fd; grpc_iomgr_unregister_object(&fd->iomgr_object); - if ((bool)gpr_atm_acq_load(&fd->shutdown1)) { - grpc_error *err = - (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); - GRPC_ERROR_UNREF(err); - } + grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); + GRPC_ERROR_UNREF(err); gpr_mu_unlock(&fd_freelist_mu); } else { @@ -980,7 +978,6 @@ static grpc_fd *fd_create(int fd, const char *name) { gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; - gpr_atm_rel_store(&new_fd->shutdown1, (gpr_atm) false); gpr_atm_rel_store(&new_fd->shutdown_error1, (gpr_atm)GRPC_ERROR_NONE); new_fd->orphaned = false; gpr_atm_rel_store(&new_fd->read_closure, CLOSURE_NOT_READY); @@ -1077,15 +1074,6 @@ static grpc_error *fd_shutdown_error(grpc_fd *fd) { } return err; - - /* TODO sreek - delete this */ - /* - if (!fd->shutdown) { - return GRPC_ERROR_NONE; - } else { - return GRPC_ERROR_CREATE_REFERENCING("FD shutdown", &fd->shutdown_error, 1); - } - */ } static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, @@ -1137,8 +1125,7 @@ static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { /* Try the fast-path first (i.e expect current value to be CLOSURE_NOT_READY * and then try to change it to CLOSURE_READY) */ if (!gpr_atm_acq_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { - /* CAS failed since the current state is not CLOSURE_NOT_READY. Find out - what is the current state */ + /* Fallback to slowpath */ gpr_atm curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_READY: { @@ -1151,8 +1138,9 @@ static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { beginning of this function but now it is CLOSURE_NOT_READY. This is only possible if the state transitioned out of CLOSURE_NOT_READY to either CLOSURE_READY or and then back to - CLOSURE_NOT_READY again. So there is no need to make the state - CLOSURE_READY now */ + CLOSURE_NOT_READY again (i.e after we entered this function, the fd + became "ready" and the necessary actions were already done). So there + is no need to make the state CLOSURE_READY now */ break; } @@ -1178,14 +1166,20 @@ static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, } static bool fd_is_shutdown(grpc_fd *fd) { - return (bool)gpr_atm_acq_load(&fd->shutdown1); + grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); + return (err != GRPC_ERROR_NONE); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { - if (gpr_atm_acq_cas(&fd->shutdown1, (gpr_atm) false, (gpr_atm) true)) { - gpr_atm_rel_store(&fd->shutdown_error1, (gpr_atm)why); + /* If 'why' is GRPC_ERROR_NONE, change it to something else so that we know + that the fd is shutdown just by looking at fd->shutdown_error */ + if (why == GRPC_ERROR_NONE) { + why = GRPC_ERROR_INTERNAL; + } + if (gpr_atm_acq_cas(&fd->shutdown_error1, (gpr_atm)GRPC_ERROR_NONE, + (gpr_atm)why)) { shutdown(fd->fd, SHUT_RDWR); /* Flush any pending read and write closures at this point. Since -- cgit v1.2.3 From 91c4da322396a6f7b4ab980f05ab2fae5b889a0b Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sun, 12 Feb 2017 14:00:39 -0800 Subject: Add shutdown-state --- src/core/lib/iomgr/ev_epoll_linux.c | 138 +++++++++++++++++++++++------------- 1 file changed, 90 insertions(+), 48 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 1b3607c255..04d078177c 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -185,6 +185,7 @@ static void fd_global_shutdown(void); #define CLOSURE_NOT_READY ((gpr_atm)0) #define CLOSURE_READY ((gpr_atm)1) + #define CLOSURE_SHUTDOWN ((gpr_atm)2) /******************************************************************************* @@ -1067,53 +1068,97 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, GRPC_ERROR_UNREF(error); } -static grpc_error *fd_shutdown_error(grpc_fd *fd) { - grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); - if (err != GRPC_ERROR_NONE) { - err = GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &err, 1); - } - - return err; -} - static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, grpc_closure *closure) { bool is_done = false; while (!is_done) { is_done = true; + /* Fast-path: CLOSURE_NOT_READY -> */ if (!gpr_atm_acq_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { - // CAS failed because the current value of 'state' is not - // 'CLOSURE_NOT_READY' + /* Fallback to slowpath */ gpr_atm curr = gpr_atm_acq_load(state); - switch (curr) { case CLOSURE_NOT_READY: { - // The CAS above failed because the state was not 'CLOSURE_NOT_READY' - // but it seems to be back to 'CLOSURE_NOT_READY'. Lets retry CAS - // again is_done = false; break; } case CLOSURE_READY: { - // Change the state to CLOSURE_NOT_READY and if successful, schedule - // the closure + /* Change the state to CLOSURE_NOT_READY and if successful, schedule + the closure */ if (gpr_atm_rel_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { - grpc_closure_sched(exec_ctx, closure, fd_shutdown_error(fd)); + grpc_closure_sched(exec_ctx, closure, GRPC_ERROR_NONE); } else { - // Looks like the current state is not CLOSURE_READY anymore. Retry - // from the beginning + /* Looks like the current state is not CLOSURE_READY anymore. Most + likely it transitioned to CLOSURE_NOT_READY. Retry the fast-path + again */ is_done = false; } + break; + } + + default: { + /* 'curr' is either a closure or the fd is shutdown (in which case + * 'curr' contains a pointer to the shutdown-error) */ + if ((curr & CLOSURE_SHUTDOWN) > 0) { + /* FD is shutdown. Schedule the closure with the shutdown error */ + grpc_error *shutdown_err = (grpc_error *)(curr & ~CLOSURE_SHUTDOWN); + grpc_closure_sched( + exec_ctx, closure, + GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); + + } else { + /* There is already a closure!. This indicates a bug in the code */ + gpr_log( + GPR_ERROR, + "User called notify_on function with a previous callback still " + "pending"); + abort(); + } + break; + } + } + } + } +} + +static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, + grpc_error *shutdown_err) { + /* Try the fast-path first (i.e expect the current value to be + CLOSURE_NOT_READY */ + gpr_atm curr = CLOSURE_NOT_READY; + gpr_atm new_state = (gpr_atm)shutdown_err | CLOSURE_SHUTDOWN; + + bool is_done = false; + while (!is_done) { + is_done = true; + if (!gpr_atm_acq_cas(state, curr, new_state)) { + /* Fallback to slowpath */ + curr = gpr_atm_acq_load(state); + switch (curr) { + case CLOSURE_READY: { + is_done = false; + break; + } + + case CLOSURE_NOT_READY: { + is_done = false; + break; } default: { - // The current state already contains a closure. This is a fatal error - gpr_log( - GPR_ERROR, - "User called notify_on function with a previous callback still " - "pending"); - abort(); + /* 'curr' is either a closure or the fd is already shutdown */ + if ((curr & CLOSURE_SHUTDOWN) > 0) { + /* fd is already shutdown. Do nothing */ + } else if (gpr_atm_rel_cas(state, curr, new_state)) { + grpc_closure_sched( + exec_ctx, (grpc_closure *)curr, + GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); + } else { + /* 'curr' was a closure but now changed to a different state. We + will have to retry */ + is_done = false; + } break; } } @@ -1122,8 +1167,8 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, } static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { - /* Try the fast-path first (i.e expect current value to be CLOSURE_NOT_READY - * and then try to change it to CLOSURE_READY) */ + /* Try the fast-path first (i.e expect the current value to be + * CLOSURE_NOT_READY */ if (!gpr_atm_acq_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { /* Fallback to slowpath */ gpr_atm curr = gpr_atm_acq_load(state); @@ -1135,24 +1180,25 @@ static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { case CLOSURE_NOT_READY: { /* The state was not CLOSURE_NOT_READY when we checked initially at the - beginning of this function but now it is CLOSURE_NOT_READY. This is - only possible if the state transitioned out of CLOSURE_NOT_READY to - either CLOSURE_READY or and then back to - CLOSURE_NOT_READY again (i.e after we entered this function, the fd - became "ready" and the necessary actions were already done). So there - is no need to make the state CLOSURE_READY now */ + beginning of this function but now it is CLOSURE_NOT_READY again. + This is only possible if the state transitioned out of + CLOSURE_NOT_READY to either CLOSURE_READY or and then + back to CLOSURE_NOT_READY again (i.e after we entered this function, + the fd became "ready" and the necessary actions were already done). + So there is no need to make the state CLOSURE_READY now */ break; } default: { - /* 'curr' is a closure. This closure should be enqueued and the current - state should be changed to CLOSURE_NOT_READY */ - if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { - grpc_closure_sched(exec_ctx, (grpc_closure *)*state, - fd_shutdown_error(fd)); - } /* else the state changed again. This can only happen due to another - racing set_ready function (which means, we do not have to do - anything else here */ + /* 'curr' is either a closure or the fd is shutdown */ + if ((curr & CLOSURE_SHUTDOWN) > 0) { + /* The fd is shutdown. Do nothing */ + } else if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { + grpc_closure_sched(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_NONE); + } + /* else the state changed again (only possible by either a racing + set_ready or set_shutdown functions. In both these cases, the closure + would have been scheduled for execution. So we are done here */ break; } } @@ -1182,12 +1228,8 @@ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { (gpr_atm)why)) { shutdown(fd->fd, SHUT_RDWR); - /* Flush any pending read and write closures at this point. Since - fd->shutdown_error1 is set, both the closures would be called with - success = false */ - set_ready(exec_ctx, fd, &fd->read_closure); - set_ready(exec_ctx, fd, &fd->write_closure); - + set_shutdown(exec_ctx, fd, &fd->read_closure, why); + set_shutdown(exec_ctx, fd, &fd->write_closure, why); } else { // Shutdown already called GRPC_ERROR_UNREF(why); -- cgit v1.2.3 From 9998338cad330d2a5a6fc9f2725e419d96717b5d Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sun, 12 Feb 2017 17:03:27 -0800 Subject: More comments --- src/core/lib/iomgr/ev_epoll_linux.c | 59 ++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 24 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 04d078177c..cc7beecba8 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -144,14 +144,42 @@ struct grpc_fd { other than GRPC_ERROR_NONE, it indicates that the fd is shutdown and this contains the reason for shutdown. Once an fd is shutdown, any pending or future read/write closures on the fd should fail */ - gpr_atm shutdown_error1; + gpr_atm shutdown_error; - /* The fd is either closed or we relinquished control of it. In either cases, - this indicates that the 'fd' on this structure is no longer valid */ + /* The fd is either closed or we relinquished control of it. In either + cases, this indicates that the 'fd' on this structure is no longer + valid */ bool orphaned; - /* Closures to call when the fd is readable or writable. The actual type - stored in these is (grpc_closure *) */ + /* Closures to call when the fd is readable or writable respectively. These + fields contain one of the following values: + CLOSURE_READY : 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 + + closure ptr : The closure to be executed when the fd has an I/O event + of interest. + shutdown_error | + CLOSURE_SHUTDOWN : 'shutdown_error' field OR'ed with CLOSURE_SHUTDOWN. + This indicates that the fd is shutdown. Since all + memory allocations are word-aligned, the lower to + bits of the shutdown_error pointer are always 0. So + it is safe to OR these with CLOSURE_SHUTDOWN. + + Valid state transitions: + + <-----3------ CLOSURE_NOT_READY ----1----> CLOSURE_READY + | | ^ | ^ | | + | | | | | | | + | +--------------4----------+ 6 +---------2---------------+ | + | | | + | v | + +-----5-------> [shutdown_error | CLOSURE_SHUTDOWN] <--------7----+ + + For 1, 4 : See set_ready() function + For 2, 3 : See notify_on() function + For 5,6,7: See set_shutdown() function */ gpr_atm read_closure; gpr_atm write_closure; @@ -185,7 +213,6 @@ static void fd_global_shutdown(void); #define CLOSURE_NOT_READY ((gpr_atm)0) #define CLOSURE_READY ((gpr_atm)1) - #define CLOSURE_SHUTDOWN ((gpr_atm)2) /******************************************************************************* @@ -1212,7 +1239,7 @@ static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, } static bool fd_is_shutdown(grpc_fd *fd) { - grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); + grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error); return (err != GRPC_ERROR_NONE); } @@ -1224,7 +1251,7 @@ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { why = GRPC_ERROR_INTERNAL; } - if (gpr_atm_acq_cas(&fd->shutdown_error1, (gpr_atm)GRPC_ERROR_NONE, + if (gpr_atm_acq_cas(&fd->shutdown_error, (gpr_atm)GRPC_ERROR_NONE, (gpr_atm)why)) { shutdown(fd->fd, SHUT_RDWR); @@ -1234,22 +1261,6 @@ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { // Shutdown already called GRPC_ERROR_UNREF(why); } - - // gpr_mu_lock(&fd->po.mu); - /* Do the actual shutdown only once */ - // if (!fd->shutdown) { - // fd->shutdown = true; - // fd->shutdown_error = why; - - // shutdown(fd->fd, SHUT_RDWR); - /* Flush any pending read and write closures. Since fd->shutdown is 'true' - at this point, the closures would be called with 'success = false' */ - // set_ready(exec_ctx, fd, &fd->read_closure); - // set_ready(exec_ctx, fd, &fd->write_closure); - // } else { - // GRPC_ERROR_UNREF(why); - // } - // gpr_mu_unlock(&fd->po.mu); } static void fd_notify_on_read(grpc_exec_ctx *exec_ctx, grpc_fd *fd, -- cgit v1.2.3 From 4db8c82e1411e65da735d25501c783e2b3b7fb4b Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sun, 12 Feb 2017 17:07:31 -0800 Subject: Fix comment style --- src/core/lib/iomgr/ev_epoll_linux.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index cc7beecba8..b896b94c37 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -1258,7 +1258,7 @@ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { set_shutdown(exec_ctx, fd, &fd->read_closure, why); set_shutdown(exec_ctx, fd, &fd->write_closure, why); } else { - // Shutdown already called + /* Shutdown already called */ GRPC_ERROR_UNREF(why); } } @@ -1461,7 +1461,7 @@ static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_pollset *notifier) { set_ready(exec_ctx, fd, &fd->read_closure); - // Note, it is possible that fd_become_readable might be called twice with + /* 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 // sets (This can happen briefly during polling island merges). In such cases // it does not really matter which notifer is set as the read_notifier_pollset -- cgit v1.2.3 From 4c60d0dc23c402c0e1b13a67a95736ec84965d53 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sun, 12 Feb 2017 17:09:08 -0800 Subject: fix comment style --- src/core/lib/iomgr/ev_epoll_linux.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index b896b94c37..680d2b05c8 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -1462,10 +1462,10 @@ static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, set_ready(exec_ctx, fd, &fd->read_closure); /* 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 - // sets (This can happen briefly during polling island merges). In such cases - // it does not really matter which notifer is set as the read_notifier_pollset - // (They would both point to the same polling island anyway) + different 'notifier's when an fd becomes readable and it is in two epoll + sets (This can happen briefly during polling island merges). In such cases + it does not really matter which notifer is set as the read_notifier_pollset + (They would both point to the same polling island anyway) */ gpr_atm_no_barrier_store(&fd->read_notifier_pollset, (gpr_atm)notifier); } -- cgit v1.2.3 From a70ccb6a0070996d1e54f78669911fb893e21268 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Mon, 13 Feb 2017 23:16:52 -0800 Subject: code review comments --- src/core/lib/iomgr/ev_epoll_linux.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 680d2b05c8..8a0d9a0a0e 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -158,14 +158,15 @@ struct grpc_fd { 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 | - CLOSURE_SHUTDOWN : 'shutdown_error' field OR'ed with CLOSURE_SHUTDOWN. - This indicates that the fd is shutdown. Since all - memory allocations are word-aligned, the lower to - bits of the shutdown_error pointer are always 0. So - it is safe to OR these with CLOSURE_SHUTDOWN. + closure ptr : The closure to be executed when the fd has an I/O + event of interest + + shutdown_error | CLOSURE_SHUTDOWN_BIT : + 'shutdown_error' field ORed with CLOSURE_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 CLOSURE_SHUTDOWN_BIT Valid state transitions: @@ -175,7 +176,7 @@ struct grpc_fd { | +--------------4----------+ 6 +---------2---------------+ | | | | | v | - +-----5-------> [shutdown_error | CLOSURE_SHUTDOWN] <--------7----+ + +-----5-------> [shutdown_error | CLOSURE_SHUTDOWN_BIT] <----7----+ For 1, 4 : See set_ready() function For 2, 3 : See notify_on() function @@ -213,7 +214,8 @@ static void fd_global_shutdown(void); #define CLOSURE_NOT_READY ((gpr_atm)0) #define CLOSURE_READY ((gpr_atm)1) -#define CLOSURE_SHUTDOWN ((gpr_atm)2) + +#define CLOSURE_SHUTDOWN_BIT 1 /******************************************************************************* * Polling island Declarations @@ -941,7 +943,7 @@ static void unref_by(grpc_fd *fd, int n) { fd_freelist = fd; grpc_iomgr_unregister_object(&fd->iomgr_object); - grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error1); + grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error); GRPC_ERROR_UNREF(err); gpr_mu_unlock(&fd_freelist_mu); @@ -1006,7 +1008,7 @@ static grpc_fd *fd_create(int fd, const char *name) { gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; - gpr_atm_rel_store(&new_fd->shutdown_error1, (gpr_atm)GRPC_ERROR_NONE); + gpr_atm_rel_store(&new_fd->shutdown_error, (gpr_atm)GRPC_ERROR_NONE); new_fd->orphaned = false; gpr_atm_rel_store(&new_fd->read_closure, CLOSURE_NOT_READY); gpr_atm_rel_store(&new_fd->write_closure, CLOSURE_NOT_READY); @@ -1127,9 +1129,10 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, default: { /* 'curr' is either a closure or the fd is shutdown (in which case * 'curr' contains a pointer to the shutdown-error) */ - if ((curr & CLOSURE_SHUTDOWN) > 0) { + if ((curr & CLOSURE_SHUTDOWN_BIT) > 0) { /* FD is shutdown. Schedule the closure with the shutdown error */ - grpc_error *shutdown_err = (grpc_error *)(curr & ~CLOSURE_SHUTDOWN); + grpc_error *shutdown_err = + (grpc_error *)(curr & ~CLOSURE_SHUTDOWN_BIT); grpc_closure_sched( exec_ctx, closure, GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); @@ -1154,7 +1157,7 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, /* Try the fast-path first (i.e expect the current value to be CLOSURE_NOT_READY */ gpr_atm curr = CLOSURE_NOT_READY; - gpr_atm new_state = (gpr_atm)shutdown_err | CLOSURE_SHUTDOWN; + gpr_atm new_state = (gpr_atm)shutdown_err | CLOSURE_SHUTDOWN_BIT; bool is_done = false; while (!is_done) { @@ -1175,7 +1178,7 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, default: { /* 'curr' is either a closure or the fd is already shutdown */ - if ((curr & CLOSURE_SHUTDOWN) > 0) { + if ((curr & CLOSURE_SHUTDOWN_BIT) > 0) { /* fd is already shutdown. Do nothing */ } else if (gpr_atm_rel_cas(state, curr, new_state)) { grpc_closure_sched( @@ -1218,7 +1221,7 @@ static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { default: { /* 'curr' is either a closure or the fd is shutdown */ - if ((curr & CLOSURE_SHUTDOWN) > 0) { + if ((curr & CLOSURE_SHUTDOWN_BIT) > 0) { /* The fd is shutdown. Do nothing */ } else if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { grpc_closure_sched(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_NONE); -- cgit v1.2.3 From 2fc2b3ed14071cf8285d5a9a780538cad314252f Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 14 Feb 2017 10:05:14 -0800 Subject: Remove GRPC_ERROR_INTERNAL type and simplify the code a bit --- src/core/lib/iomgr/error.c | 4 +--- src/core/lib/iomgr/error.h | 5 ++--- src/core/lib/iomgr/ev_epoll_linux.c | 44 ++++++++++++++++++------------------- 3 files changed, 25 insertions(+), 28 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index 68cadb70d1..dbe5b139f9 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -164,7 +164,7 @@ static const char *error_time_name(grpc_error_times key) { bool grpc_error_is_special(grpc_error *err) { return err == GRPC_ERROR_NONE || err == GRPC_ERROR_OOM || - err == GRPC_ERROR_CANCELLED || err == GRPC_ERROR_INTERNAL; + err == GRPC_ERROR_CANCELLED; } #ifdef GRPC_ERROR_REFCOUNT_DEBUG @@ -260,8 +260,6 @@ static grpc_error *copy_error_and_unref(grpc_error *in) { out = grpc_error_set_int(GRPC_ERROR_CREATE("cancelled"), GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_CANCELLED); - else if (in == GRPC_ERROR_INTERNAL) - out = GRPC_ERROR_CREATE("internal"); else out = GRPC_ERROR_CREATE("unknown"); } else { diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index 87d7344596..8393605d77 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -138,13 +138,12 @@ typedef enum { /// The following "special" errors can be propagated without allocating memory. -/// They are always even so that other code (particularly combiner locks) can -/// safely use the lower bit for themselves. +/// They are always even so that other code (particularly combiner locks, +/// polling engines) can safely use the lower bit for themselves. #define GRPC_ERROR_NONE ((grpc_error *)NULL) #define GRPC_ERROR_OOM ((grpc_error *)2) #define GRPC_ERROR_CANCELLED ((grpc_error *)4) -#define GRPC_ERROR_INTERNAL ((grpc_error *)8) const char *grpc_error_string(grpc_error *error); diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 8a0d9a0a0e..c7c893e35a 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -140,10 +140,15 @@ struct grpc_fd { Ref/Unref by two to avoid altering the orphaned bit */ gpr_atm refst; - /* Internally stores data of type (grpc_error *). If the value is anything - other than GRPC_ERROR_NONE, it indicates that the fd is shutdown and this - contains the reason for shutdown. Once an fd is shutdown, any pending or - future read/write closures on the fd should fail */ + /* Internally stores data of type (grpc_error *). If the FD is shutdown, this + contains reason for shutdown (i.e a pointer to grpc_error) ORed with + FD_SHUTDOWN_BIT. Since address allocations are word-aligned, the lower bit + of (grpc_error *) addresses is guaranteed to be zero. Even if the + (grpc_error *), is of special types like GRPC_ERROR_NONE, GRPC_ERROR_OOM + etc, the lower bit is guaranteed to be zero. + + Once an fd is shutdown, any pending or future read/write closures on the + fd should fail */ gpr_atm shutdown_error; /* The fd is either closed or we relinquished control of it. In either @@ -161,12 +166,12 @@ struct grpc_fd { closure ptr : The closure to be executed when the fd has an I/O event of interest - shutdown_error | CLOSURE_SHUTDOWN_BIT : - 'shutdown_error' field ORed with CLOSURE_SHUTDOWN_BIT. + 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 CLOSURE_SHUTDOWN_BIT + it is safe to OR these with FD_SHUTDOWN_BIT Valid state transitions: @@ -176,7 +181,7 @@ struct grpc_fd { | +--------------4----------+ 6 +---------2---------------+ | | | | | v | - +-----5-------> [shutdown_error | CLOSURE_SHUTDOWN_BIT] <----7----+ + +-----5-------> [shutdown_error | FD_SHUTDOWN_BIT] <----7---------+ For 1, 4 : See set_ready() function For 2, 3 : See notify_on() function @@ -215,7 +220,7 @@ static void fd_global_shutdown(void); #define CLOSURE_NOT_READY ((gpr_atm)0) #define CLOSURE_READY ((gpr_atm)1) -#define CLOSURE_SHUTDOWN_BIT 1 +#define FD_SHUTDOWN_BIT 1 /******************************************************************************* * Polling island Declarations @@ -1129,10 +1134,10 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, default: { /* 'curr' is either a closure or the fd is shutdown (in which case * 'curr' contains a pointer to the shutdown-error) */ - if ((curr & CLOSURE_SHUTDOWN_BIT) > 0) { + if ((curr & FD_SHUTDOWN_BIT) > 0) { /* FD is shutdown. Schedule the closure with the shutdown error */ grpc_error *shutdown_err = - (grpc_error *)(curr & ~CLOSURE_SHUTDOWN_BIT); + (grpc_error *)(curr & ~FD_SHUTDOWN_BIT); grpc_closure_sched( exec_ctx, closure, GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); @@ -1157,7 +1162,7 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, /* Try the fast-path first (i.e expect the current value to be CLOSURE_NOT_READY */ gpr_atm curr = CLOSURE_NOT_READY; - gpr_atm new_state = (gpr_atm)shutdown_err | CLOSURE_SHUTDOWN_BIT; + gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; bool is_done = false; while (!is_done) { @@ -1178,7 +1183,7 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, default: { /* 'curr' is either a closure or the fd is already shutdown */ - if ((curr & CLOSURE_SHUTDOWN_BIT) > 0) { + if ((curr & FD_SHUTDOWN_BIT) > 0) { /* fd is already shutdown. Do nothing */ } else if (gpr_atm_rel_cas(state, curr, new_state)) { grpc_closure_sched( @@ -1221,7 +1226,7 @@ static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { default: { /* 'curr' is either a closure or the fd is shutdown */ - if ((curr & CLOSURE_SHUTDOWN_BIT) > 0) { + if ((curr & FD_SHUTDOWN_BIT) > 0) { /* The fd is shutdown. Do nothing */ } else if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { grpc_closure_sched(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_NONE); @@ -1243,19 +1248,14 @@ static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, static bool fd_is_shutdown(grpc_fd *fd) { grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error); - return (err != GRPC_ERROR_NONE); + return (((intptr_t) err & FD_SHUTDOWN_BIT) > 0); } /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { - /* If 'why' is GRPC_ERROR_NONE, change it to something else so that we know - that the fd is shutdown just by looking at fd->shutdown_error */ - if (why == GRPC_ERROR_NONE) { - why = GRPC_ERROR_INTERNAL; - } - + /* Store the shutdown error ORed with FD_SHUTDOWN_BIT in fd->shutdown_error */ if (gpr_atm_acq_cas(&fd->shutdown_error, (gpr_atm)GRPC_ERROR_NONE, - (gpr_atm)why)) { + (gpr_atm)why | FD_SHUTDOWN_BIT)) { shutdown(fd->fd, SHUT_RDWR); set_shutdown(exec_ctx, fd, &fd->read_closure, why); -- cgit v1.2.3 From 61fe0944966c459b32ecc9da0b06dd8689e1d69f Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 14 Feb 2017 12:26:56 -0800 Subject: address more code review comments --- src/core/lib/iomgr/ev_epoll_linux.c | 217 ++++++++++++++++++------------------ 1 file changed, 111 insertions(+), 106 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index c7c893e35a..108cf5a613 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -949,6 +949,8 @@ static void unref_by(grpc_fd *fd, int n) { grpc_iomgr_unregister_object(&fd->iomgr_object); grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error); + /* Clear the least significant bit if it set (in case fd was shutdown) */ + err = (grpc_error *)((intptr_t)err & ~FD_SHUTDOWN_BIT); GRPC_ERROR_UNREF(err); gpr_mu_unlock(&fd_freelist_mu); @@ -1013,11 +1015,11 @@ static grpc_fd *fd_create(int fd, const char *name) { gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; - gpr_atm_rel_store(&new_fd->shutdown_error, (gpr_atm)GRPC_ERROR_NONE); + gpr_atm_no_barrier_store(&new_fd->shutdown_error, (gpr_atm)GRPC_ERROR_NONE); new_fd->orphaned = false; - gpr_atm_rel_store(&new_fd->read_closure, CLOSURE_NOT_READY); - gpr_atm_rel_store(&new_fd->write_closure, CLOSURE_NOT_READY); - gpr_atm_rel_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); + gpr_atm_no_barrier_store(&new_fd->read_closure, CLOSURE_NOT_READY); + gpr_atm_no_barrier_store(&new_fd->write_closure, CLOSURE_NOT_READY); + gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); new_fd->freelist_next = NULL; new_fd->on_done_closure = NULL; @@ -1104,57 +1106,53 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, grpc_closure *closure) { - bool is_done = false; - while (!is_done) { - is_done = true; + while (true) { /* Fast-path: CLOSURE_NOT_READY -> */ - if (!gpr_atm_acq_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { - /* Fallback to slowpath */ - gpr_atm curr = gpr_atm_acq_load(state); - switch (curr) { - case CLOSURE_NOT_READY: { - is_done = false; - break; - } + if (gpr_atm_acq_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { + return; /* Fast-path successful. Return */ + } - case CLOSURE_READY: { - /* Change the state to CLOSURE_NOT_READY and if successful, schedule - the closure */ - if (gpr_atm_rel_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { - grpc_closure_sched(exec_ctx, closure, GRPC_ERROR_NONE); - } else { - /* Looks like the current state is not CLOSURE_READY anymore. Most - likely it transitioned to CLOSURE_NOT_READY. Retry the fast-path - again */ - is_done = false; - } - break; + /* Slowpath */ + gpr_atm curr = gpr_atm_acq_load(state); + switch (curr) { + case CLOSURE_NOT_READY: { + break; /* retry */ + } + + case CLOSURE_READY: { + /* Change the state to CLOSURE_NOT_READY. + If successful: Schedule the closure + If not: Most likely the state transitioned to CLOSURE_NOT_READY. + Retry the fast-path again */ + if (gpr_atm_rel_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { + grpc_closure_sched(exec_ctx, closure, GRPC_ERROR_NONE); + return; /* Slow-path successful. Return */ } - default: { - /* 'curr' is either a closure or the fd is shutdown (in which case - * 'curr' contains a pointer to the shutdown-error) */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { - /* FD is shutdown. Schedule the closure with the shutdown error */ - grpc_error *shutdown_err = - (grpc_error *)(curr & ~FD_SHUTDOWN_BIT); - grpc_closure_sched( - exec_ctx, closure, - GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); - - } else { - /* There is already a closure!. This indicates a bug in the code */ - gpr_log( - GPR_ERROR, - "User called notify_on function with a previous callback still " - "pending"); - abort(); - } - break; + break; /* retry */ + } + + default: { + /* '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); + grpc_closure_sched( + exec_ctx, closure, + GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); + return; } + + /* 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"); + abort(); } } } + + GPR_UNREACHABLE_CODE(return ); } static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, @@ -1164,80 +1162,87 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, gpr_atm curr = CLOSURE_NOT_READY; gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; - bool is_done = false; - while (!is_done) { - is_done = true; - if (!gpr_atm_acq_cas(state, curr, new_state)) { - /* Fallback to slowpath */ - curr = gpr_atm_acq_load(state); - switch (curr) { - case CLOSURE_READY: { - is_done = false; - break; - } - - case CLOSURE_NOT_READY: { - is_done = false; - break; - } - - default: { - /* 'curr' is either a closure or the fd is already shutdown */ - if ((curr & FD_SHUTDOWN_BIT) > 0) { - /* fd is already shutdown. Do nothing */ - } else if (gpr_atm_rel_cas(state, curr, new_state)) { - grpc_closure_sched( - exec_ctx, (grpc_closure *)curr, - GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); - } else { - /* 'curr' was a closure but now changed to a different state. We - will have to retry */ - is_done = false; - } - break; - } - } + while (true) { + if (gpr_atm_acq_cas(state, curr, new_state)) { + return; /* Fast-path successful. Return */ } - } -} -static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { - /* Try the fast-path first (i.e expect the current value to be - * CLOSURE_NOT_READY */ - if (!gpr_atm_acq_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { /* Fallback to slowpath */ - gpr_atm curr = gpr_atm_acq_load(state); + curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_READY: { - /* Already ready. We are done here */ - break; + break; /* retry */ } case CLOSURE_NOT_READY: { - /* The state was not CLOSURE_NOT_READY when we checked initially at the - beginning of this function but now it is CLOSURE_NOT_READY again. - This is only possible if the state transitioned out of - CLOSURE_NOT_READY to either CLOSURE_READY or and then - back to CLOSURE_NOT_READY again (i.e after we entered this function, - the fd became "ready" and the necessary actions were already done). - So there is no need to make the state CLOSURE_READY now */ - break; + break; /* retry */ } default: { - /* 'curr' is either a closure or the fd is shutdown */ + /* '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) { - /* The fd is shutdown. Do nothing */ - } else if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { - grpc_closure_sched(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_NONE); + return; + } + + /* Fd is not shutdown. Schedule the closure and move the state to + shutdown state */ + if (gpr_atm_rel_cas(state, curr, new_state)) { + grpc_closure_sched( + exec_ctx, (grpc_closure *)curr, + GRPC_ERROR_CREATE_REFERENCING("FD Shutdown", &shutdown_err, 1)); + return; } - /* else the state changed again (only possible by either a racing - set_ready or set_shutdown functions. In both these cases, the closure - would have been scheduled for execution. So we are done here */ + + /* 'curr' was a closure but now changed to a different state. We will + have to retry */ break; } } - } /* else fast-path succeeded. We are done */ + } + + GPR_UNREACHABLE_CODE(return ); +} + +static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { + /* Try the fast-path first (i.e expect the current value to be + * CLOSURE_NOT_READY */ + if (gpr_atm_acq_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { + return; /* early out */ + } + + gpr_atm curr = gpr_atm_acq_load(state); + switch (curr) { + case CLOSURE_READY: { + /* Already ready. We are done here */ + break; + } + + case CLOSURE_NOT_READY: { + /* The state was not CLOSURE_NOT_READY when we checked initially at the + beginning of this function but now it is CLOSURE_NOT_READY again. + This is only possible if the state transitioned out of + CLOSURE_NOT_READY to either CLOSURE_READY or and then + back to CLOSURE_NOT_READY again (i.e after we entered this function, + the fd became "ready" and the necessary actions were already done). + So there is no need to make the state CLOSURE_READY now */ + break; + } + + default: { + /* 'curr' is either a closure or the fd is shutdown */ + if ((curr & FD_SHUTDOWN_BIT) > 0) { + /* The fd is shutdown. Do nothing */ + } else if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { + grpc_closure_sched(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_NONE); + } + /* else the state changed again (only possible by either a racing + set_ready or set_shutdown functions. In both these cases, the closure + would have been scheduled for execution. So we are done here */ + break; + } + } } static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, @@ -1248,7 +1253,7 @@ static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, static bool fd_is_shutdown(grpc_fd *fd) { grpc_error *err = (grpc_error *)gpr_atm_acq_load(&fd->shutdown_error); - return (((intptr_t) err & FD_SHUTDOWN_BIT) > 0); + return (((intptr_t)err & FD_SHUTDOWN_BIT) > 0); } /* Might be called multiple times */ -- cgit v1.2.3 From fb7ced6416357b86bc318fe6d460f56becf7c3b4 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 15 Feb 2017 18:31:57 -0800 Subject: use correct memory barriers --- src/core/lib/iomgr/ev_epoll_linux.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 108cf5a613..2a0b8917e8 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -1108,7 +1108,9 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, grpc_closure *closure) { while (true) { /* Fast-path: CLOSURE_NOT_READY -> */ - if (gpr_atm_acq_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { + /* Also do a release-cas here so that there is a 'happen-before' established + with acquire loads in set_ready() / set_shutdown() */ + if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { return; /* Fast-path successful. Return */ } @@ -1163,7 +1165,7 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; while (true) { - if (gpr_atm_acq_cas(state, curr, new_state)) { + if (gpr_atm_rel_cas(state, curr, new_state)) { return; /* Fast-path successful. Return */ } @@ -1206,9 +1208,9 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, } static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { - /* Try the fast-path first (i.e expect the current value to be - * CLOSURE_NOT_READY */ - if (gpr_atm_acq_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { + /* Try an optimistic case first (i.e assume current state is + CLOSURE_NOT_READY) */ + if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { return; /* early out */ } @@ -1247,7 +1249,7 @@ static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { static grpc_pollset *fd_get_read_notifier_pollset(grpc_exec_ctx *exec_ctx, grpc_fd *fd) { - gpr_atm notifier = gpr_atm_no_barrier_load(&fd->read_closure); + gpr_atm notifier = gpr_atm_acq_load(&fd->read_notifier_pollset); return (grpc_pollset *)notifier; } @@ -1259,7 +1261,7 @@ static bool fd_is_shutdown(grpc_fd *fd) { /* Might be called multiple times */ static void fd_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_error *why) { /* Store the shutdown error ORed with FD_SHUTDOWN_BIT in fd->shutdown_error */ - if (gpr_atm_acq_cas(&fd->shutdown_error, (gpr_atm)GRPC_ERROR_NONE, + if (gpr_atm_rel_cas(&fd->shutdown_error, (gpr_atm)GRPC_ERROR_NONE, (gpr_atm)why | FD_SHUTDOWN_BIT)) { shutdown(fd->fd, SHUT_RDWR); @@ -1474,7 +1476,8 @@ static void fd_become_readable(grpc_exec_ctx *exec_ctx, grpc_fd *fd, sets (This can happen briefly during polling island merges). In such cases it does not really matter which notifer is set as the read_notifier_pollset (They would both point to the same polling island anyway) */ - gpr_atm_no_barrier_store(&fd->read_notifier_pollset, (gpr_atm)notifier); + /* 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) { -- cgit v1.2.3 From 21f2f0682b1199e528c1c7fd19a397c8b7762eb9 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 16 Feb 2017 10:42:37 -0800 Subject: correct a few comments --- src/core/lib/iomgr/ev_epoll_linux.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 2a0b8917e8..9fb527bf07 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -1108,8 +1108,8 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, grpc_closure *closure) { while (true) { /* Fast-path: CLOSURE_NOT_READY -> */ - /* Also do a release-cas here so that there is a 'happen-before' established - with acquire loads in set_ready() / set_shutdown() */ + /* Also do a release-cas here so that any acqire-loads in set_ready or + set_shutdown see this */ if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { return; /* Fast-path successful. Return */ } @@ -1122,10 +1122,9 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, } case CLOSURE_READY: { - /* Change the state to CLOSURE_NOT_READY. - If successful: Schedule the closure - If not: Most likely the state transitioned to CLOSURE_NOT_READY. - Retry the fast-path again */ + /* Change the state to CLOSURE_NOT_READY. If successful: Schedule the + closure. If not, most likely the state transitioned to shutdown. We + should retry */ if (gpr_atm_rel_cas(state, CLOSURE_READY, CLOSURE_NOT_READY)) { grpc_closure_sched(exec_ctx, closure, GRPC_ERROR_NONE); return; /* Slow-path successful. Return */ -- cgit v1.2.3 From fae48aae0ffc4bf0f235ef7cd314267b68286807 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 16 Feb 2017 14:59:41 -0800 Subject: clang format code --- src/core/lib/iomgr/error.h | 1 - 1 file changed, 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index 8393605d77..2613512acb 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -136,7 +136,6 @@ typedef enum { GRPC_ERROR_TIME_CREATED, } grpc_error_times; - /// The following "special" errors can be propagated without allocating memory. /// They are always even so that other code (particularly combiner locks, /// polling engines) can safely use the lower bit for themselves. -- cgit v1.2.3 From ff4b25d802845d3237e3890db240f3059d0ff784 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 16 Feb 2017 15:07:11 -0800 Subject: Distinguish between CLOSURE_READY and (GRPC_ERROR_NONE | FD_SHUTDOWN_BIT) --- src/core/lib/iomgr/ev_epoll_linux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index e68c7971a6..5c77eda006 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -218,7 +218,7 @@ static void fd_global_init(void); static void fd_global_shutdown(void); #define CLOSURE_NOT_READY ((gpr_atm)0) -#define CLOSURE_READY ((gpr_atm)1) +#define CLOSURE_READY ((gpr_atm)2) #define FD_SHUTDOWN_BIT 1 -- cgit v1.2.3 From 8214b87fc5076c76914d3adced559cbece301015 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 23 Feb 2017 12:49:48 -0800 Subject: Comments around acquire/release --- src/core/lib/iomgr/ev_epoll_linux.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 31ef433229..f9132ec853 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -1108,13 +1108,15 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, grpc_closure *closure) { while (true) { /* Fast-path: CLOSURE_NOT_READY -> */ - /* Also do a release-cas here so that any acqire-loads in set_ready or - set_shutdown see this */ + /* The 'release' cas here matches the 'acquire' load in set_ready and + set_shutdown to ensure that the clousure being run happens-after the call + to notify_on */ if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { return; /* Fast-path successful. Return */ } /* Slowpath */ + /* The 'acquire' load matches the 'release' cas in set_ready/set_shutdown */ gpr_atm curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_NOT_READY: { @@ -1164,11 +1166,14 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; while (true) { + /* The 'release' cas here matches the 'acquire' load in notify_on and + set_ready */ if (gpr_atm_rel_cas(state, curr, new_state)) { return; /* Fast-path successful. Return */ } /* Fallback to slowpath */ + /* This 'acquire' load matches the 'release' in notify_on and set_ready */ curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_READY: { @@ -1209,10 +1214,13 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { /* Try an optimistic case first (i.e assume current state is CLOSURE_NOT_READY) */ + /* This 'release' cas matches the 'acquire' load in notify_on ensuring that + the closure being run happens-after the return from epoll_pwait */ if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { return; /* early out */ } + /* The 'acquire' here matches the 'release' in notify_on / set_shutdown */ gpr_atm curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_READY: { @@ -1863,7 +1871,7 @@ retry: (void *)pi_new, FD_FROM_PO(item)->fd, poll_obj_string(bag_type), (void *)bag); /* No need to lock 'pi_new' here since this is a new polling island - * and no one has a reference to it yet */ + and no one has a reference to it yet */ polling_island_remove_all_fds_locked(pi_new, true, &error); /* Ref and unref so that the polling island gets deleted during unref -- cgit v1.2.3 From 97e3ecc21633b0fad771e6daaa571ae6ff7b6cf6 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 23 Feb 2017 14:51:11 -0800 Subject: Comments and relaxed cas in some cases --- src/core/lib/iomgr/ev_epoll_linux.c | 56 ++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 20 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index f9132ec853..11208b9ad1 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -1107,16 +1107,16 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, grpc_closure *closure) { while (true) { - /* Fast-path: CLOSURE_NOT_READY -> */ - /* The 'release' cas here matches the 'acquire' load in set_ready and - set_shutdown to ensure that the clousure being run happens-after the call - to notify_on */ + /* Fast-path: CLOSURE_NOT_READY -> . + The 'release' cas here matches the 'acquire' load in set_ready and + set_shutdown ensuring that the closure (scheduled by set_ready or + set_shutdown) happens-after the I/O event on the fd */ if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, (gpr_atm)closure)) { return; /* Fast-path successful. Return */ } - /* Slowpath */ - /* The 'acquire' load matches the 'release' cas in set_ready/set_shutdown */ + /* Slowpath. The 'acquire' load matches the 'release' cas in set_ready and + set_shutdown */ gpr_atm curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_NOT_READY: { @@ -1124,10 +1124,15 @@ static void notify_on(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, } case CLOSURE_READY: { - /* Change the state to CLOSURE_NOT_READY. If successful: Schedule the - closure. If not, most likely the state transitioned to shutdown. We - should retry */ - if (gpr_atm_rel_cas(state, CLOSURE_READY, CLOSURE_NOT_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 + 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, CLOSURE_READY, CLOSURE_NOT_READY)) { grpc_closure_sched(exec_ctx, closure, GRPC_ERROR_NONE); return; /* Slow-path successful. Return */ } @@ -1166,14 +1171,15 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, gpr_atm new_state = (gpr_atm)shutdown_err | FD_SHUTDOWN_BIT; while (true) { - /* The 'release' cas here matches the 'acquire' load in notify_on and - set_ready */ + /* The 'release' cas here matches the 'acquire' load in notify_on to ensure + that the closure it schedules 'happens-after' the set_shutdown is called + on the fd */ if (gpr_atm_rel_cas(state, curr, new_state)) { return; /* Fast-path successful. Return */ } - /* Fallback to slowpath */ - /* This 'acquire' load matches the 'release' in notify_on and set_ready */ + /* Fallback to slowpath. This 'acquire' load matches the 'release' cas in + notify_on and set_ready */ curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_READY: { @@ -1193,7 +1199,9 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, } /* Fd is not shutdown. Schedule the closure and move the state to - shutdown state */ + shutdown state. The 'release' cas here matches the 'acquire' load in + notify_on to ensure that the closure it schedules 'happens-after' + the set_shutdown is called on the fd */ if (gpr_atm_rel_cas(state, curr, new_state)) { grpc_closure_sched( exec_ctx, (grpc_closure *)curr, @@ -1213,14 +1221,17 @@ static void set_shutdown(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state, static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { /* Try an optimistic case first (i.e assume current state is - CLOSURE_NOT_READY) */ - /* This 'release' cas matches the 'acquire' load in notify_on ensuring that - the closure being run happens-after the return from epoll_pwait */ + CLOSURE_NOT_READY). + + This 'release' cas matches the 'acquire' load in notify_on ensuring that + any closure (scheduled by notify_on) 'happens-after' the return from + epoll_pwait */ if (gpr_atm_rel_cas(state, CLOSURE_NOT_READY, CLOSURE_READY)) { return; /* early out */ } - /* The 'acquire' here matches the 'release' in notify_on / set_shutdown */ + /* The 'acquire' load here matches the 'release' cas in notify_on and + set_shutdown */ gpr_atm curr = gpr_atm_acq_load(state); switch (curr) { case CLOSURE_READY: { @@ -1243,7 +1254,12 @@ static void set_ready(grpc_exec_ctx *exec_ctx, grpc_fd *fd, gpr_atm *state) { /* 'curr' is either a closure or the fd is shutdown */ if ((curr & FD_SHUTDOWN_BIT) > 0) { /* The fd is shutdown. Do nothing */ - } else if (gpr_atm_rel_cas(state, curr, CLOSURE_NOT_READY)) { + } else if (gpr_atm_no_barrier_cas(state, curr, CLOSURE_NOT_READY)) { + /* The cas above was no-barrier since the state is being transitioned to + CLOSURE_NOT_READY; notify_on and set_shutdown do not schedule any + closures when transitioning out of CLOSURE_NO_READY state (i.e there + is no other code that needs to 'happen-after' this) */ + grpc_closure_sched(exec_ctx, (grpc_closure *)curr, GRPC_ERROR_NONE); } /* else the state changed again (only possible by either a racing -- cgit v1.2.3