Re: [RFCv4] Add pthread_cond_timedwaitonclock_np

[Mike Crowe <mac at mcrowe dot com>]

Apologies for the slow reply. I seem to have somehow missed your response
on libc-alpha. :(

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

Comment 14 of BZ#41861 proposed two solutions, but I believe that they both
have downsides:

1. Write a new condition variable implementation in libstdc++ on top of
futex(2). Since I wrote that comment I've become aware that libstdc++ has
gained an atomic_futex implementation that could be used. It currently
falls back to using std::condition_variable when futex isn't available so
that would need to be changed before it could be used as the basis for
std::condition_variable itself.

2. Make std::condition_variable use CLOCK_MONOTONIC by default and convert
to that from CLOCK_REALTIME when passed std::chrono::system_clock. I'm
surprised this wasn't done several years ago, since I think it is
preferable to the current situation. However, it's not perfect since a wait
on std::chrono::system_clock will not react to changes made to the clock
during the wait. The current implementation results in FUTEX_CLOCK_REALTIME
being used which does react to changes to the system clock during the wait,
so this solution would change the behaviour of such code.

> It also concerns that it was not brought to Austin Group
> attention as suggested by Szabolcs [2].

I shall try to work out how I should go about doing that.

> Torvald, do you think a new API to specific a clock is a reasonable addition
> for GLIBC? Rich, any thought about this new API (any possible musl inclusion
> in nearby future in case of glibc acceptance)? Do you think we should
> approach Austin Group first? The issue with C++ seems to a QoI one that is
> currently defined afaik as implementation-defined, so it would be an
> enhancement

The lack of reliable waits using a steady clock is a problem for embedded
systems in particular because the system clock may be set at any time. In
the past we used to not even attempt to set the system clock to the real
time because so many libraries would misbehave when it changed. But,
gradually, most code has been fixed to use CLOCK_MONOTONIC so we now do so.
But we're left using our own std::condition_variable variant now (that
basically implements solution 2 above) in order to avoid the problem in C++
code.

> The patch itself has some minor issues about formatting that I outlined
> below.

Thanks for the review. I shall try to incorporate your suggestions and post
a new version.

Mike.