On Sat, Oct 6, 2018 at 12:09 PM Mike Crowe <theopengr...@mac.mcrowe.com> wrote:
>
> On Wednesday 12 September 2018 at 13:47:59 -0700, Tom Cherry wrote:
> > On Wed, Sep 12, 2018 at 11:56 AM Mike Crowe <theopengr...@mac.mcrowe.com>
> > wrote:
> > >
> > > On Wednesday 12 September 2018 at 11:29:26 -0700, Tom Cherry wrote:
> > > > 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.
> > >
> > > I had got the impression that people in the "enterprise" world wanted to
> > > continue to be able to wait on CLOCK_REALTIME. Maybe that's not as true as
> > > I thought it was.
> >
> > The existing pthread_cond_timedwait() would still use CLOCK_REALTIME
> > by default, so users would get to chose between those two clocks
> > depending on the function that they used. (Of course not on Android
> > though, given the workaround I described).
>
> Given that the Android _monotonic functions exist and solve the problem I'm
> trying to solve today, I'd be very happy to see them accepted (without
> their _np suffix) into POSIX.
>
> So, as an Austin Group newbie, I'm unsure what happens now. I'm optimistic
> that describing the proposed wording might help, so here's my attempt at
> that based on the new function being pthread_cond_timedwait_monotonic. I'd
> be more than happy to change it to something more like
> pthread_cond_timedwait_on_clock. I'm also not sure whether we'd want to
> make support for the function optional. I was unable to find the source
> format for the documentation, so what follows is based on the PDF of The
> Open Group Standard, Base Specifications, Issue 7 (c181):
>
> --8<--
> * After line 52743
>
> Insert
> "
> int pthread_cond_timedwait_monotonic(pthread_cond_t *restrict cond,
> pthread_mutex_t *restrict mutex,
> const struct timespec *restrict abstime);
> "
>
> * Lines 52747, 52762, 52763, 52766, 52781, 52783, 52786, 52848, 52882
>
> Add ", pthread_cond_timedwait_monotonic()" after "pthread_cond_timedwait()"
>
> * After line 52797 insert:
>
> Insert
>
> "
> The pthread_cond_timedwait_monotonic( ) function shall be equivalent to
> pthread_cond_timedwait( ), except that the absolute time specified by
> abstime is measured against the time returned by clock_get when passed the
> CLOCK_MONOTONIC clockid regardless of any clock attribute set on the
> condition variable.
> "
>
> * Line 52822:
>
> Change to "The pthread_cond_timedwait( ) and
> pthread_cond_timedwait_monotonic functions shall fail if:"
>
> * Line 52823, 52850:
>
> Change "pthread_cond_timedwait()" to "pthread_cond_timedwait() or
> pthread_cond_timedwait_monotonic()"
>
> * Insert after line 52880:
>
> "
> Choice of clock
>
> Care should be taken to decide which clock is most appropriate when waiting
> with a timeout. The system clock, CLOCK_REALTIME, as used by default with
> pthread_cond_timedwait() may be subject to jumps forwards and backwards in
> order to correct it against actual time. CLOCK_MONOTONIC is guaranteed not
> to jump backwards and must also advance in real time, so using it via
> pthread_cond_timedwait_monotonic() or pthread_condattr_setclock() may be
> more appropriate.
> "
> -->8--
>
> [snip]
>
> > The other function that I have seen used often and
> > that suffers from this issue is sem_timedwait(). It doesn't have the
> > option to use CLOCK_MONOTONIC during initialization either, so it is
> > particularly dangerous to use on systems where CLOCK_REALTIME may
> > jump.
>
> I've also invented sem_timedwaitonclock_np in order to fix some third-party
> code that was using sem_timedwait. Perhaps that function should be next on
> our list. :-)
bionic actually has sem_timedwait_monotonic_np() since earlier this
year --- Tom, the other Googler on this thread added it for
consistency.
Android's bionic libc currently has all of:
pthread_mutex_timedlock_monotonic_np()
pthread_cond_timedwait_monotonic_np()
pthread_rwlock_timedrdlock_monotonic_np()
pthread_rwlock_timedwrlock_monotonic_np()
sem_timedwait_monotonic_np()
though the top two were the "must have"s that have been around for years.
his checkin comment from then
(https://android-review.googlesource.com/633629) explains why we went
back and forth on pthread_cond_timedwait_monotonic_np() ourselves:
Note that pthread_cond_timedwait_monotonic_np() previously existed and
was removed since it's possible to initialize a condition variable to
use CLOCK_MONOTONIC. It is added back for a mix of reasons,
1) Symmetry with the rest of the functions we're adding
2) libc++ cannot easily take advantage of the new initializer, but
will be able to use this function in order to wait on
std::steady_clock
3) Frankly, it's a better API to specify the clock in the waiter function
than to specify the clock when the condition variable is
initialized.
> > Thanks for starting the conversation here. We've also been trying to
> > fix this problem for years.
>
> Thank you for your detailed responses and patch links. I'm sorry it's taken
> me so long to respond. I was waiting to see if there were any other responses.
>
> Thanks.
>
> Mike.
>
