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. > 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. For my embedded and desktop point of view, the system clock should not be trusted, can suddenly change in any direction and doesn't necessarily help identify a point in real time. If we assume that the non-standard clock is advancing steadily too, then steady_clock is a better match than system_clock. If you have a machine that has its system clock locked with PTP to an atomic clock then you might think the opposite. However, even in that situation you're reliant on steady_clock being reliable enough for short periods of time anyway, because that shares the same local clock as system_time. > Does this work for you? Not yet, but maybe there's parts that I don't fully understand the reasoning behind. > 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). I'm definitely willing to improve the current code rather than just add to it. Mike.
