On Sun, Oct 7, 2018 at 6:09 AM Mike Crowe <theopengr...@mac.mcrowe.com> wrote:
> On Saturday 06 October 2018 at 17:43:06 -0400, Daniel Eischen wrote: > > > > > On Oct 6, 2018, at 3:00 PM, Mike Crowe <theopengr...@mac.mcrowe.com> > 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/
