On Sunday 14 January 2018 at 20:44:10 +0000, Mike Crowe wrote:
> On Sunday 14 January 2018 at 16:08:09 +0000, Mike Crowe wrote:
> > Hi Torvald,
> >
> > Thanks for reviewing this change.
> >
> > On Saturday 13 January 2018 at 16:29:57 +0100, Torvald Riegel wrote:
> > > On Sun, 2018-01-07 at 20:55 +0000, Mike Crowe wrote:
> > > > 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.
> > >
> > > I have comments on the design.
> > >
> > > First, I don't think we should not change
> > > __atomic_futex_unsigned_base::_M_futex_wait_until, as there's a risk
> > > that we'll change behavior of existing applications that work as
> > > expected.
> >
> > I assume you mean "I don't think we should change" or "I think we should
> > not change"... :-)
> >
> > The only way I can see that behaviour will change for existing programs is
> > when the system clock changes (i.e. when someone calls settimeofday.) In
> > the existing code, the maximum wait time is fixed once gettimeofday is
> > called to calculate the relative timeout. When using FUTEX_CLOCK_REALTIME,
> > the maximum wait can change based on changes to the system clock after that
> > point. It appears that glibc made this transition successfully and
> > currently uses FUTEX_CLOCK_REALTIME. I think that the new behaviour is
> > better than the old behaviour.
> >
> > Or perhaps I've missed another possibility. Did you have another risk in
> > mind?
> >
> > > Instead, ISTM we should additionally expose the two options we have at
> > > the level of futexes:
> > > * Relative timeout using CLOCK_MONOTONIC
> > > * Absolute timeout using CLOCK_REALTIME (which will fall back to the
> > > former on old kernels, which is fine I think).
> > >
> > > Then we do the following translations from functions that programs would
> > > call to the new futex functions:
> > >
> > > 1) wait_for is a loop in which we load the current time from the steady
> > > clock, then call the relative futex wait, and if that returns for a
> > > spurious reason (ie, neither timeout nor is the expected value present),
> > > we reduce the prior relative amount by the difference between the time
> > > before the futex wait and the current time.
> >
> > If we're going to loop on a relative timeout it sounds safer to convert it
> > to an absolute (steady clock) timeout. That way we won't risk increasing
> > the timeout if the scheduler decides not to run us at an inopportune moment
> > between waits. _M_load_when_equal_for already does this.
> >
> > _M_load_and_test_until already has a loop for spurious wakeup. I think that
> > it makes sense to only loop at one level. That loop relies on the timeout
> > being absolute, which is why my _M_load_and_test_until_steady also uses an
> > absolute timeout.
> >
> > > 2) wait_until using the steady clock is a loop similar to wait_for, just
> > > that we additionally compute the initial relative timeout.
> >
> > Clearly an absolute wait can be implemented in terms of a relative one and
> > vice-versa, but at least in my attempts to write them I find the code
> > easier to understand (and therefore get right) if the fundamental wait is
> > the absolute one and the relative one is implemented on top of it.
>
> I had a quick go at implementing at least the first part of your design, as
> I understood it. (I've kept the loops inside atomic_futex_unsigned - and I
> think that you wanted to move them out to the client code.) I've not tested
> it much.
>
> I think that this implementation of _M_load_and_test_for is rather more
> error-prone than my previous _M_load_and_test_until_steady. That's probably
> partly because the type-safe duration has already been separated into seconds
> and nanoseconds. It would be nice to push this separation as deeply as
> possible in the code, but I'm afraid that would break ABI compatibility.
>
> Thanks.
>
> Mike.
>
> --8<--
>
> diff --git a/libstdc++-v3/include/bits/atomic_futex.h
> b/libstdc++-v3/include/bits/atomic_futex.h
> index ad9437da4e2..fa4a4382c79 100644
> --- a/libstdc++-v3/include/bits/atomic_futex.h
> +++ b/libstdc++-v3/include/bits/atomic_futex.h
> @@ -57,6 +57,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _M_futex_wait_until(unsigned *__addr, unsigned __val, bool __has_timeout,
> chrono::seconds __s, chrono::nanoseconds __ns);
>
> + // Returns false iff a timeout occurred.
> + bool
> + _M_futex_wait_for(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);
> };
> @@ -110,6 +115,40 @@ _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.
> + unsigned
> + _M_load_and_test_for(unsigned __assumed, unsigned __operand,
> + bool __equal, memory_order __mo, bool __has_timeout,
> + chrono::seconds __s, chrono::nanoseconds __ns)
> + {
> + auto __end_time = chrono::steady_clock::now() + 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_for((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;
> +
> + const auto __remaining = __end_time - chrono::steady_clock::now();
> + __s = chrono::duration_cast<chrono::seconds>(__remaining);
> + __ns = chrono::duration_cast<chrono::nanoseconds>(__remaining - __s);
> + }
> + return __assumed;
> + }
> +
> // 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
> @@ -168,8 +207,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _M_load_when_equal_for(unsigned __val, memory_order __mo,
> const chrono::duration<_Rep, _Period>& __rtime)
> {
> - return _M_load_when_equal_until(__val, __mo,
> - __clock_t::now() + __rtime);
> + unsigned __i = _M_load(__mo);
> + if ((__i & ~_Waiter_bit) == __val)
> + return true;
> + auto __s = chrono::duration_cast<chrono::seconds>(__rtime);
> + auto __ns = chrono::duration_cast<chrono::nanoseconds>(__rtime - __s);
> + __i = _M_load_and_test_for(__i, __val, true, __mo, true, __s, __ns);
> + return (__i & ~_Waiter_bit) == __val;
> }
>
> // Returns false iff a timeout occurred.
> diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
> index f5000aa77b3..1b5fe480209 100644
> --- a/libstdc++-v3/src/c++11/futex.cc
> +++ b/libstdc++-v3/src/c++11/futex.cc
> @@ -84,6 +84,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> }
> }
>
> + bool
> + __atomic_futex_unsigned_base::_M_futex_wait_for(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_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)
> {
> --
> 2.11.0
>
Hi Torvald and Jonathan,
Do you have any comments on my replies?
Thanks.
Mike.
