This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] C11 threads: Fix timeout and locking issues


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");
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]