Re: [RFCv4] Add pthread_cond_timedwaitonclock_np

[Mike Crowe <mac at mcrowe dot com>]

> On 22/06/2017 14:05, Mike Crowe wrote:
> > C++11's std::condition_variable::wait_until specifies the clock to be used
> > at the time of the wait and permits separate waits on the same condition
> > variable instance using different clocks.
> > 
> > Unfortunately pthread_cond_timedwait always uses the clock that was
> > specified (or defaulted) when pthread_cond_init was called. libstdc++'s
> > current implementation converts all waits to
> > std::chrono::system_clock (i.e. CLOCK_REALTIME) which can race against
> > the system clock changing.
> > 
> > Let's invent a brand-new function pthread_cond_timedwaitonclock_np which
> > accepts both the clock and the time point as parameters is straightforward
> > and means that the C++11 standard behaviour can be implemented in libstdc++
> > on Linux at least.
> > 
> > Earlier versions of this patch were proposed in 2015 but further progress
> > stalled waiting for the assembly implementation of pthread_cond_timedwait.S
> > to be removed by Torvald Riegel. This happened in
> > ed19993b5b0d05d62cc883571519a67dae481a14.
> > 
> > See the following threads for previous versions and discussion:
> > 
> > * https://sourceware.org/ml/libc-alpha/2015-07/msg00193.html
> > * https://sourceware.org/ml/libc-alpha/2015-08/msg00186.html
> > * https://sourceware.org/ml/libc-alpha/2015-08/msg00230.html
> > 
> > Some of the abilist updates for other architectures appear to have gone
> > missing during the rebase. If this change is reviewed favourably then I
> > shall try to include them.

On Wednesday 28 June 2017 at 11:39:40 -0300, Adhemerval Zanella wrote:
> I think it is a reasonable addition, however I have some concerns if this
> would just another bad discussed ABI to fix and specific issues that will
> be replaced by another better or simple solution (as your last comment in
> BZ#41861 [1]).  It also concerns that it was not brought to Austin Group
> attention as suggested by Szabolcs [2].

I've now submitted the suggestion. It can be viewed at
http://austingroupbugs.net/view.php?id=1164

[Snipped uncontentious parts of patch and suggestions for brevity.]

> > diff --git a/nptl/Versions b/nptl/Versions
> > index 0ae5def464..1cf8c2fbad 100644
> > --- a/nptl/Versions
> > +++ b/nptl/Versions
> > @@ -28,6 +28,9 @@ libc {
> >      pthread_cond_wait; pthread_cond_signal;
> >      pthread_cond_broadcast; pthread_cond_timedwait;
> >    }
> > +  GLIBC_2.26 {
> > +    pthread_cond_timedwaitonclock_np;
> > +  }
> 
> The libc addition seems wrong, whhy this is required?

pthread_cond_timedwait is in libc, so I followed suite and added
pthread_cond_timedwaitonclock_np too. I must admit that I can't quite see
how the dummy empty implementations of the pthread functions end up in
libc, but they apparently do according to:

https://stackoverflow.com/questions/11161462/why-glibc-and-pthread-library-both-defined-same-apis

Maybe this means that I've missed something.

> >  /* See __pthread_cond_wait_common.  */
> > @@ -664,10 +665,31 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> >       it can assume that abstime is not NULL.  */
> >    if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
> >      return EINVAL;
> > -  return __pthread_cond_wait_common (cond, mutex, abstime);
> > +
> > +  clockid_t clockid = ((cond->__data.__wrefs & __PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0) ? CLOCK_MONOTONIC : CLOCK_REALTIME;
> > +  return __pthread_cond_wait_common (cond, mutex, clockid, abstime);
> > +}
> 
> Line to long and I think it is required to state why a plain load to
> __wrefs in this specific case is suffice for synchronization.  In fact
> I think it would be better to just use a relaxed MO:
> 
>     /* Relaxed MO is suffice because clock ID bit is only modified
>        in condition creation.  */
>     unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs);
>     clockid_t clock = (flags & __PTHREAD_COND_CLOCK_MONOTONIC_MASK)
>                       ? CLOCK_MONOTONIC : CLOCK_REALTIME;
>     return __pthread_cond_wait_common (cond, mutex, clock, abstime);

OK. I too was worried about that load, but persuaded myself that it was
safe for the same reasons. I think you're correct that it should be made
explicit and explained.

Thanks again for your review.

Mike.