Re: [PATCH v3 3/6] nptl: Add POSIX-proposed pthread_cond_clockwait

[Mike Crowe <mac at mcrowe dot com>]

On Thursday 06 June 2019 at 09:53:59 -0300, Adhemerval Zanella wrote:
> 
> 
> On 27/05/2019 17:03, Mike Crowe wrote:
> > @@ -644,10 +647,39 @@ __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);
> > +
> > +  /* 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 clockid = (flags & __PTHREAD_COND_CLOCK_MONOTONIC_MASK)
> > +                    ? CLOCK_MONOTONIC : CLOCK_REALTIME;
> > +  return __pthread_cond_wait_common (cond, mutex, clockid, abstime);
> > +}
> > +
> 
> The relaxed atomic seems right, but I would like to a ack from another
> mantainer.

I remembered doing this based on a recommendation on this list. When I dug
up the email in question I was surprised to discover that it was you. :-)
So we still need a third pair of eyes.

> > +/* See __pthread_cond_wait_common.  */
> > +int
> > +__pthread_cond_clockwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> > +			  clockid_t clockid,
> > +			  const struct timespec *abstime)
> > +{
> > +  /* Check parameter validity.  This should also tell the compiler that
> > +     it can assume that abstime is not NULL.  */
> > +  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
> > +    return EINVAL;
> 
> Wouldn' t a  __nonnull ((X)) be better for this?

Well, I think that the check is important itself, and it just so happens to
have the side effect mentioned. I just copied the comment from
__pthread_cond_timedwait.

> > +
> > +  if (!futex_abstimed_supported_clockid (clockid))
> > +    return EINVAL;
> > +
> > +  /* If we do not support waiting using CLOCK_MONOTONIC, return an error.  */
> > +  if (clockid == CLOCK_MONOTONIC
> > +      && !futex_supports_exact_relative_timeouts ())
> > +    return EINVAL;
> 
> pthread_condattr_setclock returns ENOSUP for this case, should it does
> the same?

I discussed this with the Austin Group, and I believe that they were happy
with EINVAL.

I'd considered using ENOTSUP for valid but not supported clocks, and EINVAL
for invalid clocks. This would mean that the implementation would need to
keep a list of all clocks to check against, and I was worried that this
list could become out of date.

However, the only implementation of
futex_supports_exact_relative_timeouts() always returns true, so this code
never runs anyway.

(Maybe we should consider removing futex_supports_exact_relative_timeouts?)

Thanks.

Mike.