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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 18/06/2019 10:53, Mike Crowe wrote:
> 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.

Torvald, do you think a relaxed load here would be right?

> 
>>> +/* 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.

The check is required indeed, I was referring to also add __nonnull attribute
on internal __pthread_cond_clockwait signature.

> 
>>> +
>>> +  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?)

My understanding is that NPTL is a Linux specific ABI, but it seems that
recent work from Torvald to try to decouple it and make it more a generic
interface. I am not sure if the extra-complexity does bring any gain, it
does make sense in the way the code is structured (where Linux specific
is add on sysdeps/unix/sysv/linux/).

So I do not see an issue in adding extra requirements (such as assume
futex_supports_exact_relative_timeouts) to generic futex.

> 
> Thanks.
> 
> Mike.
>