Refactor ShutdownSocketSet atomic state machine

Summary:
[Folly] Refactor `ShutdownSocketSet` atomic state machine.

* Format.
* Use `while` over `goto`.
* Avoid `memory_order_acq_rel` as the single-argument memory order; some platforms fail to build that.
* Use `memory_order_relaxed` for all atomic operations because stronger memory orders are not actually required: the atomic state never serves as a barrier for synchronizing loads and stores of associated data because there is no associated data.

Reviewed By: davidtgoldblatt

Differential Revision: D6058292

fbshipit-source-id: d45d7fcfa472e6e393a5f980e75ad9ea3358bab3

diff --git a/folly/io/ShutdownSocketSet.cpp b/folly/io/ShutdownSocketSet.cpp
index a64ae49ae5e9ed8595dbe115d78cbea45cbed4c2..3380559d8e9da46493b7cd9925bcb5e205e8800f 100644 (file)
--- a/folly/io/ShutdownSocketSet.cpp
+++ b/folly/io/ShutdownSocketSet.cpp
@@ -51,10 +51,9 @@ void ShutdownSocketSet::add(int fd) {
 
   auto& sref = data_[size_t(fd)];
   uint8_t prevState = FREE;
-  CHECK(sref.compare_exchange_strong(prevState,
-                                     IN_USE,
-                                     std::memory_order_acq_rel))
-    << "Invalid prev state for fd " << fd << ": " << int(prevState);
+  CHECK(sref.compare_exchange_strong(
+      prevState, IN_USE, std::memory_order_relaxed))
+      << "Invalid prev state for fd " << fd << ": " << int(prevState);
 }
 
 void ShutdownSocketSet::remove(int fd) {
@@ -66,23 +65,19 @@ void ShutdownSocketSet::remove(int fd) {
   auto& sref = data_[size_t(fd)];
   uint8_t prevState = 0;
 
-retry_load:
   prevState = sref.load(std::memory_order_relaxed);
-
-retry:
-  switch (prevState) {
-  case IN_SHUTDOWN:
-    std::this_thread::sleep_for(std::chrono::milliseconds(1));
-    goto retry_load;
-  case FREE:
-    LOG(FATAL) << "Invalid prev state for fd " << fd << ": " << int(prevState);
-  }
-
-  if (!sref.compare_exchange_weak(prevState,
-                                  FREE,
-                                  std::memory_order_acq_rel)) {
-    goto retry;
-  }
+  do {
+    switch (prevState) {
+      case IN_SHUTDOWN:
+        std::this_thread::sleep_for(std::chrono::milliseconds(1));
+        prevState = sref.load(std::memory_order_relaxed);
+        continue;
+      case FREE:
+        LOG(FATAL) << "Invalid prev state for fd " << fd << ": "
+                   << int(prevState);
+    }
+  } while (
+      !sref.compare_exchange_weak(prevState, FREE, std::memory_order_relaxed));
 }
 
 int ShutdownSocketSet::close(int fd) {
@@ -95,24 +90,21 @@ int ShutdownSocketSet::close(int fd) {
   uint8_t prevState = sref.load(std::memory_order_relaxed);
   uint8_t newState = 0;
 
-retry:
-  switch (prevState) {
-  case IN_USE:
-  case SHUT_DOWN:
-    newState = FREE;
-    break;
-  case IN_SHUTDOWN:
-    newState = MUST_CLOSE;
-    break;
-  default:
-    LOG(FATAL) << "Invalid prev state for fd " << fd << ": " << int(prevState);
-  }
-
-  if (!sref.compare_exchange_weak(prevState,
-                                  newState,
-                                  std::memory_order_acq_rel)) {
-    goto retry;
-  }
+  do {
+    switch (prevState) {
+      case IN_USE:
+      case SHUT_DOWN:
+        newState = FREE;
+        break;
+      case IN_SHUTDOWN:
+        newState = MUST_CLOSE;
+        break;
+      default:
+        LOG(FATAL) << "Invalid prev state for fd " << fd << ": "
+                   << int(prevState);
+    }
+  } while (!sref.compare_exchange_weak(
+      prevState, newState, std::memory_order_relaxed));
 
   return newState == FREE ? folly::closeNoInt(fd) : 0;
 }
@@ -126,18 +118,16 @@ void ShutdownSocketSet::shutdown(int fd, bool abortive) {
 
   auto& sref = data_[size_t(fd)];
   uint8_t prevState = IN_USE;
-  if (!sref.compare_exchange_strong(prevState,
-                                    IN_SHUTDOWN,
-                                    std::memory_order_acq_rel)) {
+  if (!sref.compare_exchange_strong(
+          prevState, IN_SHUTDOWN, std::memory_order_relaxed)) {
     return;
   }
 
   doShutdown(fd, abortive);
 
   prevState = IN_SHUTDOWN;
-  if (sref.compare_exchange_strong(prevState,
-                                   SHUT_DOWN,
-                                   std::memory_order_acq_rel)) {
+  if (sref.compare_exchange_strong(
+          prevState, SHUT_DOWN, std::memory_order_relaxed)) {
     return;
   }
 
@@ -146,16 +136,15 @@ void ShutdownSocketSet::shutdown(int fd, bool abortive) {
 
   folly::closeNoInt(fd);  // ignore errors, nothing to do
 
-  CHECK(sref.compare_exchange_strong(prevState,
-                                     FREE,
-                                     std::memory_order_acq_rel))
-    << "Invalid prev state for fd " << fd << ": " << int(prevState);
+  CHECK(
+      sref.compare_exchange_strong(prevState, FREE, std::memory_order_relaxed))
+      << "Invalid prev state for fd " << fd << ": " << int(prevState);
 }
 
 void ShutdownSocketSet::shutdownAll(bool abortive) {
   for (int i = 0; i < maxFd_; ++i) {
     auto& sref = data_[size_t(i)];
-    if (sref.load(std::memory_order_acquire) == IN_USE) {
+    if (sref.load(std::memory_order_relaxed) == IN_USE) {
       shutdown(i, abortive);
     }
   }

diff --git a/folly/io/ShutdownSocketSet.h b/folly/io/ShutdownSocketSet.h
index 9ba145db080b22db9080ec633392f14500e39b60..dee5fb40f0e5478de2fb94a7605339d8f7679f18 100644 (file)
--- a/folly/io/ShutdownSocketSet.h
+++ b/folly/io/ShutdownSocketSet.h
@@ -117,12 +117,20 @@ class ShutdownSocketSet : private boost::noncopyable {
   //   (::shutdown())
   //   IN_SHUTDOWN -> SHUT_DOWN
   //   MUST_CLOSE -> (::close()) -> FREE
+  //
+  // State atomic operation memory orders:
+  // All atomic operations on per-socket states use std::memory_order_relaxed
+  // because there is no associated per-socket data guarded by the state and
+  // the states for different sockets are unrelated. If there were associated
+  // per-socket data, acquire and release orders would be desired; and if the
+  // states for different sockets were related, it could be that sequential
+  // consistent orders would be desired.
   enum State : uint8_t {
     FREE = 0,
     IN_USE,
     IN_SHUTDOWN,
     SHUT_DOWN,
-    MUST_CLOSE
+    MUST_CLOSE,
   };
 
   struct Free {