Message ID | 20180724211805.E5409439942EF@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | C11 threads: Fix timeout and locking issues | expand |
On 24/07/2018 18:18, Florian Weimer wrote: > 2018-07-24 Florian Weimer <fweimer@redhat.com> > > * nptl/tst-mtx-timedlock.c (do_test): Implement carry from > nanoseconds into seconds. > * nptl/tst-cnd-basic.c (signal_parent): Lock and unlock mutex. > (do_test): Likewise. > * nptl/tst-cnd-timedwait.c (signal_parent): Likewise. > (do_test): Likewise. Avoid nanosecond overflow and spurious > timeouts due to system load. > * nptl/tst-cnd-broadcast.c (waiting_threads): New variable. > (child_wait): Increment it. > (do_test): Wait as long as necessary until all expected threads > have arrived. LGTM, thanks. > > diff --git a/nptl/tst-cnd-basic.c b/nptl/tst-cnd-basic.c > index 84b7f5f647..eb2fb6a77e 100644 > --- a/nptl/tst-cnd-basic.c > +++ b/nptl/tst-cnd-basic.c > @@ -31,8 +31,14 @@ static mtx_t mutex; > static int > signal_parent (void) > { > + /* Acquire the lock so that cnd_signal does not run until > + cnd_timedwait has been called. */ > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); > if (cnd_signal (&cond) != thrd_success) > FAIL_EXIT1 ("cnd_signal"); > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); > > thrd_exit (thrd_success); > } > @@ -47,6 +53,9 @@ do_test (void) > if (mtx_init (&mutex, mtx_plain) != thrd_success) > FAIL_EXIT1 ("mtx_init failed"); > > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); > + > if (thrd_create (&id, (thrd_start_t) signal_parent, NULL) > != thrd_success) > FAIL_EXIT1 ("thrd_create failed"); > @@ -59,6 +68,9 @@ do_test (void) > if (thrd_join (id, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_join failed"); > > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); > + > mtx_destroy (&mutex); > cnd_destroy (&cond); > > diff --git a/nptl/tst-cnd-broadcast.c b/nptl/tst-cnd-broadcast.c > index 90d6843154..62a4ab5a39 100644 > --- a/nptl/tst-cnd-broadcast.c > +++ b/nptl/tst-cnd-broadcast.c > @@ -17,6 +17,7 @@ > <http://www.gnu.org/licenses/>. */ > > #include <threads.h> > +#include <stdbool.h> > #include <stdio.h> > #include <unistd.h> > > @@ -28,12 +29,16 @@ static cnd_t cond; > /* Mutex to control wait on cond. */ > static mtx_t mutex; > > +/* Number of threads which have entered the cnd_wait region. */ > +static unsigned int waiting_threads; > + > /* Code executed by each thread. */ > static int > child_wait (void* data) > { > /* Wait until parent thread sends broadcast here. */ > mtx_lock (&mutex); > + ++waiting_threads; > cnd_wait (&cond, &mutex); > mtx_unlock (&mutex); > > @@ -61,7 +66,16 @@ do_test (void) > } > > /* Wait for other threads to reach their wait func. */ > - thrd_sleep (&((struct timespec){.tv_sec = 2}), NULL); > + while (true) > + { > + mtx_lock (&mutex); > + TEST_VERIFY (waiting_threads <= N); > + bool done_waiting = waiting_threads == N; > + mtx_unlock (&mutex); > + if (done_waiting) > + break; > + thrd_sleep (&((struct timespec){.tv_nsec = 100 * 1000 * 1000}), NULL); > + } > > mtx_lock (&mutex); > if (cnd_broadcast (&cond) != thrd_success) > diff --git a/nptl/tst-cnd-timedwait.c b/nptl/tst-cnd-timedwait.c > index 45a1512940..7d8a8e3557 100644 > --- a/nptl/tst-cnd-timedwait.c > +++ b/nptl/tst-cnd-timedwait.c > @@ -31,8 +31,14 @@ static mtx_t mutex; > static int > signal_parent (void *arg) > { > + /* Acquire the lock so that cnd_signal does not run until > + cnd_timedwait has been called. */ > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); > if (cnd_signal (&cond) != thrd_success) > FAIL_EXIT1 ("cnd_signal failed"); > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); > > thrd_exit (thrd_success); > } > @@ -47,10 +53,15 @@ do_test (void) > FAIL_EXIT1 ("cnd_init failed"); > if (mtx_init (&mutex, mtx_plain) != thrd_success) > FAIL_EXIT1 ("mtx_init failed"); > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); > > if (clock_gettime (CLOCK_REALTIME, &w_time) != 0) > FAIL_EXIT1 ("clock_gettime failed"); > - w_time.tv_nsec += 150000; > + > + /* This needs to be sufficiently long to prevent the cnd_timedwait > + call from timing out. */ > + w_time.tv_sec += 3600; > > if (thrd_create (&id, signal_parent, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_create failed"); > @@ -61,6 +72,9 @@ do_test (void) > if (thrd_join (id, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_join failed"); > > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); > + > mtx_destroy (&mutex); > cnd_destroy (&cond); > > diff --git a/nptl/tst-mtx-timedlock.c b/nptl/tst-mtx-timedlock.c > index dcae828fb2..616db722eb 100644 > --- a/nptl/tst-mtx-timedlock.c > +++ b/nptl/tst-mtx-timedlock.c > @@ -78,6 +78,11 @@ do_test (void) > /* Tiny amount of time, to assure that if any thread finds it busy. > It will receive thrd_timedout. */ > wait_time.tv_nsec += 1; > + if (wait_time.tv_nsec == 1000 * 1000 * 1000) > + { > + wait_time.tv_sec += 1; > + wait_time.tv_nsec = 0; > + } > > if (thrd_create (&id, child_add, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_create failed"); >
On 07/24/2018 05:18 PM, Florian Weimer wrote: > 2018-07-24 Florian Weimer <fweimer@redhat.com> > > * nptl/tst-mtx-timedlock.c (do_test): Implement carry from > nanoseconds into seconds. > * nptl/tst-cnd-basic.c (signal_parent): Lock and unlock mutex. > (do_test): Likewise. > * nptl/tst-cnd-timedwait.c (signal_parent): Likewise. > (do_test): Likewise. Avoid nanosecond overflow and spurious > timeouts due to system load. > * nptl/tst-cnd-broadcast.c (waiting_threads): New variable. > (child_wait): Increment it. > (do_test): Wait as long as necessary until all expected threads > have arrived. OK for 2.28. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > diff --git a/nptl/tst-cnd-basic.c b/nptl/tst-cnd-basic.c > index 84b7f5f647..eb2fb6a77e 100644 > --- a/nptl/tst-cnd-basic.c > +++ b/nptl/tst-cnd-basic.c > @@ -31,8 +31,14 @@ static mtx_t mutex; > static int > signal_parent (void) > { > + /* Acquire the lock so that cnd_signal does not run until > + cnd_timedwait has been called. */ > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); OK. happens-after the parent locks the mutex, so this always blocks. > if (cnd_signal (&cond) != thrd_success) > FAIL_EXIT1 ("cnd_signal"); > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); > > thrd_exit (thrd_success); > } > @@ -47,6 +53,9 @@ do_test (void) > if (mtx_init (&mutex, mtx_plain) != thrd_success) > FAIL_EXIT1 ("mtx_init failed"); > > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); OK. Happens-before the thread even starts. > + > if (thrd_create (&id, (thrd_start_t) signal_parent, NULL) > != thrd_success) > FAIL_EXIT1 ("thrd_create failed"); > @@ -59,6 +68,9 @@ do_test (void) > if (thrd_join (id, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_join failed"); > > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); OK. Happens-after the thread unlocks the mutex. > + > mtx_destroy (&mutex); > cnd_destroy (&cond); > > diff --git a/nptl/tst-cnd-broadcast.c b/nptl/tst-cnd-broadcast.c > index 90d6843154..62a4ab5a39 100644 > --- a/nptl/tst-cnd-broadcast.c > +++ b/nptl/tst-cnd-broadcast.c > @@ -17,6 +17,7 @@ > <http://www.gnu.org/licenses/>. */ > > #include <threads.h> > +#include <stdbool.h> > #include <stdio.h> > #include <unistd.h> > > @@ -28,12 +29,16 @@ static cnd_t cond; > /* Mutex to control wait on cond. */ > static mtx_t mutex; > > +/* Number of threads which have entered the cnd_wait region. */ > +static unsigned int waiting_threads; > + > /* Code executed by each thread. */ > static int > child_wait (void* data) > { > /* Wait until parent thread sends broadcast here. */ > mtx_lock (&mutex); > + ++waiting_threads; > cnd_wait (&cond, &mutex); > mtx_unlock (&mutex); > > @@ -61,7 +66,16 @@ do_test (void) > } > > /* Wait for other threads to reach their wait func. */ > - thrd_sleep (&((struct timespec){.tv_sec = 2}), NULL); OK, removing racy wait. > + while (true) > + { > + mtx_lock (&mutex); > + TEST_VERIFY (waiting_threads <= N); > + bool done_waiting = waiting_threads == N; > + mtx_unlock (&mutex); > + if (done_waiting) > + break; > + thrd_sleep (&((struct timespec){.tv_nsec = 100 * 1000 * 1000}), NULL); > + } OK, using polling loop to ensure happens-before via mtx_lock to read waiting_threads and ensure all threads are ready. > > mtx_lock (&mutex); > if (cnd_broadcast (&cond) != thrd_success) > diff --git a/nptl/tst-cnd-timedwait.c b/nptl/tst-cnd-timedwait.c > index 45a1512940..7d8a8e3557 100644 > --- a/nptl/tst-cnd-timedwait.c > +++ b/nptl/tst-cnd-timedwait.c > @@ -31,8 +31,14 @@ static mtx_t mutex; > static int > signal_parent (void *arg) > { > + /* Acquire the lock so that cnd_signal does not run until > + cnd_timedwait has been called. */ > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); OK, same problem as before, should take the lock to prevent early signaling. > if (cnd_signal (&cond) != thrd_success) > FAIL_EXIT1 ("cnd_signal failed"); > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); OK. > > thrd_exit (thrd_success); > } > @@ -47,10 +53,15 @@ do_test (void) > FAIL_EXIT1 ("cnd_init failed"); > if (mtx_init (&mutex, mtx_plain) != thrd_success) > FAIL_EXIT1 ("mtx_init failed"); > + if (mtx_lock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_lock failed"); OK. > > if (clock_gettime (CLOCK_REALTIME, &w_time) != 0) > FAIL_EXIT1 ("clock_gettime failed"); > - w_time.tv_nsec += 150000; > + > + /* This needs to be sufficiently long to prevent the cnd_timedwait > + call from timing out. */ > + w_time.tv_sec += 3600; OK, racy, but this is what we're trying to test, so no easy way around this. > > if (thrd_create (&id, signal_parent, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_create failed"); > @@ -61,6 +72,9 @@ do_test (void) > if (thrd_join (id, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_join failed"); > > + if (mtx_unlock (&mutex) != thrd_success) > + FAIL_EXIT1 ("mtx_unlock"); OK. > + > mtx_destroy (&mutex); > cnd_destroy (&cond); > > diff --git a/nptl/tst-mtx-timedlock.c b/nptl/tst-mtx-timedlock.c > index dcae828fb2..616db722eb 100644 > --- a/nptl/tst-mtx-timedlock.c > +++ b/nptl/tst-mtx-timedlock.c > @@ -78,6 +78,11 @@ do_test (void) > /* Tiny amount of time, to assure that if any thread finds it busy. > It will receive thrd_timedout. */ > wait_time.tv_nsec += 1; > + if (wait_time.tv_nsec == 1000 * 1000 * 1000) > + { > + wait_time.tv_sec += 1; > + wait_time.tv_nsec = 0; > + } OK. I guess it has to happen every once in a while... > > if (thrd_create (&id, child_add, NULL) != thrd_success) > FAIL_EXIT1 ("thrd_create failed"); >
diff --git a/nptl/tst-cnd-basic.c b/nptl/tst-cnd-basic.c index 84b7f5f647..eb2fb6a77e 100644 --- a/nptl/tst-cnd-basic.c +++ b/nptl/tst-cnd-basic.c @@ -31,8 +31,14 @@ static mtx_t mutex; static int signal_parent (void) { + /* Acquire the lock so that cnd_signal does not run until + cnd_timedwait has been called. */ + if (mtx_lock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_lock failed"); if (cnd_signal (&cond) != thrd_success) FAIL_EXIT1 ("cnd_signal"); + if (mtx_unlock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_unlock"); thrd_exit (thrd_success); } @@ -47,6 +53,9 @@ do_test (void) if (mtx_init (&mutex, mtx_plain) != thrd_success) FAIL_EXIT1 ("mtx_init failed"); + if (mtx_lock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_lock failed"); + if (thrd_create (&id, (thrd_start_t) signal_parent, NULL) != thrd_success) FAIL_EXIT1 ("thrd_create failed"); @@ -59,6 +68,9 @@ do_test (void) if (thrd_join (id, NULL) != thrd_success) FAIL_EXIT1 ("thrd_join failed"); + if (mtx_unlock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_unlock"); + mtx_destroy (&mutex); cnd_destroy (&cond); diff --git a/nptl/tst-cnd-broadcast.c b/nptl/tst-cnd-broadcast.c index 90d6843154..62a4ab5a39 100644 --- a/nptl/tst-cnd-broadcast.c +++ b/nptl/tst-cnd-broadcast.c @@ -17,6 +17,7 @@ <http://www.gnu.org/licenses/>. */ #include <threads.h> +#include <stdbool.h> #include <stdio.h> #include <unistd.h> @@ -28,12 +29,16 @@ static cnd_t cond; /* Mutex to control wait on cond. */ static mtx_t mutex; +/* Number of threads which have entered the cnd_wait region. */ +static unsigned int waiting_threads; + /* Code executed by each thread. */ static int child_wait (void* data) { /* Wait until parent thread sends broadcast here. */ mtx_lock (&mutex); + ++waiting_threads; cnd_wait (&cond, &mutex); mtx_unlock (&mutex); @@ -61,7 +66,16 @@ do_test (void) } /* Wait for other threads to reach their wait func. */ - thrd_sleep (&((struct timespec){.tv_sec = 2}), NULL); + while (true) + { + mtx_lock (&mutex); + TEST_VERIFY (waiting_threads <= N); + bool done_waiting = waiting_threads == N; + mtx_unlock (&mutex); + if (done_waiting) + break; + thrd_sleep (&((struct timespec){.tv_nsec = 100 * 1000 * 1000}), NULL); + } mtx_lock (&mutex); if (cnd_broadcast (&cond) != thrd_success) diff --git a/nptl/tst-cnd-timedwait.c b/nptl/tst-cnd-timedwait.c index 45a1512940..7d8a8e3557 100644 --- a/nptl/tst-cnd-timedwait.c +++ b/nptl/tst-cnd-timedwait.c @@ -31,8 +31,14 @@ static mtx_t mutex; static int signal_parent (void *arg) { + /* Acquire the lock so that cnd_signal does not run until + cnd_timedwait has been called. */ + if (mtx_lock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_lock failed"); if (cnd_signal (&cond) != thrd_success) FAIL_EXIT1 ("cnd_signal failed"); + if (mtx_unlock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_unlock"); thrd_exit (thrd_success); } @@ -47,10 +53,15 @@ do_test (void) FAIL_EXIT1 ("cnd_init failed"); if (mtx_init (&mutex, mtx_plain) != thrd_success) FAIL_EXIT1 ("mtx_init failed"); + if (mtx_lock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_lock failed"); if (clock_gettime (CLOCK_REALTIME, &w_time) != 0) FAIL_EXIT1 ("clock_gettime failed"); - w_time.tv_nsec += 150000; + + /* This needs to be sufficiently long to prevent the cnd_timedwait + call from timing out. */ + w_time.tv_sec += 3600; if (thrd_create (&id, signal_parent, NULL) != thrd_success) FAIL_EXIT1 ("thrd_create failed"); @@ -61,6 +72,9 @@ do_test (void) if (thrd_join (id, NULL) != thrd_success) FAIL_EXIT1 ("thrd_join failed"); + if (mtx_unlock (&mutex) != thrd_success) + FAIL_EXIT1 ("mtx_unlock"); + mtx_destroy (&mutex); cnd_destroy (&cond); diff --git a/nptl/tst-mtx-timedlock.c b/nptl/tst-mtx-timedlock.c index dcae828fb2..616db722eb 100644 --- a/nptl/tst-mtx-timedlock.c +++ b/nptl/tst-mtx-timedlock.c @@ -78,6 +78,11 @@ do_test (void) /* Tiny amount of time, to assure that if any thread finds it busy. It will receive thrd_timedout. */ wait_time.tv_nsec += 1; + if (wait_time.tv_nsec == 1000 * 1000 * 1000) + { + wait_time.tv_sec += 1; + wait_time.tv_nsec = 0; + } if (thrd_create (&id, child_add, NULL) != thrd_success) FAIL_EXIT1 ("thrd_create failed");