| Message ID | 1436277389-22478-1-git-send-email-mac@mcrowe.com |
|---|---|
| State | New |
| Headers | show |
On 07/07/15 14:56, 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 > 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. > > Inventing 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. > do you have a reference to the rationale that introduced this api to C++? this should be reported as an enhancement request to the austingroup if there is real world need for it so all platforms implement it consistently in the c runtime. > (This patch is only to the C implementation of pthread_cond_timedwait so > it is necessary to remove pthread_cond_timedwait.S and > pthread_cond_wait.S to use it on x86 and x86_64 at the moment. If the > approach is deemed acceptable I'll work on fixing those too along with > the documentation.) > > Signed-off-by: Mike Crowe <mac@mcrowe.com> > --- > conform/data/pthread.h-data | 3 + > nptl/Makefile | 4 +- > nptl/Versions | 1 + > nptl/pthread_cond_timedwait.c | 26 ++++-- > nptl/tst-cond11-onclock.c | 206 ++++++++++++++++++++++++++++++++++++++++++ > nptl/tst-cond5-onclock.c | 113 +++++++++++++++++++++++ > sysdeps/nptl/pthread.h | 13 +++ > 7 files changed, 356 insertions(+), 10 deletions(-) > create mode 100644 nptl/tst-cond11-onclock.c > create mode 100644 nptl/tst-cond5-onclock.c > > diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data > index c1e32c8..d52f298 100644 > --- a/conform/data/pthread.h-data > +++ b/conform/data/pthread.h-data > @@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*) > function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*) > function int pthread_cond_signal (pthread_cond_t*) > function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*) > +#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K > +function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*) > +#endif > function int pthread_cond_wait (pthread_cond_t*, pthread_mutex_t*) > function int pthread_condattr_destroy (pthread_condattr_t*) > #if !defined POSIX && !defined UNIX98 && !defined XOPEN2K > diff --git a/nptl/Makefile b/nptl/Makefile > index 4544aa2..ff06939 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -221,8 +221,8 @@ tests = tst-typesizes \ > tst-mutexpi5a tst-mutexpi6 tst-mutexpi7 tst-mutexpi7a tst-mutexpi8 \ > tst-mutexpi9 \ > tst-spin1 tst-spin2 tst-spin3 tst-spin4 \ > - tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond6 tst-cond7 \ > - tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond12 tst-cond13 \ > + tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond5-onclock tst-cond6 tst-cond7 \ > + tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond11-onclock tst-cond12 tst-cond13 \ > tst-cond14 tst-cond15 tst-cond16 tst-cond17 tst-cond18 tst-cond19 \ > tst-cond20 tst-cond21 tst-cond22 tst-cond23 tst-cond24 tst-cond25 \ > tst-cond-except \ > diff --git a/nptl/Versions b/nptl/Versions > index 34e4b46..476301f 100644 > --- a/nptl/Versions > +++ b/nptl/Versions > @@ -267,6 +267,7 @@ libpthread { > } > > GLIBC_2.22 { > + pthread_cond_timedwaitonclock_np; > } > > GLIBC_PRIVATE { > diff --git a/nptl/pthread_cond_timedwait.c b/nptl/pthread_cond_timedwait.c > index 10b0a61..a4c0ee3 100644 > --- a/nptl/pthread_cond_timedwait.c > +++ b/nptl/pthread_cond_timedwait.c > @@ -49,8 +49,9 @@ struct _condvar_cleanup_buffer > }; > > int > -__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, > - const struct timespec *abstime) > +pthread_cond_timedwaitonclock_np (pthread_cond_t *cond, pthread_mutex_t *mutex, > + clockid_t clockid, > + const struct timespec *abstime) > { > struct _pthread_cleanup_buffer buffer; > struct _condvar_cleanup_buffer cbuffer; > @@ -120,8 +121,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, > # ifdef __NR_clock_gettime > INTERNAL_SYSCALL_DECL (err); > (void) INTERNAL_VSYSCALL (clock_gettime, err, 2, > - (cond->__data.__nwaiters > - & ((1 << COND_NWAITERS_SHIFT) - 1)), > + clockid, > &rt); > /* Convert the absolute timeout value to a relative timeout. */ > rt.tv_sec = abstime->tv_sec - rt.tv_sec; > @@ -175,8 +175,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, > > if (pi_flag) > { > - unsigned int clockbit = (cond->__data.__nwaiters & 1 > - ? 0 : FUTEX_CLOCK_REALTIME); > + unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME; > err = lll_futex_timed_wait_requeue_pi (&cond->__data.__futex, > futex_val, abstime, clockbit, > &mutex->__data.__lock, > @@ -193,8 +192,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, > err = lll_futex_timed_wait (&cond->__data.__futex, > futex_val, &rt, pshared); > #else > - unsigned int clockbit = (cond->__data.__nwaiters & 1 > - ? 0 : FUTEX_CLOCK_REALTIME); > + unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME; > err = lll_futex_timed_wait_bitset (&cond->__data.__futex, futex_val, > abstime, clockbit, pshared); > #endif > @@ -264,5 +262,17 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, > return err ?: result; > } > > +int > +__pthread_cond_timedwait (cond, mutex, abstime) > + pthread_cond_t *cond; > + pthread_mutex_t *mutex; > + const struct timespec *abstime; > +{ > + return pthread_cond_timedwaitonclock_np (cond, mutex, > + (cond->__data.__nwaiters > + & ((1 << COND_NWAITERS_SHIFT) - 1)), > + abstime); > +} > + > versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait, > GLIBC_2_3_2); > diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c > new file mode 100644 > index 0000000..15b3730 > --- /dev/null > +++ b/nptl/tst-cond11-onclock.c > @@ -0,0 +1,206 @@ > +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + Contributed by Ulrich Drepper <drepper@redhat.com>, 2003. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <pthread.h> > +#include <stdio.h> > +#include <time.h> > +#include <unistd.h> > + > + > +#if defined _POSIX_CLOCK_SELECTION && _POSIX_CLOCK_SELECTION >= 0 > +static int > +run_test (clockid_t attr_clock, clockid_t wait_clock) > +{ > + pthread_condattr_t condattr; > + pthread_cond_t cond; > + pthread_mutexattr_t mutattr; > + pthread_mutex_t mut; > + > + printf ("attr_clock = %d, wait_clock = %d\n", (int) attr_clock, (int) wait_clock); > + > + if (pthread_condattr_init (&condattr) != 0) > + { > + puts ("condattr_init failed"); > + return 1; > + } > + > + if (pthread_condattr_setclock (&condattr, attr_clock) != 0) > + { > + puts ("condattr_setclock failed"); > + return 1; > + } > + > + clockid_t cl2; > + if (pthread_condattr_getclock (&condattr, &cl2) != 0) > + { > + puts ("condattr_getclock failed"); > + return 1; > + } > + if (attr_clock != cl2) > + { > + printf ("condattr_getclock returned wrong value: %d, expected %d\n", > + (int) cl2, (int) attr_clock); > + return 1; > + } > + > + if (pthread_cond_init (&cond, &condattr) != 0) > + { > + puts ("cond_init failed"); > + return 1; > + } > + > + if (pthread_condattr_destroy (&condattr) != 0) > + { > + puts ("condattr_destroy failed"); > + return 1; > + } > + > + if (pthread_mutexattr_init (&mutattr) != 0) > + { > + puts ("mutexattr_init failed"); > + return 1; > + } > + > + if (pthread_mutexattr_settype (&mutattr, PTHREAD_MUTEX_ERRORCHECK) != 0) > + { > + puts ("mutexattr_settype failed"); > + return 1; > + } > + > + if (pthread_mutex_init (&mut, &mutattr) != 0) > + { > + puts ("mutex_init failed"); > + return 1; > + } > + > + if (pthread_mutexattr_destroy (&mutattr) != 0) > + { > + puts ("mutexattr_destroy failed"); > + return 1; > + } > + > + if (pthread_mutex_lock (&mut) != 0) > + { > + puts ("mutex_lock failed"); > + return 1; > + } > + > + if (pthread_mutex_lock (&mut) != EDEADLK) > + { > + puts ("2nd mutex_lock did not return EDEADLK"); > + return 1; > + } > + > + struct timespec ts; > + if (clock_gettime (wait_clock, &ts) != 0) > + { > + puts ("clock_gettime failed"); > + return 1; > + } > + > + /* Wait one second. */ > + ++ts.tv_sec; > + > + int e = pthread_cond_timedwaitonclock_np (&cond, &mut, wait_clock, &ts); > + if (e == 0) > + { > + puts ("cond_timedwait succeeded"); > + return 1; > + } > + else if (e != ETIMEDOUT) > + { > + puts ("cond_timedwait did not return ETIMEDOUT"); > + return 1; > + } > + > + struct timespec ts2; > + if (clock_gettime (wait_clock, &ts2) != 0) > + { > + puts ("second clock_gettime failed"); > + return 1; > + } > + > + if (ts2.tv_sec < ts.tv_sec > + || (ts2.tv_sec == ts.tv_sec && ts2.tv_nsec < ts.tv_nsec)) > + { > + puts ("timeout too short"); > + return 1; > + } > + > + if (pthread_mutex_unlock (&mut) != 0) > + { > + puts ("mutex_unlock failed"); > + return 1; > + } > + > + if (pthread_mutex_destroy (&mut) != 0) > + { > + puts ("mutex_destroy failed"); > + return 1; > + } > + > + if (pthread_cond_destroy (&cond) != 0) > + { > + puts ("cond_destroy failed"); > + return 1; > + } > + > + return 0; > +} > +#endif > + > + > +static int > +do_test (void) > +{ > +#if !defined _POSIX_CLOCK_SELECTION || _POSIX_CLOCK_SELECTION == -1 > + > + puts ("_POSIX_CLOCK_SELECTION not supported, test skipped"); > + return 0; > + > +#else > + > + int res = run_test (CLOCK_REALTIME, CLOCK_REALTIME); > + > +# if defined _POSIX_MONOTONIC_CLOCK && _POSIX_MONOTONIC_CLOCK >= 0 > +# if _POSIX_MONOTONIC_CLOCK == 0 > + int e = sysconf (_SC_MONOTONIC_CLOCK); > + if (e < 0) > + puts ("CLOCK_MONOTONIC not supported"); > + else if (e == 0) > + { > + puts ("sysconf (_SC_MONOTONIC_CLOCK) must not return 0"); > + res = 1; > + } > + else > +# endif > + res |= run_test (CLOCK_REALTIME, CLOCK_MONOTONIC); > + res |= run_test (CLOCK_MONOTONIC, CLOCK_MONOTONIC); > + res |= run_test (CLOCK_MONOTONIC, CLOCK_REALTIME); > +# else > + puts ("_POSIX_MONOTONIC_CLOCK not defined"); > +# endif > + > + return res; > +#endif > +} > + > +#define TIMEOUT 12 > +#define TEST_FUNCTION do_test () > +#include "../test-skeleton.c" > diff --git a/nptl/tst-cond5-onclock.c b/nptl/tst-cond5-onclock.c > new file mode 100644 > index 0000000..ddb0ad1 > --- /dev/null > +++ b/nptl/tst-cond5-onclock.c > @@ -0,0 +1,113 @@ > +/* Copyright (C) 2002-2014 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + Contributed by Ulrich Drepper <drepper@redhat.com>, 2002. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <pthread.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <time.h> > +#include <sys/time.h> > + > + > +static pthread_mutex_t mut; > +static pthread_cond_t cond = PTHREAD_COND_INITIALIZER; > + > + > +static int > +do_test_onclock(clockid_t clockid) > +{ > + pthread_mutexattr_t ma; > + int err; > + struct timespec ts; > + > + if (pthread_mutexattr_init (&ma) != 0) > + { > + puts ("mutexattr_init failed"); > + exit (1); > + } > + > + if (pthread_mutexattr_settype (&ma, PTHREAD_MUTEX_ERRORCHECK) != 0) > + { > + puts ("mutexattr_settype failed"); > + exit (1); > + } > + > + if (pthread_mutex_init (&mut, &ma) != 0) > + { > + puts ("mutex_init failed"); > + exit (1); > + } > + > + /* Get the mutex. */ > + if (pthread_mutex_lock (&mut) != 0) > + { > + puts ("mutex_lock failed"); > + exit (1); > + } > + > + /* Waiting for the condition will fail. But we want the timeout here. */ > + if (clock_gettime (clockid, &ts) != 0) > + { > + puts ("clock_gettime failed"); > + exit (1); > + } > + > + ts.tv_nsec += 500000000; > + if (ts.tv_nsec >= 1000000000) > + { > + ts.tv_nsec -= 1000000000; > + ++ts.tv_sec; > + } > + err = pthread_cond_timedwaitonclock_np (&cond, &mut, clockid, &ts); > + if (err == 0) > + { > + /* This could in theory happen but here without any signal and > + additional waiter it should not. */ > + puts ("cond_timedwait succeeded"); > + exit (1); > + } > + else if (err != ETIMEDOUT) > + { > + printf ("cond_timedwait returned with %s\n", strerror (err)); > + exit (1); > + } > + > + err = pthread_mutex_unlock (&mut); > + if (err != 0) > + { > + printf ("mutex_unlock failed: %s\n", strerror (err)); > + exit (1); > + } > + > + return 0; > +} > + > +static int > +do_test (void) > +{ > + int rc; > + rc = do_test_onclock(CLOCK_MONOTONIC); > + if (rc == 0) > + rc = do_test_onclock(CLOCK_REALTIME); > + > + return rc; > +} > + > +#define TEST_FUNCTION do_test () > +#include "../test-skeleton.c" > diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h > index 89d0882..221924b 100644 > --- a/sysdeps/nptl/pthread.h > +++ b/sysdeps/nptl/pthread.h > @@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond, > const struct timespec *__restrict __abstime) > __nonnull ((1, 2, 3)); > > +/* Wait for condition variable COND to be signaled or broadcast until > + ABSTIME measured by the specified clock. MUTEX is assumed to be > + locked before. CLOCK is a clock specified. ABSTIME is an absolute > + time specification against CLOCK's epoch. > + > + This function is a cancellation point and therefore not marked with > + __THROW. */ > +extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond, > + pthread_mutex_t *__restrict __mutex, > + __clockid_t __clock_id, > + const struct timespec *__restrict __abstime) > + __nonnull ((1, 2, 4)); > + > /* Functions for handling condition variable attributes. */ > > /* Initialize condition variable attribute ATTR. */ > -- > 2.1.4 > > >
On Tuesday 07 July 2015 at 15:28:56 +0100, Szabolcs Nagy wrote: > On 07/07/15 14:56, 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 > > 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. > > > > Inventing 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. > > > > do you have a reference to the rationale that introduced this api to C++? Not exactly. Here are some interesting links: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2999.html http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4486.html (search for "887") https://www.redhat.com/archives/posix-c++-wg/2009-July/msg00002.html (search for "887") https://github.com/cplusplus/LWG/blob/master/xml/issue0887.xml https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861 It seems that even more work than I was previously aware of has been invested in trying to solve this over the past six years or so. :( Mike.
On Tue, Jul 07, 2015 at 02:56:29PM +0100, 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 > 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. > > Inventing 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. > > (This patch is only to the C implementation of pthread_cond_timedwait so > it is necessary to remove pthread_cond_timedwait.S and > pthread_cond_wait.S to use it on x86 and x86_64 at the moment. If the > approach is deemed acceptable I'll work on fixing those too along with > the documentation.) > Assembly will be deleted soon by Torvald's patch. I didn't make any sense in first place.
On Tue, 7 Jul 2015, Mike Crowe wrote: > diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data > index c1e32c8..d52f298 100644 > --- a/conform/data/pthread.h-data > +++ b/conform/data/pthread.h-data > @@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*) > function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*) > function int pthread_cond_signal (pthread_cond_t*) > function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*) > +#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K > +function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*) > +#endif No, this is wrong. A function not in any released version of POSIX should not be added to the conform/ data. > diff --git a/nptl/Versions b/nptl/Versions > index 34e4b46..476301f 100644 > --- a/nptl/Versions > +++ b/nptl/Versions > @@ -267,6 +267,7 @@ libpthread { > } > > GLIBC_2.22 { > + pthread_cond_timedwaitonclock_np; New symbol versions require all the ABI test baselines to be updated as well, in the same patch adding the new ABI. > diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c > new file mode 100644 > index 0000000..15b3730 > --- /dev/null > +++ b/nptl/tst-cond11-onclock.c > @@ -0,0 +1,206 @@ > +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + Contributed by Ulrich Drepper <drepper@redhat.com>, 2003. No "Contributed by" lines in new files. 2015 in copyright dates. > diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h > index 89d0882..221924b 100644 > --- a/sysdeps/nptl/pthread.h > +++ b/sysdeps/nptl/pthread.h > @@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond, > const struct timespec *__restrict __abstime) > __nonnull ((1, 2, 3)); > > +/* Wait for condition variable COND to be signaled or broadcast until > + ABSTIME measured by the specified clock. MUTEX is assumed to be > + locked before. CLOCK is a clock specified. ABSTIME is an absolute > + time specification against CLOCK's epoch. > + > + This function is a cancellation point and therefore not marked with > + __THROW. */ > +extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond, > + pthread_mutex_t *__restrict __mutex, > + __clockid_t __clock_id, > + const struct timespec *__restrict __abstime) > + __nonnull ((1, 2, 4)); New non-POSIX functions should be conditional on __USE_GNU in headers. New APIs also need documentation in the glibc manual.
On Wednesday 22 July 2015 at 13:53:58 +0000, Joseph Myers wrote: > On Tue, 7 Jul 2015, Mike Crowe wrote: > > > diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data > > index c1e32c8..d52f298 100644 > > --- a/conform/data/pthread.h-data > > +++ b/conform/data/pthread.h-data > > @@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*) > > function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*) > > function int pthread_cond_signal (pthread_cond_t*) > > function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*) > > +#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K > > +function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*) > > +#endif > > No, this is wrong. A function not in any released version of POSIX should > not be added to the conform/ data. OK. > > diff --git a/nptl/Versions b/nptl/Versions > > index 34e4b46..476301f 100644 > > --- a/nptl/Versions > > +++ b/nptl/Versions > > @@ -267,6 +267,7 @@ libpthread { > > } > > > > GLIBC_2.22 { > > + pthread_cond_timedwaitonclock_np; > > New symbol versions require all the ABI test baselines to be updated as > well, in the same patch adding the new ABI. OK. I'll move to GLIBC_2.23 too since 2.22 is now frozen. > > diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c > > new file mode 100644 > > index 0000000..15b3730 > > --- /dev/null > > +++ b/nptl/tst-cond11-onclock.c > > @@ -0,0 +1,206 @@ > > +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. > > + This file is part of the GNU C Library. > > + Contributed by Ulrich Drepper <drepper@redhat.com>, 2003. > > No "Contributed by" lines in new files. 2015 in copyright dates. I can fix that (although since it's basically a copy of tst-cond11.c I think that I need to keep Ulrich Drepper's notice too.) > > diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h > > index 89d0882..221924b 100644 > > --- a/sysdeps/nptl/pthread.h > > +++ b/sysdeps/nptl/pthread.h > > @@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond, > > const struct timespec *__restrict __abstime) > > __nonnull ((1, 2, 3)); > > > > +/* Wait for condition variable COND to be signaled or broadcast until > > + ABSTIME measured by the specified clock. MUTEX is assumed to be > > + locked before. CLOCK is a clock specified. ABSTIME is an absolute > > + time specification against CLOCK's epoch. > > + > > + This function is a cancellation point and therefore not marked with > > + __THROW. */ > > +extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond, > > + pthread_mutex_t *__restrict __mutex, > > + __clockid_t __clock_id, > > + const struct timespec *__restrict __abstime) > > + __nonnull ((1, 2, 4)); > > New non-POSIX functions should be conditional on __USE_GNU in headers. OK. > New APIs also need documentation in the glibc manual. I'm having some trouble with the safety section. pthread_cond_timedwait isn't documented in the glibc manual since it is a POSIX function. The pthread_cond_timedwait man page tells me that pthread_cond_timedwait is MT-Safe but it's presumably neither AC-Safe nor AS-Safe. I'm not sure what to put in the asunsafe and acunsafe reasons. Cut and paste gave me: @safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@asulock}} is this correct? (I also plan to submit changes to the pthread_cond_timedwait man page.) Thanks for the review. Mike.
On 07/27/2015 03:00 PM, Mike Crowe wrote: >> New APIs also need documentation in the glibc manual. > > I'm having some trouble with the safety section. pthread_cond_timedwait > isn't documented in the glibc manual since it is a POSIX function. > > The pthread_cond_timedwait man page tells me that pthread_cond_timedwait is > MT-Safe but it's presumably neither AC-Safe nor AS-Safe. I'm not sure what to put in > the asunsafe and acunsafe reasons. Cut and paste gave me: Yes, the pthread_* functions are by definition MT-safe. > @safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@asulock}} > > is this correct? Almost. @safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@aculock}} Note: Use of "aculock" for acunsafe. Here it matters that the lock is lost not that we deadlock. With suggested source level comments like this: @c If exactly the same function with arguments is called from a signal @c handler that interrupts between the mutex unlock and sleep then it @c will unlock the mutex twice resulting in undefined behaviour. Keep @c in mind that the unlock and sleep are only atomic with respect to other @c threads (really a happens-after relationship for pthread_cond_broadcast @c and pthread_cond_signal). @c In the AC case we would cancel the thread and the mutex would remain @c locked and we can't recover from that. > (I also plan to submit changes to the pthread_cond_timedwait man page.) > > Thanks for the review. This does not constitute a full review, and I think a lot more people need to review this before we have consensus over a new API like this. Cheers, Carlos.
On Mon, 27 Jul 2015, Mike Crowe wrote: > > > diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c > > > new file mode 100644 > > > index 0000000..15b3730 > > > --- /dev/null > > > +++ b/nptl/tst-cond11-onclock.c > > > @@ -0,0 +1,206 @@ > > > +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. > > > + This file is part of the GNU C Library. > > > + Contributed by Ulrich Drepper <drepper@redhat.com>, 2003. > > > > No "Contributed by" lines in new files. 2015 in copyright dates. > > I can fix that (although since it's basically a copy of tst-cond11.c I > think that I need to keep Ulrich Drepper's notice too.) No. The first line of a new file, before the copyright notice, should be a comment describing the file's contents. You can include "based on tst-cond11.c" in that comment, but the place for identifying contributors is the list in the manual, the NEWS file entries for major contributions, etc. - not comments in the sources. (Or depending on how large the differences are, consider making them conditional in tst-cond11.c so that tst-cond11-onclock.c can just #define TEST_ONCLOCK then include tst-cond11.c, and avoid duplication.)
diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data index c1e32c8..d52f298 100644 --- a/conform/data/pthread.h-data +++ b/conform/data/pthread.h-data @@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*) function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*) function int pthread_cond_signal (pthread_cond_t*) function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*) +#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K +function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*) +#endif function int pthread_cond_wait (pthread_cond_t*, pthread_mutex_t*) function int pthread_condattr_destroy (pthread_condattr_t*) #if !defined POSIX && !defined UNIX98 && !defined XOPEN2K diff --git a/nptl/Makefile b/nptl/Makefile index 4544aa2..ff06939 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -221,8 +221,8 @@ tests = tst-typesizes \ tst-mutexpi5a tst-mutexpi6 tst-mutexpi7 tst-mutexpi7a tst-mutexpi8 \ tst-mutexpi9 \ tst-spin1 tst-spin2 tst-spin3 tst-spin4 \ - tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond6 tst-cond7 \ - tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond12 tst-cond13 \ + tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond5-onclock tst-cond6 tst-cond7 \ + tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond11-onclock tst-cond12 tst-cond13 \ tst-cond14 tst-cond15 tst-cond16 tst-cond17 tst-cond18 tst-cond19 \ tst-cond20 tst-cond21 tst-cond22 tst-cond23 tst-cond24 tst-cond25 \ tst-cond-except \ diff --git a/nptl/Versions b/nptl/Versions index 34e4b46..476301f 100644 --- a/nptl/Versions +++ b/nptl/Versions @@ -267,6 +267,7 @@ libpthread { } GLIBC_2.22 { + pthread_cond_timedwaitonclock_np; } GLIBC_PRIVATE { diff --git a/nptl/pthread_cond_timedwait.c b/nptl/pthread_cond_timedwait.c index 10b0a61..a4c0ee3 100644 --- a/nptl/pthread_cond_timedwait.c +++ b/nptl/pthread_cond_timedwait.c @@ -49,8 +49,9 @@ struct _condvar_cleanup_buffer }; int -__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, - const struct timespec *abstime) +pthread_cond_timedwaitonclock_np (pthread_cond_t *cond, pthread_mutex_t *mutex, + clockid_t clockid, + const struct timespec *abstime) { struct _pthread_cleanup_buffer buffer; struct _condvar_cleanup_buffer cbuffer; @@ -120,8 +121,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, # ifdef __NR_clock_gettime INTERNAL_SYSCALL_DECL (err); (void) INTERNAL_VSYSCALL (clock_gettime, err, 2, - (cond->__data.__nwaiters - & ((1 << COND_NWAITERS_SHIFT) - 1)), + clockid, &rt); /* Convert the absolute timeout value to a relative timeout. */ rt.tv_sec = abstime->tv_sec - rt.tv_sec; @@ -175,8 +175,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, if (pi_flag) { - unsigned int clockbit = (cond->__data.__nwaiters & 1 - ? 0 : FUTEX_CLOCK_REALTIME); + unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME; err = lll_futex_timed_wait_requeue_pi (&cond->__data.__futex, futex_val, abstime, clockbit, &mutex->__data.__lock, @@ -193,8 +192,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, err = lll_futex_timed_wait (&cond->__data.__futex, futex_val, &rt, pshared); #else - unsigned int clockbit = (cond->__data.__nwaiters & 1 - ? 0 : FUTEX_CLOCK_REALTIME); + unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME; err = lll_futex_timed_wait_bitset (&cond->__data.__futex, futex_val, abstime, clockbit, pshared); #endif @@ -264,5 +262,17 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex, return err ?: result; } +int +__pthread_cond_timedwait (cond, mutex, abstime) + pthread_cond_t *cond; + pthread_mutex_t *mutex; + const struct timespec *abstime; +{ + return pthread_cond_timedwaitonclock_np (cond, mutex, + (cond->__data.__nwaiters + & ((1 << COND_NWAITERS_SHIFT) - 1)), + abstime); +} + versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait, GLIBC_2_3_2); diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c new file mode 100644 index 0000000..15b3730 --- /dev/null +++ b/nptl/tst-cond11-onclock.c @@ -0,0 +1,206 @@ +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Ulrich Drepper <drepper@redhat.com>, 2003. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <pthread.h> +#include <stdio.h> +#include <time.h> +#include <unistd.h> + + +#if defined _POSIX_CLOCK_SELECTION && _POSIX_CLOCK_SELECTION >= 0 +static int +run_test (clockid_t attr_clock, clockid_t wait_clock) +{ + pthread_condattr_t condattr; + pthread_cond_t cond; + pthread_mutexattr_t mutattr; + pthread_mutex_t mut; + + printf ("attr_clock = %d, wait_clock = %d\n", (int) attr_clock, (int) wait_clock); + + if (pthread_condattr_init (&condattr) != 0) + { + puts ("condattr_init failed"); + return 1; + } + + if (pthread_condattr_setclock (&condattr, attr_clock) != 0) + { + puts ("condattr_setclock failed"); + return 1; + } + + clockid_t cl2; + if (pthread_condattr_getclock (&condattr, &cl2) != 0) + { + puts ("condattr_getclock failed"); + return 1; + } + if (attr_clock != cl2) + { + printf ("condattr_getclock returned wrong value: %d, expected %d\n", + (int) cl2, (int) attr_clock); + return 1; + } + + if (pthread_cond_init (&cond, &condattr) != 0) + { + puts ("cond_init failed"); + return 1; + } + + if (pthread_condattr_destroy (&condattr) != 0) + { + puts ("condattr_destroy failed"); + return 1; + } + + if (pthread_mutexattr_init (&mutattr) != 0) + { + puts ("mutexattr_init failed"); + return 1; + } + + if (pthread_mutexattr_settype (&mutattr, PTHREAD_MUTEX_ERRORCHECK) != 0) + { + puts ("mutexattr_settype failed"); + return 1; + } + + if (pthread_mutex_init (&mut, &mutattr) != 0) + { + puts ("mutex_init failed"); + return 1; + } + + if (pthread_mutexattr_destroy (&mutattr) != 0) + { + puts ("mutexattr_destroy failed"); + return 1; + } + + if (pthread_mutex_lock (&mut) != 0) + { + puts ("mutex_lock failed"); + return 1; + } + + if (pthread_mutex_lock (&mut) != EDEADLK) + { + puts ("2nd mutex_lock did not return EDEADLK"); + return 1; + } + + struct timespec ts; + if (clock_gettime (wait_clock, &ts) != 0) + { + puts ("clock_gettime failed"); + return 1; + } + + /* Wait one second. */ + ++ts.tv_sec; + + int e = pthread_cond_timedwaitonclock_np (&cond, &mut, wait_clock, &ts); + if (e == 0) + { + puts ("cond_timedwait succeeded"); + return 1; + } + else if (e != ETIMEDOUT) + { + puts ("cond_timedwait did not return ETIMEDOUT"); + return 1; + } + + struct timespec ts2; + if (clock_gettime (wait_clock, &ts2) != 0) + { + puts ("second clock_gettime failed"); + return 1; + } + + if (ts2.tv_sec < ts.tv_sec + || (ts2.tv_sec == ts.tv_sec && ts2.tv_nsec < ts.tv_nsec)) + { + puts ("timeout too short"); + return 1; + } + + if (pthread_mutex_unlock (&mut) != 0) + { + puts ("mutex_unlock failed"); + return 1; + } + + if (pthread_mutex_destroy (&mut) != 0) + { + puts ("mutex_destroy failed"); + return 1; + } + + if (pthread_cond_destroy (&cond) != 0) + { + puts ("cond_destroy failed"); + return 1; + } + + return 0; +} +#endif + + +static int +do_test (void) +{ +#if !defined _POSIX_CLOCK_SELECTION || _POSIX_CLOCK_SELECTION == -1 + + puts ("_POSIX_CLOCK_SELECTION not supported, test skipped"); + return 0; + +#else + + int res = run_test (CLOCK_REALTIME, CLOCK_REALTIME); + +# if defined _POSIX_MONOTONIC_CLOCK && _POSIX_MONOTONIC_CLOCK >= 0 +# if _POSIX_MONOTONIC_CLOCK == 0 + int e = sysconf (_SC_MONOTONIC_CLOCK); + if (e < 0) + puts ("CLOCK_MONOTONIC not supported"); + else if (e == 0) + { + puts ("sysconf (_SC_MONOTONIC_CLOCK) must not return 0"); + res = 1; + } + else +# endif + res |= run_test (CLOCK_REALTIME, CLOCK_MONOTONIC); + res |= run_test (CLOCK_MONOTONIC, CLOCK_MONOTONIC); + res |= run_test (CLOCK_MONOTONIC, CLOCK_REALTIME); +# else + puts ("_POSIX_MONOTONIC_CLOCK not defined"); +# endif + + return res; +#endif +} + +#define TIMEOUT 12 +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/nptl/tst-cond5-onclock.c b/nptl/tst-cond5-onclock.c new file mode 100644 index 0000000..ddb0ad1 --- /dev/null +++ b/nptl/tst-cond5-onclock.c @@ -0,0 +1,113 @@ +/* Copyright (C) 2002-2014 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Ulrich Drepper <drepper@redhat.com>, 2002. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <pthread.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <time.h> +#include <sys/time.h> + + +static pthread_mutex_t mut; +static pthread_cond_t cond = PTHREAD_COND_INITIALIZER; + + +static int +do_test_onclock(clockid_t clockid) +{ + pthread_mutexattr_t ma; + int err; + struct timespec ts; + + if (pthread_mutexattr_init (&ma) != 0) + { + puts ("mutexattr_init failed"); + exit (1); + } + + if (pthread_mutexattr_settype (&ma, PTHREAD_MUTEX_ERRORCHECK) != 0) + { + puts ("mutexattr_settype failed"); + exit (1); + } + + if (pthread_mutex_init (&mut, &ma) != 0) + { + puts ("mutex_init failed"); + exit (1); + } + + /* Get the mutex. */ + if (pthread_mutex_lock (&mut) != 0) + { + puts ("mutex_lock failed"); + exit (1); + } + + /* Waiting for the condition will fail. But we want the timeout here. */ + if (clock_gettime (clockid, &ts) != 0) + { + puts ("clock_gettime failed"); + exit (1); + } + + ts.tv_nsec += 500000000; + if (ts.tv_nsec >= 1000000000) + { + ts.tv_nsec -= 1000000000; + ++ts.tv_sec; + } + err = pthread_cond_timedwaitonclock_np (&cond, &mut, clockid, &ts); + if (err == 0) + { + /* This could in theory happen but here without any signal and + additional waiter it should not. */ + puts ("cond_timedwait succeeded"); + exit (1); + } + else if (err != ETIMEDOUT) + { + printf ("cond_timedwait returned with %s\n", strerror (err)); + exit (1); + } + + err = pthread_mutex_unlock (&mut); + if (err != 0) + { + printf ("mutex_unlock failed: %s\n", strerror (err)); + exit (1); + } + + return 0; +} + +static int +do_test (void) +{ + int rc; + rc = do_test_onclock(CLOCK_MONOTONIC); + if (rc == 0) + rc = do_test_onclock(CLOCK_REALTIME); + + return rc; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h index 89d0882..221924b 100644 --- a/sysdeps/nptl/pthread.h +++ b/sysdeps/nptl/pthread.h @@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond, const struct timespec *__restrict __abstime) __nonnull ((1, 2, 3)); +/* Wait for condition variable COND to be signaled or broadcast until + ABSTIME measured by the specified clock. MUTEX is assumed to be + locked before. CLOCK is a clock specified. ABSTIME is an absolute + time specification against CLOCK's epoch. + + This function is a cancellation point and therefore not marked with + __THROW. */ +extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond, + pthread_mutex_t *__restrict __mutex, + __clockid_t __clock_id, + const struct timespec *__restrict __abstime) + __nonnull ((1, 2, 4)); + /* Functions for handling condition variable attributes. */ /* Initialize condition variable attribute ATTR. */
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 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. Inventing 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. (This patch is only to the C implementation of pthread_cond_timedwait so it is necessary to remove pthread_cond_timedwait.S and pthread_cond_wait.S to use it on x86 and x86_64 at the moment. If the approach is deemed acceptable I'll work on fixing those too along with the documentation.) Signed-off-by: Mike Crowe <mac@mcrowe.com> --- conform/data/pthread.h-data | 3 + nptl/Makefile | 4 +- nptl/Versions | 1 + nptl/pthread_cond_timedwait.c | 26 ++++-- nptl/tst-cond11-onclock.c | 206 ++++++++++++++++++++++++++++++++++++++++++ nptl/tst-cond5-onclock.c | 113 +++++++++++++++++++++++ sysdeps/nptl/pthread.h | 13 +++ 7 files changed, 356 insertions(+), 10 deletions(-) create mode 100644 nptl/tst-cond11-onclock.c create mode 100644 nptl/tst-cond5-onclock.c