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. :-)

> 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.

Reply via email to