[RFC PATCH 2/4] libstdc++ futex: Support waiting on std::chrono::steady_clock directly

[RFC PATCH 2/4] libstdc++ futex: Support waiting on std::chrono::steady_clock directly

[Mike Crowe]

The user-visible effect of this change is for std::future::wait_until
to use CLOCK_MONOTONIC when passed a timeout of
std::chrono::steady_clock::time_point type. This makes it immune to
any changes made to the system clock CLOCK_REALTIME.

Add an overload of __atomic_futex_unsigned::_M_load_and_text_until_impl
that accepts a std::chrono::steady_clock, and correctly passes this through
to __atomic_futex_unsigned_base::_M_futex_wait_until_steady which uses
CLOCK_MONOTONIC for the timeout within the futex system call. These
functions are mostly just copies of the std::chrono::system_clock versions
with small tweaks.

Prior to this commit, a std::chrono::steady timeout would be converted via
std::chrono::system_clock which risks reducing or increasing the timeout if
someone changes CLOCK_REALTIME whilst the wait is happening. (The commit
immediately prior to this one increases the window of opportunity for that
from a short period during the calculation of a relative timeout, to the
entire duration of the wait.)

I believe that I've added this functionality in a way that it doesn't break
ABI compatibility, but that has made it more verbose and less type safe. I
believe that it would be better to maintain the timeout as an instance of
the correct clock type all the way down to a single _M_futex_wait_until
function with an overload for each clock. The current scheme of separating
out the seconds and nanoseconds early risks accidentally calling the wait
function for the wrong clock. Unfortunately, doing this would break code
that compiled against the old header.
---
 libstdc++-v3/include/bits/atomic_futex.h | 67 +++++++++++++++++++++++++++++++-
 libstdc++-v3/src/c++11/futex.cc          | 33 ++++++++++++++++
 2 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
index afcfeb7720d..c2b3df03592 100644
--- a/libstdc++-v3/include/bits/atomic_futex.h
+++ b/libstdc++-v3/include/bits/atomic_futex.h
@@ -52,11 +52,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if defined(_GLIBCXX_HAVE_LINUX_FUTEX) && ATOMIC_INT_LOCK_FREE > 1
   struct __atomic_futex_unsigned_base
   {
-    // Returns false iff a timeout occurred.
+    // __s and __ns are measured against CLOCK_REALTIME. Returns false
+    // iff a timeout occurred.
     bool
     _M_futex_wait_until(unsigned *__addr, unsigned __val, bool __has_timeout,
 	chrono::seconds __s, chrono::nanoseconds __ns);
 
+    // __s and __ns are measured against CLOCK_MONOTONIC. Returns
+    // false iff a timeout occurred.
+    bool
+    _M_futex_wait_until_steady(unsigned *__addr, unsigned __val, bool __has_timeout,
+	chrono::seconds __s, chrono::nanoseconds __ns);
+
     // This can be executed after the object has been destroyed.
     static void _M_futex_notify_all(unsigned* __addr);
   };
@@ -86,6 +93,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // value if equal is false.
     // The assumed value is the caller's assumption about the current value
     // when making the call.
+    // __s and __ns are measured against CLOCK_REALTIME.
     unsigned
     _M_load_and_test_until(unsigned __assumed, unsigned __operand,
 	bool __equal, memory_order __mo, bool __has_timeout,
@@ -110,6 +118,36 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	}
     }
 
+    // If a timeout occurs, returns a current value after the timeout;
+    // otherwise, returns the operand's value if equal is true or a different
+    // value if equal is false.
+    // The assumed value is the caller's assumption about the current value
+    // when making the call.
+    // __s and __ns are measured against CLOCK_MONOTONIC.
+    unsigned
+    _M_load_and_test_until_steady(unsigned __assumed, unsigned __operand,
+	bool __equal, memory_order __mo, bool __has_timeout,
+	chrono::seconds __s, chrono::nanoseconds __ns)
+    {
+      for (;;)
+	{
+	  // Don't bother checking the value again because we expect the caller
+	  // to have done it recently.
+	  // memory_order_relaxed is sufficient because we can rely on just the
+	  // modification order (store_notify uses an atomic RMW operation too),
+	  // and the futex syscalls synchronize between themselves.
+	  _M_data.fetch_or(_Waiter_bit, memory_order_relaxed);
+	  bool __ret = _M_futex_wait_until_steady((unsigned*)(void*)&_M_data,
+					   __assumed | _Waiter_bit,
+					   __has_timeout, __s, __ns);
+	  // Fetch the current value after waiting (clears _Waiter_bit).
+	  __assumed = _M_load(__mo);
+	  if (!__ret || ((__operand == __assumed) == __equal))
+	    return __assumed;
+	  // TODO adapt wait time
+	}
+    }
+
     // Returns the operand's value if equal is true or a different value if
     // equal is false.
     // The assumed value is the caller's assumption about the current value
