This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFCv4] Add pthread_cond_timedwaitonclock_np



On 30/09/2017 13:01, Mike Crowe wrote:
>> 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.]

Thanks, I think this is the best way forward, although it might require
some time to get Austin group any reply.  If and once we get a positive
indication for API inclusion, we speed up by adding as a GNU extension
for upcoming releases and later adding in the default API.

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

This is an GLIB implementation detail, where some libraries which
are expected to be usable without explicit linking to libpthread use
some pthread functions (for instance aio from librt). 

In the single thread case libc.so uses wrappers that may or not call the
libpthread.so implementation (nptl/forward.c) if libpthread.so.0 is
loaded (by dlopen for instance).

So since there is no current use of pthread_cond_timedwaitonclock_np,
there is no requirement to add it on libc.so. 

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

I think you can add a comment stating that since it is undefined behaviour
to call pthread_mutex_timedwait with a cond or mutex argument which has not
been initialized, the __wrefs bits used for clockid is not expected to be
set concurrently and so it should be safe to use a relaxed load in this
case.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]