Re: C++ condition_variable and timed_mutex with steady_clock and pthreads

2018-09-12 Thread Mike Crowe
On Wednesday 12 September 2018 at 11:29:26 -0700, Tom Cherry wrote:
> On Sun, Sep 9, 2018 at 8:28 AM Mike Crowe  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.

[snip]

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

Re: C++ condition_variable and timed_mutex with steady_clock and pthreads

2018-09-12 Thread Tom Cherry
On Wed, Sep 12, 2018 at 11:56 AM Mike Crowe  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  
> > 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 

Re: C++ condition_variable and timed_mutex with steady_clock and pthreads

2018-09-12 Thread Tom Cherry
On Sun, Sep 9, 2018 at 8:28 AM Mike Crowe  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