Kicked worker shouldn't add pollset to neighbhourhood

1 files changed, 29 insertions, 16 deletions

diff --git a/src/core/lib/iomgr/ev_epoll1_linux.c b/src/core/lib/iomgr/ev_epoll1_linux.c
index 7f053fa728..b76eb9e1c9 100644
--- a/src/core/lib/iomgr/ev_epoll1_linux.c
+++ b/src/core/lib/iomgr/ev_epoll1_linux.c
@@ -698,22 +698,30 @@ static bool begin_worker(grpc_pollset *pollset, grpc_pollset_worker *worker,
         gpr_mu_unlock(&pollset->mu);
         goto retry_lock_neighbourhood;
       }
-      pollset->seen_inactive = false;
-      if (neighbourhood->active_root == NULL) {
-        neighbourhood->active_root = pollset->next = pollset->prev = pollset;
-        /* TODO: sreek. Why would this worker state be other than UNKICKED
-         * here ? (since the worker isn't added to the pollset yet, there is no
-         * way it can be "found" by other threads to get kicked). */
-
-        /* If there is no designated poller, make this the designated poller */
-        if (worker->kick_state == UNKICKED &&
-            gpr_atm_no_barrier_cas(&g_active_poller, 0, (gpr_atm)worker)) {
-          SET_KICK_STATE(worker, DESIGNATED_POLLER);
+
+      /* In the brief time we released the pollset locks above, the worker MAY
+         have been kicked. In this case, the worker should get out of this
+         pollset ASAP and hence this should neither add the pollset to
+         neighbourhood nor mark the pollset as active.
+
+         On a side note, the only way a worker's kick state could have changed
+         at this point is if it were "kicked specifically". Since the worker has
+         not added itself to the pollset yet (by calling worker_insert()), it is
+         not visible in the "kick any" path yet */
+      if (worker->kick_state == UNKICKED) {
+        pollset->seen_inactive = false;
+        if (neighbourhood->active_root == NULL) {
+          neighbourhood->active_root = pollset->next = pollset->prev = pollset;
+          /* Make this the designated poller if there isn't one already */
+          if (worker->kick_state == UNKICKED &&
+              gpr_atm_no_barrier_cas(&g_active_poller, 0, (gpr_atm)worker)) {
+            SET_KICK_STATE(worker, DESIGNATED_POLLER);
+          }
+        } else {
+          pollset->next = neighbourhood->active_root;
+          pollset->prev = pollset->next->prev;
+          pollset->next->prev = pollset->prev->next = pollset;
         }
-      } else {
-        pollset->next = neighbourhood->active_root;
-        pollset->prev = pollset->next->prev;
-        pollset->next->prev = pollset->prev->next = pollset;
       }
     }
     if (is_reassigning) {
@@ -1001,6 +1009,7 @@ static grpc_error *pollset_kick(grpc_pollset *pollset,
     gpr_log(GPR_ERROR, "%s", tmp);
     gpr_free(tmp);
   }
+
   if (specific_worker == NULL) {
     if (gpr_tls_get(&g_current_thread_pollset) != (intptr_t)pollset) {
       grpc_pollset_worker *root_worker = pollset->root_worker;
@@ -1076,7 +1085,11 @@ static grpc_error *pollset_kick(grpc_pollset *pollset,
       }
       goto done;
     }
-  } else if (specific_worker->kick_state == KICKED) {
+
+    GPR_UNREACHABLE_CODE(goto done);
+  }
+
+  if (specific_worker->kick_state == KICKED) {
     if (GRPC_TRACER_ON(grpc_polling_trace)) {
       gpr_log(GPR_ERROR, " .. specific worker already kicked");
     }