Hoist epoll_ctl outside of pollset lock

4 files changed, 46 insertions, 16 deletions

diff --git a/src/core/iomgr/pollset_multipoller_with_epoll.c b/src/core/iomgr/pollset_multipoller_with_epoll.c
index 3746c8edaf..d697b59e4c 100644
--- a/src/core/iomgr/pollset_multipoller_with_epoll.c
+++ b/src/core/iomgr/pollset_multipoller_with_epoll.c
@@ -50,12 +50,17 @@ typedef struct {
 } pollset_hdr;
 
 static void multipoll_with_epoll_pollset_add_fd(grpc_pollset *pollset,
-                                                grpc_fd *fd) {
+                                                grpc_fd *fd,
+                                                int and_unlock_pollset) {
   pollset_hdr *h = pollset->data.ptr;
   struct epoll_event ev;
   int err;
   grpc_fd_watcher watcher;
 
+  if (and_unlock_pollset) {
+    gpr_mu_unlock(&pollset->mu);
+  }
+
   /* We pretend to be polling whilst adding an fd to keep the fd from being
      closed during the add. This may result in a spurious wakeup being assigned
      to this pollset whilst adding, but that should be benign. */
@@ -76,9 +81,15 @@ static void multipoll_with_epoll_pollset_add_fd(grpc_pollset *pollset,
 }
 
 static void multipoll_with_epoll_pollset_del_fd(grpc_pollset *pollset,
-                                                grpc_fd *fd) {
+                                                grpc_fd *fd,
+                                                int and_unlock_pollset) {
   pollset_hdr *h = pollset->data.ptr;
   int err;
+
+  if (and_unlock_pollset) {
+    gpr_mu_unlock(&pollset->mu);
+  }
+
   /* Note that this can race with concurrent poll, but that should be fine since
    * at worst it creates a spurious read event on a reused grpc_fd object. */
   err = epoll_ctl(h->epoll_fd, EPOLL_CTL_DEL, fd->fd, NULL);
@@ -183,7 +194,7 @@ static void epoll_become_multipoller(grpc_pollset *pollset, grpc_fd **fds,
     abort();
   }
   for (i = 0; i < nfds; i++) {
-    multipoll_with_epoll_pollset_add_fd(pollset, fds[i]);
+    multipoll_with_epoll_pollset_add_fd(pollset, fds[i], 0);
   }
 
   grpc_wakeup_fd_create(&h->wakeup_fd);
diff --git a/src/core/iomgr/pollset_multipoller_with_poll_posix.c b/src/core/iomgr/pollset_multipoller_with_poll_posix.c
index 7b717bd159..7377a04817 100644
--- a/src/core/iomgr/pollset_multipoller_with_poll_posix.c
+++ b/src/core/iomgr/pollset_multipoller_with_poll_posix.c
@@ -66,7 +66,8 @@ typedef struct {
 } pollset_hdr;
 
 static void multipoll_with_poll_pollset_add_fd(grpc_pollset *pollset,
-                                               grpc_fd *fd) {
+                                               grpc_fd *fd,
+                                               int and_unlock_pollset) {
   size_t i;
   pollset_hdr *h = pollset->data.ptr;
   /* TODO(ctiller): this is O(num_fds^2); maybe switch to a hash set here */
@@ -79,10 +80,14 @@ static void multipoll_with_poll_pollset_add_fd(grpc_pollset *pollset,
   }
   h->fds[h->fd_count++] = fd;
   GRPC_FD_REF(fd, "multipoller");
+  if (and_unlock_pollset) {
+    gpr_mu_unlock(&pollset->mu);
+  }
 }
 
 static void multipoll_with_poll_pollset_del_fd(grpc_pollset *pollset,
-                                               grpc_fd *fd) {
+                                               grpc_fd *fd,
+                                               int and_unlock_pollset) {
   /* will get removed next poll cycle */
   pollset_hdr *h = pollset->data.ptr;
   if (h->del_count == h->del_capacity) {
@@ -91,6 +96,9 @@ static void multipoll_with_poll_pollset_del_fd(grpc_pollset *pollset,
   }
   h->dels[h->del_count++] = fd;
   GRPC_FD_REF(fd, "multipoller_del");
+  if (and_unlock_pollset) {
+    gpr_mu_unlock(&pollset->mu);
+  }
 }
 
 static void end_polling(grpc_pollset *pollset) {
diff --git a/src/core/iomgr/pollset_posix.c b/src/core/iomgr/pollset_posix.c
index 85101764d2..e8189c28ad 100644
--- a/src/core/iomgr/pollset_posix.c
+++ b/src/core/iomgr/pollset_posix.c
@@ -105,14 +105,12 @@ void grpc_pollset_init(grpc_pollset *pollset) {
 
 void grpc_pollset_add_fd(grpc_pollset *pollset, grpc_fd *fd) {
   gpr_mu_lock(&pollset->mu);
-  pollset->vtable->add_fd(pollset, fd);
-  gpr_mu_unlock(&pollset->mu);
+  pollset->vtable->add_fd(pollset, fd, 1);
 }
 
 void grpc_pollset_del_fd(grpc_pollset *pollset, grpc_fd *fd) {
   gpr_mu_lock(&pollset->mu);
-  pollset->vtable->del_fd(pollset, fd);
-  gpr_mu_unlock(&pollset->mu);
+  pollset->vtable->del_fd(pollset, fd, 1);
 }
 
 static void finish_shutdown(grpc_pollset *pollset) {
@@ -257,7 +255,7 @@ static void basic_do_promote(void *args, int success) {
   } else if (grpc_fd_is_orphaned(fd)) {
     /* Don't try to add it to anything, we'll drop our ref on it below */
   } else if (pollset->vtable != original_vtable) {
-    pollset->vtable->add_fd(pollset, fd);
+    pollset->vtable->add_fd(pollset, fd, 0);
   } else if (fd != pollset->data.ptr) {
     grpc_fd *fds[2];
     fds[0] = pollset->data.ptr;
@@ -287,10 +285,11 @@ static void basic_do_promote(void *args, int success) {
   GRPC_FD_UNREF(fd, "basicpoll_add");
 }
 
-static void basic_pollset_add_fd(grpc_pollset *pollset, grpc_fd *fd) {
+static void basic_pollset_add_fd(grpc_pollset *pollset, grpc_fd *fd,
+                                 int and_unlock_pollset) {
   grpc_unary_promote_args *up_args;
   GPR_ASSERT(fd);
-  if (fd == pollset->data.ptr) return;
+  if (fd == pollset->data.ptr) goto exit;
 
   if (!pollset->counter) {
     /* Fast path -- no in flight cbs */
@@ -313,7 +312,7 @@ static void basic_pollset_add_fd(grpc_pollset *pollset, grpc_fd *fd) {
       pollset->data.ptr = fd;
       GRPC_FD_REF(fd, "basicpoll");
     }
-    return;
+    goto exit;
   }
 
   /* Now we need to promote. This needs to happen when we're not polling. Since
@@ -329,14 +328,24 @@ static void basic_pollset_add_fd(grpc_pollset *pollset, grpc_fd *fd) {
   grpc_iomgr_add_callback(&up_args->promotion_closure);
 
   grpc_pollset_kick(pollset);
+
+exit:
+  if (and_unlock_pollset) {
+    gpr_mu_unlock(&pollset->mu);
+  }
 }
 
-static void basic_pollset_del_fd(grpc_pollset *pollset, grpc_fd *fd) {
+static void basic_pollset_del_fd(grpc_pollset *pollset, grpc_fd *fd,
+                                 int and_unlock_pollset) {
   GPR_ASSERT(fd);
   if (fd == pollset->data.ptr) {
     GRPC_FD_UNREF(pollset->data.ptr, "basicpoll");
     pollset->data.ptr = NULL;
   }
+
+  if (and_unlock_pollset) {
+    gpr_mu_unlock(&pollset->mu);
+  }
 }
 
 static void basic_pollset_maybe_work(grpc_pollset *pollset,
diff --git a/src/core/iomgr/pollset_posix.h b/src/core/iomgr/pollset_posix.h
index 53585a2886..37de1276d1 100644
--- a/src/core/iomgr/pollset_posix.h
+++ b/src/core/iomgr/pollset_posix.h
@@ -66,8 +66,10 @@ typedef struct grpc_pollset {
 } grpc_pollset;
 
 struct grpc_pollset_vtable {
-  void (*add_fd)(grpc_pollset *pollset, struct grpc_fd *fd);
-  void (*del_fd)(grpc_pollset *pollset, struct grpc_fd *fd);
+  void (*add_fd)(grpc_pollset *pollset, struct grpc_fd *fd,
+                 int and_unlock_pollset);
+  void (*del_fd)(grpc_pollset *pollset, struct grpc_fd *fd,
+                 int and_unlock_pollset);
   void (*maybe_work)(grpc_pollset *pollset, gpr_timespec deadline,
                      gpr_timespec now, int allow_synchronous_callback);
   void (*kick)(grpc_pollset *pollset);