This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] C11 threads: Fix timeout and locking issues
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Tue, 24 Jul 2018 18:25:36 -0300
- Subject: Re: [PATCH] C11 threads: Fix timeout and locking issues
- References: <20180724211805.E5409439942EF@oldenburg.str.redhat.com>
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");
>