[waffle] [PATCH v2 3/3] wcore_error_unittest: Fix race condition leading to forever loop

[Chad Versace]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/30/2012 08:52 AM, Pauli Nieminen wrote:
> Reading non-volatile value in loop without function calls allows C
> compiler to generate forever loop without checking the memory value. To
> avoid the problem locking or atomic operations could be used.
> 
> But there is better solution of using pthread_barrier_t that test case
> was trying to simulate with mutex and conditional. Barrier promises that
> all threads are released when requested number of threads are blocked in
> pthread_barrier_wait.
> 
> Signed-off-by: Pauli Nieminen <pauli.nieminen at linux.intel.com>
> ---
>  tests/unittests/waffle/core/wcore_error_unittest.c |   37 ++++----------------
>  1 file changed, 7 insertions(+), 30 deletions(-)

This patch is almost a revert of cc85550, in which I removed
pthread_barrier_t and emulated it with pthread_cond_t. Yes, pthread_barrier_t
is better pthread_cond_t, but MacOS 10.7 doesn't support barriers.

Is this the problematic loop that you are referring to?
    while (num_threads_waiting < NUM_THREADS)
        ;;

> 
> diff --git a/tests/unittests/waffle/core/wcore_error_unittest.c b/tests/unittests/waffle/core/wcore_error_unittest.c
> index 2a1f5c1..0098450 100644
> --- a/tests/unittests/waffle/core/wcore_error_unittest.c
> +++ b/tests/unittests/waffle/core/wcore_error_unittest.c
> @@ -131,13 +131,7 @@ struct thread_arg {
>      int thread_id;
>  
>      /// Protects `num_threads_waiting` and `cond`.
> -    pthread_mutex_t *mutex;
> -
> -    /// Satisfied when `num_threads_waiting == TOTAL_THREADS`.
> -    pthread_cond_t *cond;
> -
> -    /// Number of threads waiting on `cond`.
> -    int *num_threads_waiting;
> +    pthread_barrier_t *barrier;
>  };
>  
>  /// The start routine given to threads in test wcore_error.thread_local.
> @@ -159,13 +153,7 @@ thread_start(struct thread_arg *a)
>      // Each thread sets its error code.
>      wcore_error(error_code);
>  
> -    // Wait for all threads to set their error codes, thus giving
> -    // the threads opportunity to clobber each other's codes.
> -    pthread_mutex_lock(a->mutex); {
> -        *a->num_threads_waiting += 1;
> -        pthread_cond_wait(a->cond, a->mutex);
> -        pthread_mutex_unlock(a->mutex);
> -    }
> +    pthread_barrier_wait(a->barrier);
>  
>      // Verify that the threads did not clobber each other's
>      // error codes.
> @@ -177,23 +165,18 @@ thread_start(struct thread_arg *a)
>  // Test that threads do not clobber each other's error codes.
>  TEST(wcore_error, thread_local)
>  {
> -    pthread_mutex_t mutex;
> -    pthread_cond_t cond;
> -    int num_threads_waiting = 0;
> +    pthread_barrier_t barrier;
>  
>      pthread_t threads[NUM_THREADS];
>      struct thread_arg thread_args[NUM_THREADS];
>      bool exit_codes[NUM_THREADS];
>  
> -    pthread_mutex_init(&mutex, NULL);
> -    pthread_cond_init(&cond, NULL);
> +    pthread_barrier_init(&barrier, NULL, NUM_THREADS + 1);
>  
>      for (intptr_t i = 0; i < NUM_THREADS; ++i) {
>          struct thread_arg *a = &thread_args[i];
>          a->thread_id = i;
> -        a->mutex = &mutex;
> -        a->cond = &cond;
> -        a->num_threads_waiting = &num_threads_waiting;
> +        a->barrier = &barrier;
>  
>          pthread_create(&threads[i], NULL,
>                        (void* (*)(void*)) thread_start,
> @@ -202,20 +185,14 @@ TEST(wcore_error, thread_local)
>  
>      // Wait for all threads to set their error codes, thus giving
>      // the threads opportunity to clobber each other's codes.
> -    while (num_threads_waiting < NUM_THREADS)
> -        ;;
> -    pthread_mutex_lock(&mutex); {
> -        pthread_cond_broadcast(&cond);
> -        pthread_mutex_unlock(&mutex);
> -    }
> +    pthread_barrier_wait(&barrier);
>  
>      for (int i = 0; i < NUM_THREADS; ++i) {
>          pthread_join(threads[i], (void**) &exit_codes[i]);
>          EXPECT_TRUE(exit_codes[i] == true);
>      }
>  
> -    pthread_cond_destroy(&cond);
> -    pthread_mutex_destroy(&mutex);
> +    pthread_barrier_destroy(&barrier);
>  }
>  
>  void
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPxqsFAAoJEAIvNt057x8iEjsP/2gL5jqUD8NXf6sk2ReXoyH+
gKlOmH0/j4mMim+hxfnGIe3eg2VcP/0vR6be5tgwCuoOEgz+cUEPhPp2lpqbdFDR
zUuWRyoGzZIjI7o/+F3pIVZSO0s5e3o/6e49TcpBI0ftj7F2CPUiSbYUtc5ZiD91
oT0duIGdcLE1AVOZuAIR1MHPGoQiI0Bjd8PPneMZzK5E0kxLkbTy/Y/eQ7jroztm
Igq/WufrcnG9jICKrJI4doA0jzhChYmSRypryNgJx2qekSLq+hefS5QUJVlcOrvT
gWDqXGmfSlcBLgIXy1B+tTp2nItpRfSHBOL/WNSaXIjVg556edgHmEZePhs4mY3y
LyRj2mDUQkNC/AC1Asx82jDw/WaWcLwZRqVjrDIAC7HtRQWLO9XLRiGCUoYk6tj8
e/Mg03bEEexpGTYhTmpQteTAR5/+ffCpHsmkLfzKEHqoE3Et/1I1VsbKEl29PVOG
RtI2qCdE9dTn6Lr9t+NzU8RRed8hBMqRaAUMJJC2E2OQW/PkfpbPnoZrTQWdNqvh
IFXk9p468Oa3ohoiIuZbLcM7ytKujGZE5yCls98hjJ2AYs0kBPeKs6Ouv0+5QGJW
UYxtIUePZGDhHqSLwEMMAnq4QiOVNxOTBmGmGtqpywvi/Ne6Y3Qt8JXzIYEOntZp
w327zF0jfJNv12Vefcle
=WF9E
-----END PGP SIGNATURE-----