Re: Adding a clockid_t parameter to functions that accept absolute timespecs
On Mon, Nov 26, 2018 at 10:20 AM Mike Crowe wrote: > > On Sunday 04 November 2018 at 10:32:38 +, Mike Crowe wrote: > > * Function naming: > > > > Several proposals have been made for naming: > > I only had one reply to the naming suggestions below, so I've combed > through prior emails looking for preferences there. I apologise if I missed > your preference. Please let me know! Results below. > > > ** 1. clock prefix > > sem_clock_timedwait > > pthread_mutex_clock_timedlock > > pthread_cond_clock_timedwait > > pthread_rwlock_clock_timedrdlock > > pthread_rwlock_clock_timedwrlock > > mq_clock_timedreceive > > mq_clock_timedsend > > Preferred by SC (in private email) > > > ** 2. timed->clock > > sem_clockwait > > pthread_mutex_clocklock > > pthread_rwlock_clockrdlock > > pthread_rwlock_clockwrlock > > mq_clockreceive > > mq_clocksend > > pthread_cond_clockwait > > Preferred by Mark Harris, Daniel Eischen. > > > ** 3. onclock suffix > > sem_timedwait_onclock > > pthread_mutex_timedlock_onclock > > pthread_rwlock_timedrdlock_onclock > > pthread_rwlock_timedwrlock_onclock > > mq_timedreceive_onclock > > mq_timedsend_onclock > > pthread_cond_timedwait_onclock > > Preferred by Tom Cherry (although I suspect that, like me, he's be happy > with either of the others too if it meant that the feature was added!) Yes, I'm fine with the others too if it gets these added. > > > All guidance gratefully appreciated. > > So, it looks like there isn't a clear favourite, but timed->clock has a > slight majority. I plan to enter a new defect report to supersede > http://austingroupbugs.net/view.php?id=1164 by covering all the above > functions. Sounds good to me. Thanks for driving this forward. > > Thanks. > > Mike.
Re: Adding a clockid_t parameter to functions that accept absolute timespecs (was Re: C++ condition_variable and timed_mutex with steady_clock and pthreads)
On Sat, Oct 27, 2018 at 5:16 PM Mark Harris wrote: > > On Sat, 27 Oct 2018 at 13:14, Mike Crowe wrote: >> >> On Saturday 27 October 2018 at 15:26:07 -0400, Daniel Eischen wrote: >> > >> > > On Oct 27, 2018, at 12:14 PM, Mike Crowe >> > > wrote: >> > > Looking through POSIX Base Specifications Issue 7, I >> > > believe that the following other functions lack a way to use >> > > CLOCK_MONOTONIC directly (and unlike pthread_cond_timedwait, I can see no >> > > other way to make them do so): >> > > >> > > sem_timedwait >> > > pthread_mutex_timedlock >> > > pthread_rwlock_timedrdlock >> > > pthread_rwlock_timedwrlock >> > > mq_timedreceive >> > > mq_timedsend >> > > posix_trace_timedgetnext_event The two message queue functions are directly implemented with a system call on Linux at least, so I'm not sure that we should make new versions of these unless we get the underlying system call to accept different clocks. I believe the posix tracing functions are marked as obsolescent, so it seems to be best to leave them alone as well. I agree with the creating these same variants for other four functions. >> > > >> > > (Many other functions that take a struct timespec treat it as a relative >> > > timeout.) Of these, I've also found it necessary to implement a version >> > > of >> > > sem_timedwait that used CLOCK_MONOTONIC. >> > > >> > > I originally named my function pthread_cond_timedwaitonclock since it >> > > seemed that running the words together matched what was happening with >> > > "timedwait". Others suggested that it should be >> > > pthread_cond_timedwait_on_clock. I've tried to apply that style in the >> > > function prototypes below. >> > >> > I think at least the "on_clock" should be "onclock" or just "clock" for >> > the same reason timedwait is not timed_wait. Perhaps "_cid" for clock_id? >> > >> > Also, "clock_" is prefixed to nanosleep for similar functionality, perhaps >> > pthread_clock_cond_timedwait, etc instead of suffixing with "_clock" or >> > "_onclock"? >> >> I think that the clock prefix has some benefits. This would mean: >> >> sem_clock_timedwait >> pthread_clock_mutex_timedlock >> pthread_clock_rwlock_timedrdlock >> pthread_clock_rwlock_timedwrlock >> mq_clock_timedreceive >> mq_clock_timedsend >> posix_clock_trace_timedgetnext_event >> pthread_clock_cond_timedwait >> >> Although, if you consider the prefix to contain multiple words then they >> would be: >> >> sem_clock_timedwait >> pthread_mutex_clock_timedlock >> pthread_rwlock_clock_timedrdlock >> pthread_rwlock_clock_timedwrlock >> mq_clock_timedreceive >> mq_clock_timedsend >> posix_trace_clock_timedgetnext_event >> pthread_cond_clock_timedwait > > > How about just replacing "timed" with "clock"? > > sem_clockwait > pthread_mutex_clocklock > pthread_rwlock_clockrdlock > pthread_rwlock_clockwrlock > mq_clockreceive > mq_clocksend > posix_trace_clockgetnext_event > pthread_cond_clockwait > > To me these names seem more appropriate, given that these calls wait until > the specified clock reaches the specified absolute value. If the clock can > be set, or if an implementation were to support using these with, say, > CPUTIME clocks, then "timed" does not seem correct since it is not the time > taken by the call but rather the behavior of the specified clock that is > significant. > > - Mark pthread_cond_timedwait_onclock() is my top suggestion. It makes it clear that this is a modification to the parent pthread_cond_timedwait() functions and that its semantics should be identical with the change that it waits on the provided clock as opposed to the one used during initialization. I agree with moving this forward to the next required step. We can trivially implement them in the Android libc.
Re: C++ condition_variable and timed_mutex with steady_clock and pthreads
On Tue, Oct 9, 2018 at 6:19 AM Mike Crowe wrote: > > On Monday 08 October 2018 at 22:06:10 -0400, Daniel Eischen wrote: > > > > > On Oct 8, 2018, at 1:43 PM, enh wrote: > > > > > >> On Sat, Oct 6, 2018 at 12:09 PM Mike Crowe > > >> wrote: > > >> > > >>> On Wednesday 12 September 2018 at 13:47:59 -0700, Tom Cherry wrote: > > >>>> On Wed, Sep 12, 2018 at 11:56 AM Mike Crowe > > >>>> wrote: > > >>>>> > > >>>>> We (libc team for Android) have observed this problem too and have > > >>>>> seen it cause issues on multiple occasions, enough times that we > > >>>>> actually submitted a workaround that always converts the input > > >>>>> timespec to CLOCK_MONOTONIC and waits with that clock[1]. Of course > > >>>>> this only ameliorates the problem, but does not completely solve it, > > >>>>> since there is still a period in time before the conversion to > > >>>>> CLOCK_MONOTONIC happens, that if CLOCK_REALTIME changes abruptly, > > >>>>> we're back to the same bad behavior as before. > > >>>>> > > >>>>> For this reason, we added pthread_cond_timedwait_monotonic_np() back > > >>>>> in the P release of Android[2] along with equivalent functions for the > > >>>>> rest of the timed waits. I've also pushed various patches to libc++, > > >>>>> one that builds upon these changes to our libc[3] and one that > > >>>>> implements std::condition_variable from scratch, borrowing the > > >>>>> implementation from our libc[4], however neither made progress. > > >>>>> > > >>>>> As for having a separate clock id parameter, we thought about that > > >>>>> during the review of the above changes, but we both thought that 1) > > >>>>> there's no strong reason to wait on any clock other than > > >>>>> CLOCK_MONOTONIC for these functions and 2) the underlying futex > > >>>>> implementation only allows CLOCK_REALTIME and CLOCK_MONOTONIC, so we'd > > >>>>> have to do conversions in userspace and inevitably wait on > > >>>>> CLOCK_MONOTONIC anyway, so it's best to be clear and only give > > >>>>> CLOCK_MONOTONIC as an option. > > >>>> > > > > I'd rather see the clock id passed in as it's more generally useful. By > > using _monotonic, it implies that there are only 2 clocks that matter, > > the default clock is widely chosen wrong, and that you can't change it on > > a per-thread basis, not that you want other clock options. That is literally what we intended when we added these functions. I'm biased here, but Linux and therefore Android only have two clocks that can be waited on: CLOCK_REALTIME and CLOCK_MONOTONIC. In our experience, developers virtually never intend on waiting on CLOCK_REALTIME despite that being the default option. > > The Linux futex system call currently has a single bit to switch between > CLOCK_REALTIME and CLOCK_MONOTONIC. There's nothing so stop it adding > support for other clocks, but doing so would require even more messing > about than futex already has. > > > I can't change the c++ standard, but it doesn't seem like an elegant > > solution to add a lot of duplicate _monotonic functions that serve only > > the monotonic clock. I admit, I am not familiar with the history behind > > this. > > The C++11 standard provides three clocks: system_clock, > high_resolution_clock and steady_clock. In libstdc++ the second is simply > an alias for the first. system_clock uses CLOCK_REALTIME and steady_clock > uses CLOCK_MONOTONIC. C++20 adds more clocks (but I suspect that libstdc++ > will end up treating most of them as aliases for system_clock too.) > > However, there's nothing to stop implementations or even client code > creating new clocks. Time points measured against these clocks can be > passed to std::condition_variable::wait_until etc. The current > implementations convert these time points to system_clock (slightly > inaccurately since it's not possible to get the current time on two > different clocks at exactly the same time.) With proper support for > CLOCK_MONOTONIC, I think that conversion to steady_clock would be more > appropriate. > > > It is conceivable that you could wait on more than just the real-time and > > monotonic clocks. FreeB
Re: C++ condition_variable and timed_mutex with steady_clock and pthreads
On Sun, Oct 7, 2018 at 6:09 AM Mike Crowe wrote: > On Saturday 06 October 2018 at 17:43:06 -0400, Daniel Eischen wrote: > > > > > On Oct 6, 2018, at 3:00 PM, Mike Crowe > wrote: > > > > > > Thank you for your detailed responses and patch links. I'm sorry it's > taken > > > me so long to respond. I was waiting to see if there were any other > responses. > > > > Aside from what may be needed by the implementation for the proposed C++ > > standard functionality, I don't understand why initializing a mutex or > > condition variable (via the attribute) with the desired clock is not > > sufficient. I can see adding an analogue to pthread_condattr_setclock() > > for a mutex attribute, but what is the real need to provide both > > real-time and monotonic sleeps on the same mutex or condition variable? > > Initialize them once with the clock you want, and be done with it. > > For application code written to talk directly to the pthread functions, I > agree. But for a library, like the C++ standard library, that wishes to > make the clock to use part of the timeout then this is not possible. > > If the choice of clock is distant from the wait (perhaps the condition > variable is passed into or out of library code) then there is a higher risk > of the wrong clock being accidentally used, and there is no way to detect > that automatically at compile time or run time[1]. Libraries, such as the > C++ library, completely avoid that class of bugs at compile time using > their type system. A C library could too, if it so wished, but it would > require separate functions to accept the different clock types. > +1 to this. As I wrote in my commit for Android, I frankly think it's a better interface to have the clock provided at the same point that the timespec is provided. It makes it harder to erroneously provide a timespec for a different clock than the one used to initialize the condition variable. One other reason why the pthread_condattr_setclock() function is not enough to solve the C++ use case, is that the constructor for std::condition_variable is defined to be constexpr, so it cannot call any API functions such as pthread_condattr_setclock(). This could be solved via a condition variable initializer #define that defaults to using the monotonic clock. We also created this in Android in our quest to solve this issue across API levels [1]. I fully understand if that is too C++ specific for this list to be interested in though. > > > I'd almost rather see the timespec changed (controversial much?!) to add > > the clock id rather than keep adding new interfaces to support > > waits/sleeps on different clocks. > > I think that if we were starting from scratch then that wouldn't be a bad > plan, and it would support the C++ library use case. I can't see it > happening now though. > Agreed here too that this would be ideal but likely impossible at this current point. > > > Or maybe to provide a hint to the implementation as to what clock to use > > on a per-thread basis via something like pthread_attr_setpreferredclock() > > and pthread_setpreferredclock(). If you had these, I suppose you could > > implement the C++ functionality by saving the current thread's preferred > > clock, setting it to the desired clock, calling the mutex lock or CV wait > > function, then restoring the thread's clock before returning to the > > caller. > > That all seems like a lot of overhead of messing about with TLS just to > influence a single flag passed to the syscall, that may not even end up > needing to be called. I imagine that the overhead of passing a single extra > parameter to a function is tiny compared to that, especially when the > thread's preferred clock is not already in the data cache. > Agreed. That is a novel solution but seems overall more complex without a benefit than simply passing an additional clock argument or having this separate set of monotonic waiter functions. > > Thanks. > > Mike. > > [1] Well, I suppose on platforms that have 64-bit time_t, a high bit could > always be set for CLOCK_MONOTONIC which would allow detection at run time. > I'm also new to this list, so I can't help with the verbiage, but it looks good to me. Thanks Tom [1] https://android-review.googlesource.com/c/platform/bionic/+/349297/
Re: C++ condition_variable and timed_mutex with steady_clock and pthreads
On Wed, Sep 12, 2018 at 11:56 AM Mike Crowe wrote: > > On Wednesday 12 September 2018 at 11:29:26 -0700, Tom Cherry wrote: > > On Sun, Sep 9, 2018 at 8:28 AM Mike Crowe > > wrote: > > > > > > On Thursday 30 August 2018 at 21:10:48 +0100, Mike Crowe wrote: > > > > C++11's std::condition_variable, std::timed_mutex and > > > > std::recursive_timed_mutex support waiting with a timeout specified > > > > using > > > > an arbitrary clock. It's common to use std::chrono::steady_clock (which > > > > corresponds to CLOCK_MONOTONIC) or std::chrono::system_clock (which > > > > corresponds to CLOCK_REALTIME) for these waits, and other clocks end up > > > > being converted to one of those when performing the wait. > > > > > > > > Unfortunately, I don't believe that it's possible to implement these on > > > > top > > > > of the pthread equivalents correctly for std::chrono::steady_clock (i.e. > > > > CLOCK_MONOTONIC) since: > > > > > > > > 1. pthread_mutex_timedlock only supports a time measured against > > > >CLOCK_REALTIME. > > > > > > > > 2. The clock used for pthread_cond_timedwait must be specified when the > > > >condition variable is created by pthread_cond_init. The clock to be > > > > used > > > >for future waits is not known at the point that the > > > >std::condition_variable constructor is called. > > > > > > > > I'm most interested in the std::condition_variable case. since I have > > > > code > > > > that wants to wait using std::chrono::steady_clock. > > > > > > > > There are a number of possible workarounds for these. I've described > > > > some > > > > in a blog post[1] and in defect 0001164[2]. But, my favourite solution > > > > is > > > > to introduce a variant of pthread_cond_timedwait that takes a an > > > > additional > > > > clockid_t parameter: > > > > > > > > int pthread_cond_timedwaitonclock(pthread_cond_t *cond, > > > >pthread_mutex_t *mutex, > > > >clockid_t clockid, > > > >const struct timespec *abstimeout); > > > > > > > > I've proposed[3] an implementation of this function for glibc (as > > > > pthread_cond_timedwaitonclock_np) and it was suggested that I ought to > > > > raise it here. (This led me to enter defect 0001164, but since that > > > > yielded > > > > no response I've finally got round to writing this email too.) > > > > > > > > The equivalent for mutex would probably be: > > > > > > > > int pthread_mutex_timedlockonclock(pthread_mutex_t *mutex, > > > > clockid_t clockid, > > > > const struct timespec *abstimeout); > > > > > > > > but I've not yet made any attempt to implement it in glibc. > > > > > > > > Would making the C++ standard library implementable on top of POSIX be > > > > considered a good enough reason to add such functions? > > > > > > > > Are these functions generic enough or perhaps too generic? Android had a > > > > pthread_cond_timedwait_monotonic function for a while, but deprecated it > > > > when they added support for pthread_condattr_setclock. > > > > We (libc team for Android) have observed this problem too and have > > seen it cause issues on multiple occasions, enough times that we > > actually submitted a workaround that always converts the input > > timespec to CLOCK_MONOTONIC and waits with that clock[1]. Of course > > this only ameliorates the problem, but does not completely solve it, > > since there is still a period in time before the conversion to > > CLOCK_MONOTONIC happens, that if CLOCK_REALTIME changes abruptly, > > we're back to the same bad behavior as before. > > > > For this reason, we added pthread_cond_timedwait_monotonic_np() back > > in the P release of Android[2] along with equivalent functions for the > > rest of the timed waits. I've also pushed various patches to libc++, > > one that builds upon these changes to our libc[3] and one that > > implements std::condition_variable from scratch, borrowing the > > implementation from our libc[4], howeve
Re: C++ condition_variable and timed_mutex with steady_clock and pthreads
On Sun, Sep 9, 2018 at 8:28 AM Mike Crowe wrote: > > On Thursday 30 August 2018 at 21:10:48 +0100, Mike Crowe wrote: > > C++11's std::condition_variable, std::timed_mutex and > > std::recursive_timed_mutex support waiting with a timeout specified using > > an arbitrary clock. It's common to use std::chrono::steady_clock (which > > corresponds to CLOCK_MONOTONIC) or std::chrono::system_clock (which > > corresponds to CLOCK_REALTIME) for these waits, and other clocks end up > > being converted to one of those when performing the wait. > > > > Unfortunately, I don't believe that it's possible to implement these on top > > of the pthread equivalents correctly for std::chrono::steady_clock (i.e. > > CLOCK_MONOTONIC) since: > > > > 1. pthread_mutex_timedlock only supports a time measured against > >CLOCK_REALTIME. > > > > 2. The clock used for pthread_cond_timedwait must be specified when the > >condition variable is created by pthread_cond_init. The clock to be used > >for future waits is not known at the point that the > >std::condition_variable constructor is called. > > > > I'm most interested in the std::condition_variable case. since I have code > > that wants to wait using std::chrono::steady_clock. > > > > There are a number of possible workarounds for these. I've described some > > in a blog post[1] and in defect 0001164[2]. But, my favourite solution is > > to introduce a variant of pthread_cond_timedwait that takes a an additional > > clockid_t parameter: > > > > int pthread_cond_timedwaitonclock(pthread_cond_t *cond, > >pthread_mutex_t *mutex, > >clockid_t clockid, > >const struct timespec *abstimeout); > > > > I've proposed[3] an implementation of this function for glibc (as > > pthread_cond_timedwaitonclock_np) and it was suggested that I ought to > > raise it here. (This led me to enter defect 0001164, but since that yielded > > no response I've finally got round to writing this email too.) > > > > The equivalent for mutex would probably be: > > > > int pthread_mutex_timedlockonclock(pthread_mutex_t *mutex, > > clockid_t clockid, > > const struct timespec *abstimeout); > > > > but I've not yet made any attempt to implement it in glibc. > > > > Would making the C++ standard library implementable on top of POSIX be > > considered a good enough reason to add such functions? > > > > Are these functions generic enough or perhaps too generic? Android had a > > pthread_cond_timedwait_monotonic function for a while, but deprecated it > > when they added support for pthread_condattr_setclock. We (libc team for Android) have observed this problem too and have seen it cause issues on multiple occasions, enough times that we actually submitted a workaround that always converts the input timespec to CLOCK_MONOTONIC and waits with that clock[1]. Of course this only ameliorates the problem, but does not completely solve it, since there is still a period in time before the conversion to CLOCK_MONOTONIC happens, that if CLOCK_REALTIME changes abruptly, we're back to the same bad behavior as before. For this reason, we added pthread_cond_timedwait_monotonic_np() back in the P release of Android[2] along with equivalent functions for the rest of the timed waits. I've also pushed various patches to libc++, one that builds upon these changes to our libc[3] and one that implements std::condition_variable from scratch, borrowing the implementation from our libc[4], however neither made progress. As for having a separate clock id parameter, we thought about that during the review of the above changes, but we both thought that 1) there's no strong reason to wait on any clock other than CLOCK_MONOTONIC for these functions and 2) the underlying futex implementation only allows CLOCK_REALTIME and CLOCK_MONOTONIC, so we'd have to do conversions in userspace and inevitably wait on CLOCK_MONOTONIC anyway, so it's best to be clear and only give CLOCK_MONOTONIC as an option. > > > > Thanks. > > > > Mike. > > > > [1] > > https://randombitsofuselessinformation.blogspot.com/2018/06/its-about-time-monotonic-time.html > > [2] http://austingroupbugs.net/view.php?id=1164 > > [3] > > http://libc-alpha.sourceware.narkive.com/3ZPZzEOy/rfcv4-add-pthread-cond-timedwaitonclock-np#post1 > > I received one direct reply to this email from someone who wasn't sure if > their response was worth of the list. I thought it was. I understood their > main points to be: > > > The library code now has to track the clock for each invocation of > > pthread_cond_timedwaitonclock and not (once) per condition variable. > > Only if the underlying implementation expects that. Note: our implementation in Android does not need this, nor should any other implementations. It's simply a separate flag passed to the underlying futex call. It is possible to have d