@@ -140,6 +178,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  true, __s.time_since_epoch(), __ns);
     }
 
+    template<typename _Dur>
+    unsigned
+    _M_load_and_test_until_impl(unsigned __assumed, unsigned __operand,
+	bool __equal, memory_order __mo,
+	const chrono::time_point<std::chrono::steady_clock, _Dur>& __atime)
+    {
+      auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
+      auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
+      // XXX correct?
+      return _M_load_and_test_until_steady(__assumed, __operand, __equal, __mo,
+	  true, __s.time_since_epoch(), __ns);
+    }
+
   public:
 
     _GLIBCXX_ALWAYS_INLINE unsigned
@@ -200,6 +251,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return (__i & ~_Waiter_bit) == __val;
     }
 
+    // Returns false iff a timeout occurred.
+    template<typename _Duration>
+    _GLIBCXX_ALWAYS_INLINE bool
+    _M_load_when_equal_until(unsigned __val, memory_order __mo,
+	const chrono::time_point<std::chrono::steady_clock, _Duration>& __atime)
+    {
+      unsigned __i = _M_load(__mo);
+      if ((__i & ~_Waiter_bit) == __val)
+	return true;
+      // TODO Spin-wait first.  Ignore effect on timeout.
+      __i = _M_load_and_test_until_impl(__i, __val, true, __mo, __atime);
+      return (__i & ~_Waiter_bit) == __val;
+    }
+
     _GLIBCXX_ALWAYS_INLINE void
     _M_store_notify_all(unsigned __val, memory_order __mo)
     {
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index 217aeefe005..fc2dcd7ff99 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -36,6 +36,7 @@
 // Constants for the wait/wake futex syscall operations
 const unsigned futex_wait_op = 0;
 const unsigned futex_wait_bitset_op = 9;
+const unsigned futex_clock_monotonic_flag = 0;
 const unsigned futex_clock_realtime_flag = 256;
 const unsigned futex_bitset_match_any = ~0;
 const unsigned futex_wake_op = 1;
@@ -75,6 +76,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
   }
 
