On Sun, Sep 9, 2018 at 8:28 AM Mike Crowe <theopengr...@mac.mcrowe.com> wrote: > > On Thursday 30 August 2018 at 21:10:48 +0100, Mike Crowe wrote: > > C++11's std::condition_variable, std::timed_mutex and > > std::recursive_timed_mutex support waiting with a timeout specified using > > an arbitrary clock. It's common to use std::chrono::steady_clock (which > > corresponds to CLOCK_MONOTONIC) or std::chrono::system_clock (which > > corresponds to CLOCK_REALTIME) for these waits, and other clocks end up > > being converted to one of those when performing the wait. > > > > Unfortunately, I don't believe that it's possible to implement these on top > > of the pthread equivalents correctly for std::chrono::steady_clock (i.e. > > CLOCK_MONOTONIC) since: > > > > 1. pthread_mutex_timedlock only supports a time measured against > > CLOCK_REALTIME. > > > > 2. The clock used for pthread_cond_timedwait must be specified when the > > condition variable is created by pthread_cond_init. The clock to be used > > for future waits is not known at the point that the > > std::condition_variable constructor is called. > > > > I'm most interested in the std::condition_variable case. since I have code > > that wants to wait using std::chrono::steady_clock. > > > > There are a number of possible workarounds for these. I've described some > > in a blog post[1] and in defect 0001164[2]. But, my favourite solution is > > to introduce a variant of pthread_cond_timedwait that takes a an additional > > clockid_t parameter: > > > > int pthread_cond_timedwaitonclock(pthread_cond_t *cond, > > pthread_mutex_t *mutex, > > clockid_t clockid, > > const struct timespec *abstimeout); > > > > I've proposed[3] an implementation of this function for glibc (as > > pthread_cond_timedwaitonclock_np) and it was suggested that I ought to > > raise it here. (This led me to enter defect 0001164, but since that yielded > > no response I've finally got round to writing this email too.) > > > > The equivalent for mutex would probably be: > > > > int pthread_mutex_timedlockonclock(pthread_mutex_t *mutex, > > clockid_t clockid, > > const struct timespec *abstimeout); > > > > but I've not yet made any attempt to implement it in glibc. > > > > Would making the C++ standard library implementable on top of POSIX be > > considered a good enough reason to add such functions? > > > > Are these functions generic enough or perhaps too generic? Android had a > > pthread_cond_timedwait_monotonic function for a while, but deprecated it > > when they added support for pthread_condattr_setclock.
We (libc team for Android) have observed this problem too and have seen it cause issues on multiple occasions, enough times that we actually submitted a workaround that always converts the input timespec to CLOCK_MONOTONIC and waits with that clock[1]. Of course this only ameliorates the problem, but does not completely solve it, since there is still a period in time before the conversion to CLOCK_MONOTONIC happens, that if CLOCK_REALTIME changes abruptly, we're back to the same bad behavior as before. For this reason, we added pthread_cond_timedwait_monotonic_np() back in the P release of Android[2] along with equivalent functions for the rest of the timed waits. I've also pushed various patches to libc++, one that builds upon these changes to our libc[3] and one that implements std::condition_variable from scratch, borrowing the implementation from our libc[4], however neither made progress. As for having a separate clock id parameter, we thought about that during the review of the above changes, but we both thought that 1) there's no strong reason to wait on any clock other than CLOCK_MONOTONIC for these functions and 2) the underlying futex implementation only allows CLOCK_REALTIME and CLOCK_MONOTONIC, so we'd have to do conversions in userspace and inevitably wait on CLOCK_MONOTONIC anyway, so it's best to be clear and only give CLOCK_MONOTONIC as an option. > > > > Thanks. > > > > Mike. > > > > [1] > > https://randombitsofuselessinformation.blogspot.com/2018/06/its-about-time-monotonic-time.html > > [2] http://austingroupbugs.net/view.php?id=1164 > > [3] > > http://libc-alpha.sourceware.narkive.com/3ZPZzEOy/rfcv4-add-pthread-cond-timedwaitonclock-np#post1 > > I received one direct reply to this email from someone who wasn't sure if > their response was worth of the list. I thought it was. I understood their > main points to be: > > > The library code now has to track the clock for each invocation of > > pthread_cond_timedwaitonclock and not (once) per condition variable. > > Only if the underlying implementation expects that. Note: our implementation in Android does not need this, nor should any other implementations. It's simply a separate flag passed to the underlying futex call. It is possible to have different waiters wait on a futex each using different clock sources without issue as well. > > At least in the glibc implementation I've been working with, the clock to > use is a parameter to the underlying operating system futex wait operation. > Supplying it at construction time means that the clock must be stored > within the condition variable and retrieved when the wait operation is > called. I have implemented pthread_cond_timedwait in terms of > pthread_cond_timedwaitonclock. > > > The situations where a single condition variable (or mutex) has waiting > > threads that use different clocks would seem to be very rare; > > I agree, but the clock to use is a property of the wait and not a property > of the condition variable so it makes more sense (and it is easier to > reason about the code locally) if the clock is supplied in the call rather > than at construction. In C++ the clock is part of the type of the parameter > passed to the wait operation so it can be automatically inferred. > > > An application as a whole is interested in real time or in clock > > monotonic time. > > I don't believe that's true, especially when code is hidden away in > libraries. A calendar application may wish to use CLOCK_REALTIME for events > but CLOCK_MONOTONIC for notification timeouts. Realistically, there are better waiting mechanisms than condition variables that a calender application should use when waiting for events. We have broken this use case if anyone is trying to do it on Android given the above work around, but we believe the benefits outweigh the risk someone is attempting to do something like this. > > > pthread_mutex_timedlockonclock can be implemented using a condition > > variable configured to use an appropriate clock. > > It can be, if pthread_cond_timedwaitonclock is available, otherwise you're > back to not knowing the clock at construction time. I'm sceptical that > adding the overhead of a condition variable for every std::timed_mutex > instance will be acceptable to C++ standard library authors. Note that std::timed_mutex in libc++ is actually implemented as a combination of a std::mutex and a std::condition_variable currently. > > > Non-monotonic changes to CLOCK_REALTIME should be rare since methods > > exist to correct a slightly-wrong clock without it going backwards. > > Systems, particularly embedded ones, may boot without knowing the current > time and only set the system clock later. This may be quite a long time > later if they don't immediately have an Internet connection. A desktop user > may notice that their clock is completely wrong and take action to correct > it. We saw this happen on Android in real life use cases often enough that we had multiple bugs filed against it, which lead us to the above workaround that always uses CLOCK_MONOTONIC, regardless of what clock the user selected, so unfortunately it is a real issue. > > I know of several systems that have had real bugs due to using > CLOCK_REALTIME when they meant CLOCK_MONOTONIC. There has been a trend > among many Linux system libraries over the last few years away from using > CLOCK_REALTIME (often just via time(2) or gettimeofday(2)) towards using > CLOCK_MONOTONIC instead. > > Thanks. > > Mike. > Thanks Tom [1] https://android-review.googlesource.com/c/platform/bionic/+/460658 [2] https://android-review.googlesource.com/c/platform/bionic/+/633629 [3] https://reviews.llvm.org/D44945 [4] https://reviews.llvm.org/D36767
