[PATCHv3,3/6] libstdc++ futex: Support waiting on std::chrono::steady_clock directly

Message ID 20180801131913.6576-4-mac@mcrowe.com
State New
Headers show
Series
  • std::future::wait_* improvements
Related show

Commit Message

Mike Crowe Aug. 1, 2018, 1:19 p.m.
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
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.)

FUTEX_WAIT_BITSET was added in kernel v2.6.25. If futex reports ENOSYS to
indicate that this operation is not supported then the code falls back to
using clock_gettime(2) to calculate a relative time to wait for.

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          | 79 ++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 1 deletion(-)

--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

Comments

Jonathan Wakely Aug. 1, 2018, 7:38 p.m. | #1
On 01/08/18 14:19 +0100, Mike Crowe wrote:
>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

>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.)

>

>FUTEX_WAIT_BITSET was added in kernel v2.6.25. If futex reports ENOSYS to

>indicate that this operation is not supported then the code falls back to

>using clock_gettime(2) to calculate a relative time to wait for.

>

>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.


Surely that would just be a programming error? Users aren't calling
these functions, and we only call them in a very limited number of
places, so the risk of calling the wrong one seems manageable. Just
don't do that.


>Unfortunately, doing this would break code

>that compiled against the old header.


We could add the new functions taking steady_clock::time_point and
system_clock::time_point, and as long as the existing function is also
still exported from the library nothing would break, would it?

We could make the existing function call the new one, or vice versa,
to avoid duplicating code.

I'm nto sure we actually want to do that, but I don't see why it
wouldn't be possible to do without breaking existing code.


>diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc

>index 72062a4285e..5c02f0f55ed 100644

>--- a/libstdc++-v3/src/c++11/futex.cc

>+++ b/libstdc++-v3/src/c++11/futex.cc

>@@ -118,6 +125,78 @@ _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

>+      {

>+       if (!futex_clock_monotonic_unavailable.load(std::memory_order_relaxed))

>+         {

>+           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 || errno == ENOSYS);

>+               if (errno == ETIMEDOUT)

>+                 return false;

>+               else if (errno == ENOSYS)

>+                 {

>+                   futex_clock_monotonic_unavailable.store(true, std::memory_order_relaxed);

>+                   // Fall through to legacy implementation if the system

>+                   // call is unavailable.

>+                 }

>+               else

>+                 return true;

>+             }

>+         }

>+

>+       // We only get to here if futex_clock_realtime_unavailable was


This should say futex_clock_monotonic_unavailable.

I'm also replacing the _GLIBCXX_DEBUG_ASSERT checks in that file with
__glibcxx_assert checks, because that file is never going to be
compiled with Debug Mode.
Mike Crowe Aug. 2, 2018, 11:20 a.m. | #2
On Wednesday 01 August 2018 at 20:38:33 +0100, Jonathan Wakely wrote:
> On 01/08/18 14:19 +0100, Mike Crowe wrote:

> > 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.

> 

> Surely that would just be a programming error?


Yes, but it's not one that the compiler can detect.

> Users aren't calling

> these functions, and we only call them in a very limited number of

> places, so the risk of calling the wrong one seems manageable. Just

> don't do that.


I see no reason not to use the type system to help avoid mistakes in the
internal impementation of the library as well as in its external interface.
But, as you say, the function is only called from a few places so there's
little reason to change it now.

> > diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc

[snip]
> > +       // We only get to here if futex_clock_realtime_unavailable was

> 

> This should say futex_clock_monotonic_unavailable.


Thanks. I've fixed that, but I'm not sure whether you want me to continue
to updating these patches or whether you plan to apply all the fixes
yourself before committing them.

Thanks.

Mike.

Patch

diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
index ecf5b02031a..47ecd329ea9 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 72062a4285e..5c02f0f55ed 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -33,9 +33,15 @@ 
 #include <errno.h>
 #include <debug/debug.h>

+#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
+#include <unistd.h>
+#include <sys/syscall.h>
+#endif
+
 // 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;
@@ -43,6 +49,7 @@  const unsigned futex_wake_op = 1;
 namespace
 {
   std::atomic<bool> futex_clock_realtime_unavailable;
+  std::atomic<bool> futex_clock_monotonic_unavailable;
 }

 namespace std _GLIBCXX_VISIBILITY(default)
@@ -118,6 +125,78 @@  _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
+      {
+       if (!futex_clock_monotonic_unavailable.load(std::memory_order_relaxed))
+         {
+           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 || errno == ENOSYS);
+               if (errno == ETIMEDOUT)
+                 return false;
+               else if (errno == ENOSYS)
+                 {
+                   futex_clock_monotonic_unavailable.store(true, std::memory_order_relaxed);
+                   // Fall through to legacy implementation if the system
+                   // call is unavailable.
+                 }
+               else
+                 return true;
+             }
+         }
+
+       // We only get to here if futex_clock_realtime_unavailable was
+       // true or has just been set to true.
+       struct timespec ts;
+#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
+       syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &ts);
+#else
+       clock_gettime(CLOCK_MONOTONIC, &ts);
+#endif
+       // Convert the absolute timeout value to a relative timeout
+       struct timespec rt;
+       rt.tv_sec = __s.count() - ts.tv_sec;
+       rt.tv_nsec = __ns.count() - ts.tv_nsec;
+       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)
+         {
+           _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)
   {