public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
From: Torvald Riegel <triegel@redhat.com>
To: Mike Crowe <mac@mcrowe.com>
Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org,
	Jonathan Wakely	 <jwakely@redhat.com>
Subject: Re: [PATCH 0/5] Make std::future::wait_* use std::chrono::steady_clock when required
Date: Sat, 13 Jan 2018 15:30:00 -0000	[thread overview]
Message-ID: <1515857397.4439.189.camel@redhat.com> (raw)
In-Reply-To: <20180107205532.13138-1-mac@mcrowe.com>

On Sun, 2018-01-07 at 20:55 +0000, Mike Crowe wrote:
> This patch series was originally submitted back in September at
> https://gcc.gnu.org/ml/libstdc++/2017-09/msg00083.html which ended up
> as https://patchwork.ozlabs.org/cover/817379/ . The patches received
> no comments at all, which may mean that they are perfect or that they
> are so bad that no-one knew where to start with criticising them. It
> would be good to know which. :)
> 
> The patches are unchanged from last time, but I have corrected a typo
> in one of the commit messages. The original cover message follows.
> 
> 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.
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.

2) wait_until using the steady clock is a loop similar to wait_for, just
that we additionally compute the initial relative timeout.

3) wait_until using the system clock is a loop that uses
absolute-timeout futex wait.

4) For wait_until using an unknown clock, I'd say that synching to the
system clock is the right approach.  Using wait_until indicates that the
programmer wanted to have a point in time, not a duration.

Does this work for you?

If so, could you provide a revised patch that uses this approach and
includes this approach in the documentation?
(Sorry for the lack of comments in the current code).

  parent reply	other threads:[~2018-01-13 15:30 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-07 20:55 Mike Crowe
2018-01-07 20:55 ` [PATCH 3/5] libstdc++ futex: Support waiting on std::chrono::steady_clock directly Mike Crowe
2018-01-07 20:55 ` [PATCH 1/5] Improve libstdc++-v3 async test Mike Crowe
2018-01-09 14:31   ` Jonathan Wakely
2018-01-07 20:55 ` [PATCH 5/5] Extra async tests, not for merging Mike Crowe
2018-01-07 20:55 ` [PATCH 2/5] libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait Mike Crowe
2018-01-09 13:50   ` Jonathan Wakely
2018-01-09 17:54     ` Mike Crowe
2018-01-12 13:18       ` Torvald Riegel
2018-01-12 14:49         ` Jonathan Wakely
2018-01-12 17:56         ` Joseph Myers
2018-01-07 20:55 ` [PATCH 4/5] libstdc++ atomic_futex: Use std::chrono::steady_clock as reference clock Mike Crowe
2018-01-09 14:43 ` [PATCH 0/5] Make std::future::wait_* use std::chrono::steady_clock when required Jonathan Wakely
2018-01-13 15:30 ` Torvald Riegel [this message]
2018-01-14 16:08   ` Mike Crowe
2018-01-14 20:44     ` Mike Crowe
2018-02-14 16:32       ` Mike Crowe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1515857397.4439.189.camel@redhat.com \
    --to=triegel@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jwakely@redhat.com \
    --cc=libstdc++@gcc.gnu.org \
    --cc=mac@mcrowe.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).