From a25c5a91f3e0e5b60e43fdf2f1542649996bdcbf Mon Sep 17 00:00:00 2001 From: Yedidya Feldblum Date: Tue, 17 Oct 2017 16:09:36 -0700 Subject: [PATCH] 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 --- folly/io/ShutdownSocketSet.cpp | 87 +++++++++++++++------------------- folly/io/ShutdownSocketSet.h | 10 +++- 2 files changed, 47 insertions(+), 50 deletions(-) diff --git a/folly/io/ShutdownSocketSet.cpp b/folly/io/ShutdownSocketSet.cpp index a64ae49a..3380559d 100644 --- 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 9ba145db..dee5fb40 100644 --- 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 { -- 2.34.1