[PATCH 3/7] nptl: Add POSIX-proposed pthread_cond_clockwait
Mike Crowe
mac@mcrowe.com
Sat May 4 20:23:00 GMT 2019
On Tuesday 05 March 2019 at 13:45:13 -0300, Adhemerval Zanella wrote:
>
>
> On 27/02/2019 15:23, Mike Crowe wrote:
> > Add:
> >
> > int pthread_cond_clockwait (pthread_cond_t *cond,
> > pthread_mutex_t *mutex,
> > clockid_t clockid,
> > const struct timespec *abstime)
> >
> > which behaves just like pthread_cond_timedwait except it always measures
> > abstime against the supplied clockid. Currently supports CLOCK_REALTIME and
> > CLOCK_MONOTONIC and returns EINVAL if any other clock is specified.
> >
> > Includes feedback from many others. This function was originally
> > proposed[1] as pthread_cond_timedwaitonclock_np, but The Austin Group
> > preferred the new name.
> >
> > * nptl/Makefile: Add tst-cond26 and tst-cond27
> >
> > * nptl/Versions (GLIBC_2.30): Add pthread_cond_clockwait
> >
> > * sysdeps/nptl/pthread.h: Likewise
> >
> > * nptl/forward.c: Add __pthread_cond_clockwait (not sure if it should be
> > __USE_GNU while it's still only proposed for POSIX.)
> >
> > * nptl/forward.c: Likewise
> >
> > * nptl/pthreadP.h: Likewise
> >
> > * sysdeps/nptl/pthread-functions.h: Likewise.
> >
> > * nptl/pthread_cond_wait.c (__pthread_cond_wait_common): Add clockid
> > parameter and comment describing why we don't need to check its value.
> > Use that value rather than reading the clock from the flags.
> > (__pthread_cond_wait): Pass unused clockid parameter.
> > (__pthread_cond_timedwait): Read clock from flags and pass it to
> > __pthread_cond_wait_common. (__pthread_cond_clockwait): Add new function
> > with weak alias from pthread_cond_clockwait.
> >
> > * nptl/tst-cond11.c (run_test): Support testing pthread_cond_clockwait too
> > by using a special magic CLOCK_USE_ATTR_CLOCK value to determine whether
> > to call pthread_cond_timedwait or pthread_cond_clockwait. (do_test): Pass
> > CLOCK_USE_ATTR_CLOCK for existing tests, and add new tests using all
> > combinations of CLOCK_MONOTONIC and CLOCK_REALTIME.
> >
> > * ntpl/tst-cond26.c: New test for passing unsupported and invalid clocks to
> > pthread_cond_clockwait.
> >
> > * nptl/tst-cond27.c: Add test similar to tst-cond5.c, but using struct
> > timespec and pthread_cond_clockwait.
> >
> > * sysdeps/unix/sysv/linux/arm/libpthread.abilist,
> > sysdeps/unix/sysv/linux/i386/libpthread.abilist,
> > sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist: Add
> > pthread_cond_clockwait
> >
> > * manual/threads.texi: Document pthread_cond_clockwait. The comment was
> > provided by Carlos O'Donell.
> >
> > [1] https://sourceware.org/ml/libc-alpha/2015-07/msg00193.html
> > ---
> > manual/threads.texi | 20 ++-
> > nptl/Makefile | 1 +-
> > nptl/Versions | 2 +-
> > nptl/forward.c | 5 +-
> > nptl/nptl-init.c | 1 +-
> > nptl/pthreadP.h | 4 +-
> > nptl/pthread_cond_wait.c | 43 ++++-
> > nptl/tst-cond11.c | 30 ++-
> > nptl/tst-cond26.c | 91 ++++++++++-
> > nptl/tst-cond27.c | 113 ++++++++++++-
> > sysdeps/nptl/pthread-functions.h | 4 +-
> > sysdeps/nptl/pthread.h | 13 +-
> > sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist | 1 +-
> > 13 files changed, 314 insertions(+), 14 deletions(-)
> > create mode 100644 nptl/tst-cond26.c
> > create mode 100644 nptl/tst-cond27.c
[snip]
> > diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
> > index daa4e25..5deb54b 100644
> > --- a/nptl/pthread_cond_wait.c
> > +++ b/nptl/pthread_cond_wait.c
[snip]
> > @@ -664,10 +671,40 @@ __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);
> > +}
> > +
>
> Ok.
>
> > +/* 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;
>
> The timespec check is used in different parts internally, I wonder
> if it would better to consolidate it somewhere.
Yes, I think some sort of timespec_valid function would make sense. Where
would be a good place to put it?
> > +
> > + /* We only support CLOCK_REALTIME and CLOCK_MONOTONIC */
> > + if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC)
> > + return EINVAL;
>
> Why not use lll_futex_supported_clockid here?
In fact, it should be futex_abstimed_supported_clockid.
> > +
> > + /* If we do not support waiting using CLOCK_MONOTONIC, return an error. */
> > + if (clockid == CLOCK_MONOTONIC
> > + && !futex_supports_exact_relative_timeouts ())
> > + return EINVAL;
>
> Why exactly do we need futex_supports_exact_relative_timeouts if Linux
> always set it to true?
I copied the use of it from the implementation of
pthread_condattr_setclock. It's described in sysdeps/nptl/futex-internal.h,
but there's no definition of it for anything other than Linux. It seems
quite possible that it has no use at all.
I can try removing it from pthread_condattr_setclock, but that would
probably be better as a separate change.
Mike.
More information about the Libc-alpha
mailing list