+  bool
+  __atomic_futex_unsigned_base::_M_futex_wait_until_steady(unsigned *__addr,
+      unsigned __val,
+      bool __has_timeout, chrono::seconds __s, chrono::nanoseconds __ns)
+  {
+    if (!__has_timeout)
+      {
+	// Ignore whether we actually succeeded to block because at worst,
+	// we will fall back to spin-waiting.  The only thing we could do
+	// here on errors is abort.
+	int ret __attribute__((unused));
+	ret = syscall (SYS_futex, __addr, futex_wait_op, __val, nullptr);
+	_GLIBCXX_DEBUG_ASSERT(ret == 0 || errno == EINTR || errno == EAGAIN);
+	return true;
+      }
+    else
+      {
+	struct timespec rt;
+	rt.tv_sec = __s.count();
+	rt.tv_nsec = __ns.count();
+
+	if (syscall (SYS_futex, __addr, futex_wait_bitset_op | futex_clock_monotonic_flag, __val, &rt, nullptr, futex_bitset_match_any) == -1)
+	  {
+	    _GLIBCXX_DEBUG_ASSERT(errno == EINTR || errno == EAGAIN
+				  || errno == ETIMEDOUT);
+	    if (errno == ETIMEDOUT)
+	      return false;
+	  }
+	return true;
+      }
+  }
+
   void
   __atomic_futex_unsigned_base::_M_futex_notify_all(unsigned* __addr)
   {
-- 
2.11.0

[RFC PATCH 0/4] Make std::future::wait_* use std::chrono::steady_clock when required

[Mike Crowe]

This is a first attempt to make std::future::wait_until and
std::future::wait_for make correct use of
std::chrono::steady_clock/CLOCK_MONOTONIC. It also makes
std::future::wait_until react to changes to CLOCK_REALTIME during the
wait, but only when passed a std::chrono::system_clock time point.

Prior to these patches, the situation is similar to that described at
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861 for
std::condition_variable, but with the benefit that this time the bug can be
fixed entirely within libstdc++.

(I've omitted the ChangeLog entries for now, since I don't believe that
this is ready to be accepted in its current form.)

Mike.

Mike Crowe (4):
  libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait
  libstdc++ futex: Support waiting on std::chrono::steady_clock directly
  libstdc++ atomic_futex: Use std::chrono::steady_clock as reference
    clock
  Extra async tests, not for merging

 libstdc++-v3/include/bits/atomic_futex.h         | 74 +++++++++++++++++++++--
 libstdc++-v3/src/c++11/futex.cc                  | 48 +++++++++++----
 libstdc++-v3/testsuite/30_threads/async/async.cc | 76 ++++++++++++++++++++++++
 3 files changed, 181 insertions(+), 17 deletions(-)

-- 
2.11.0

[RFC PATCH 1/4] libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait

[Mike Crowe]

The futex system call supports waiting for an absolute time if
FUTEX_WAIT_BITSET is used rather than FUTEX_WAIT. Doing so provides two
benefits:

1. The call to gettimeofday is not required in order to calculate a
   relative timeout.

2. If someone changes the system clock during the wait then the futex
   timeout will correctly expire earlier or later. Currently that only
   happens if the clock is changed prior to the call to gettimeofday.

According to futex(2), support for FUTEX_CLOCK_REALTIME was added in the
v2.6.28 Linux kernel and FUTEX_WAIT_BITSET was added in v2.6.25. There is
no attempt to detect the kernel version and fall back to the previous
method.
---
 libstdc++-v3/src/c++11/futex.cc | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index 64aada06969..217aeefe005 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -35,6 +35,9 @@
 
 // Constants for the wait/wake futex syscall operations
 const unsigned futex_wait_op = 0;
+const unsigned futex_wait_bitset_op = 9;
+const unsigned futex_clock_realtime_flag = 256;
+const unsigned futex_bitset_match_any = ~0;
 const unsigned futex_wake_op = 1;
 
 namespace std _GLIBCXX_VISIBILITY(default)
@@ -58,22 +61,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
     else
       {
-	struct timeval tv;
-	gettimeofday (&tv, NULL);
-	// Convert the absolute timeout value to a relative timeout
 	struct timespec rt;
-	rt.tv_sec = __s.count() - tv.tv_sec;
-	rt.tv_nsec = __ns.count() - tv.tv_usec * 1000;
-	if (rt.tv_nsec < 0)
-	  {
-	    rt.tv_nsec += 1000000000;
-	    --rt.tv_sec;
-	  }
-	// Did we already time out?
-	if (rt.tv_sec < 0)
-	  return false;
-
-	if (syscall (SYS_futex, __addr, futex_wait_op, __val, &rt) == -1)
+	rt.tv_sec = __s.count();
+	rt.tv_nsec = __ns.count();
+	if (syscall (SYS_futex, __addr, futex_wait_bitset_op | futex_clock_realtime_flag, __val, &rt, nullptr, futex_bitset_match_any) == -1)
 	  {
 	    _GLIBCXX_DEBUG_ASSERT(errno == EINTR || errno == EAGAIN
 				  || errno == ETIMEDOUT);
-- 
2.11.0

[RFC PATCH 3/4] libstdc++ atomic_futex: Use std::chrono::steady_clock as reference clock

[Mike Crowe]

The user-visible effect of this change is that std::future::wait_for now
uses std::chrono::steady_clock to determine the timeout. This makes it
immune to changes made to the system clock. It also means that anyone using
their own clock types with std::future::wait_until will have the timeout
converted to std::chrono::steady_clock rather than
std::chrono::system_clock.

Now that use of both std::chrono::steady_clock and
std::chrono::system_clock are correctly supported for the wait
timeout, I believe that std::chrono::steady_clock is a better choice
for the reference clock that is used by std::future::wait_for, and all
other clocks are converted to, since it is guaranteed to advance
steadily. The previous behaviour of converting to
std::chrono::system_clock risks timeouts changing dramatically when
the system clock is changed.

---
 libstdc++-v3/include/bits/atomic_futex.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
index c2b3df03592..7ba86dfeec7 100644
--- a/libstdc++-v3/include/bits/atomic_futex.h
+++ b/libstdc++-v3/include/bits/atomic_futex.h
@@ -71,7 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template <unsigned _Waiter_bit = 0x80000000>
   class __atomic_futex_unsigned : __atomic_futex_unsigned_base
   {
-    typedef chrono::system_clock __clock_t;
+    typedef chrono::steady_clock __clock_t;
 
     // This must be lock-free and at offset 0.
     atomic<unsigned> _M_data;
@@ -169,7 +169,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     unsigned
     _M_load_and_test_until_impl(unsigned __assumed, unsigned __operand,
 	bool __equal, memory_order __mo,
-	const chrono::time_point<__clock_t, _Dur>& __atime)
+	const chrono::time_point<std::chrono::system_clock, _Dur>& __atime)
     {
       auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
       auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
@@ -229,7 +229,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_load_when_equal_until(unsigned __val, memory_order __mo,
 	  const chrono::time_point<_Clock, _Duration>& __atime)
       {
-	// DR 887 - Sync unknown clock to known clock.
 	const typename _Clock::time_point __c_entry = _Clock::now();
 	const __clock_t::time_point __s_entry = __clock_t::now();
 	const auto __delta = __atime - __c_entry;
@@ -241,7 +240,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     template<typename _Duration>
     _GLIBCXX_ALWAYS_INLINE bool
     _M_load_when_equal_until(unsigned __val, memory_order __mo,
-	const chrono::time_point<__clock_t, _Duration>& __atime)
+	const chrono::time_point<std::chrono::system_clock, _Duration>& __atime)
     {
       unsigned __i = _M_load(__mo);
       if ((__i & ~_Waiter_bit) == __val)
-- 
2.11.0

[RFC PATCH 4/4] Extra async tests, not for merging

[Mike Crowe]

These tests show that changing the system clock has an effect on
std::future::wait_until when using std::chrono::system_clock but not when
using std::chrono::steady_clock. Unfortunately these tests have a number of
downsides:

1. Nothing that is attempting to keep the clock set correctly (ntpd,
   systemd-timesyncd) can be running at the same time.

2. The test process requires the CAP_SYS_TIME capability.

3. Other processes running concurrently may misbehave when the clock darts
   back and forth.

4. They are slow to run.

As such, I don't think they are suitable for merging. I include them here
because I wanted to document how I had tested the changes in the previous
commits.
---
 libstdc++-v3/testsuite/30_threads/async/async.cc | 76 ++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc
index 59905b4666f..0a3cb28fbda 100644
--- a/libstdc++-v3/testsuite/30_threads/async/async.cc
+++ b/libstdc++-v3/testsuite/30_threads/async/async.cc
@@ -25,6 +25,7 @@
 
 #include <future>
 #include <testsuite_hooks.h>
+#include <sys/time.h>
 
 using namespace std;
 
@@ -94,11 +95,86 @@ void test03()
   VERIFY( elapsed < std::chrono::seconds(20) );
 }
 
+void perturb_system_clock(const std::chrono::seconds &seconds)
+{
+  struct timeval tv;
+  if (gettimeofday(&tv, NULL))
+    abort();
+
+  tv.tv_sec += seconds.count();
+  if (settimeofday(&tv, NULL))
+    abort();
+}
+
+void work04()
+{
+  std::this_thread::sleep_for(std::chrono::seconds(10));
+}
+
+// Ensure that advancing CLOCK_REALTIME doesn't make any difference
+// when we're waiting on std::chrono::steady_clock.
+void test04()
+{
+  auto const start = chrono::steady_clock::now();
+  future<void> f1 = async(launch::async, &work04);
+
+  perturb_system_clock(chrono::seconds(20));
+
+  std::future_status status;
+  status = f1.wait_for(std::chrono::seconds(4));
+  VERIFY( status == std::future_status::timeout );
+
+  status = f1.wait_until(start + std::chrono::seconds(6));
+  VERIFY( status == std::future_status::timeout );
+
+  status = f1.wait_until(start + std::chrono::seconds(12));
+  VERIFY( status == std::future_status::ready );
+
+  auto const elapsed = chrono::steady_clock::now() - start;
+  VERIFY( elapsed >= std::chrono::seconds(10) );
+  VERIFY( elapsed < std::chrono::seconds(15) );
+
+  perturb_system_clock(chrono::seconds(-20));
+}
+
+void work05()
+{
+  std::this_thread::sleep_for(std::chrono::seconds(5));
+  perturb_system_clock(chrono::seconds(60));
+  std::this_thread::sleep_for(std::chrono::seconds(5));
+}
+
+// Ensure that advancing CLOCK_REALTIME does make a difference when
+// we're waiting on std::chrono::system_clock.
+void test05()
+{
+  auto const start = chrono::system_clock::now();
+  auto const start_steady = chrono::steady_clock::now();
+
+  future<void> f1 = async(launch::async, &work05);
+  future_status status;
+  status = f1.wait_until(start + std::chrono::seconds(60));
+  VERIFY( status == std::future_status::timeout );
+
+  auto const elapsed_steady = chrono::steady_clock::now() - start_steady;
+  VERIFY( elapsed_steady >= std::chrono::seconds(5) );
+  VERIFY( elapsed_steady < std::chrono::seconds(10) );
+
+  status = f1.wait_until(start + std::chrono::seconds(75));
+  VERIFY( status == std::future_status::ready );
+
+  perturb_system_clock(chrono::seconds(-60));
+}
+
 int main()
 {
   test01();
   test02();
   test03<std::chrono::system_clock>();
   test03<std::chrono::steady_clock>();
+  if (geteuid() == 0) {
+    test04();
+    test05();
+  }
   return 0;
 }
-- 
2.11.0