This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]