[vlc-devel] [vlc-commits] threads: fix race in vlc_cond_wait()

Steve Lhomme robux4 at gmail.com
Thu Jun 9 18:11:38 CEST 2016


On Thu, Jun 2, 2016 at 8:14 AM, Rémi Denis-Courmont <git at videolan.org> wrote:
> vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Wed Jun  1 21:47:38 2016 +0300| [b269f60e182c9a26f046518cb52439f88b4a3c05] | committer: Rémi Denis-Courmont
>
> threads: fix race in vlc_cond_wait()
>
> Could lose wake-up if vlc_cond_wait() in one thread, then
> vlc_cond_signal() in anotherthread, then vlc_cond_wait() in a third
> thread.

There's still some issues with 3 threads scenario.

During the first wait, between the time the time you
vlc_mutex_unlock(mutex) and vlc_wait_addr() there's time for a thread
to vlc_cond_signal() (will increment to the next odd value) and
another thread to vlc_cond_wait() (will increment to the next even
value). So the first thread will be not be waiting because the value
it will tried to match to wait has already changed. Is that the
expected behaviour ? In this case the vlc_cond_signal() will allow 2
condition waits to pass.

>> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=b269f60e182c9a26f046518cb52439f88b4a3c05
> ---
>
>  include/vlc_threads.h |    2 +-
>  src/android/thread.c  |    2 +-
>  src/misc/threads.c    |   46 ++++++++++++++++++++++++++++++----------------
>  src/win32/thread.c    |    2 +-
>  4 files changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
> index 0f4aefe..33f2a8a 100644
> --- a/include/vlc_threads.h
> +++ b/include/vlc_threads.h
> @@ -326,7 +326,7 @@ typedef struct vlc_timer *vlc_timer_t;
>  #ifdef LIBVLC_NEED_CONDVAR
>  typedef struct
>  {
> -    int value;
> +    unsigned value;
>  } vlc_cond_t;
>  # define VLC_STATIC_COND { 0 }
>  #endif
> diff --git a/src/android/thread.c b/src/android/thread.c
> index be18204..8e12115 100644
> --- a/src/android/thread.c
> +++ b/src/android/thread.c
> @@ -317,7 +317,7 @@ void vlc_cancel (vlc_thread_t thread_id)
>      addr = thread_id->wait.addr;
>      if (addr != NULL)
>      {
> -        atomic_fetch_and_explicit(addr, -2, memory_order_relaxed);
> +        atomic_fetch_or_explicit(addr, 1, memory_order_relaxed);
>          vlc_addr_broadcast(addr);
>      }
>      vlc_mutex_unlock(&thread_id->wait.lock);
> diff --git a/src/misc/threads.c b/src/misc/threads.c
> index 044296f..8bdcb1f 100644
> --- a/src/misc/threads.c
> +++ b/src/misc/threads.c
> @@ -93,14 +93,14 @@ void (msleep)(mtime_t delay)
>  #ifdef LIBVLC_NEED_CONDVAR
>  #include <stdalign.h>
>
> -static inline atomic_int *vlc_cond_value(vlc_cond_t *cond)
> +static inline atomic_uint *vlc_cond_value(vlc_cond_t *cond)
>  {
>      /* XXX: ugly but avoids including vlc_atomic.h in vlc_threads.h */
> -    static_assert (sizeof (cond->value) <= sizeof (atomic_int),
> +    static_assert (sizeof (cond->value) <= sizeof (atomic_uint),
>                     "Size mismatch!");
> -    static_assert ((alignof (cond->value) % alignof (atomic_int)) == 0,
> +    static_assert ((alignof (cond->value) % alignof (atomic_uint)) == 0,
>                     "Alignment mismatch");
> -    return (atomic_int *)&cond->value;
> +    return (atomic_uint *)&cond->value;
>  }
>
>  void vlc_cond_init(vlc_cond_t *cond)
> @@ -127,30 +127,36 @@ void vlc_cond_destroy(vlc_cond_t *cond)
>  void vlc_cond_signal(vlc_cond_t *cond)
>  {
>      /* Probably the best documented approach is that of Bionic: increment
> -     * the futex here, and simply load the value in cond_wait(). This has a bug
> +     * the futex here, and simply load the value in cnd_wait(). This has a bug
>       * as unlikely as well-known: signals get lost if the futex is incremented
>       * an exact multiple of 2^(CHAR_BIT * sizeof (int)) times.
>       *
> -     * Here, we simply clear the low order bit, while cond_wait() sets it.
> -     * It makes cond_wait() somewhat slower, but it should be free of bugs,
> -     * leaves the other bits free for other uses (e.g. flags).
> +     * A different presumably bug-free solution is used here:
> +     * - cnd_signal() sets the futex to the equal-or-next odd value, while
> +     * - cnd_wait() sets the futex to the equal-or-next even value.
>       **/
> -    atomic_fetch_and_explicit(vlc_cond_value(cond), -2, memory_order_relaxed);
> -    /* We have to wake the futex even if the low order bit is cleared, as there
> -     * could be more than one thread queued, not all of them already awoken. */
> +    atomic_fetch_or_explicit(vlc_cond_value(cond), 1, memory_order_relaxed);
>      vlc_addr_signal(&cond->value);
>  }
>
>  void vlc_cond_broadcast(vlc_cond_t *cond)
>  {
> -    atomic_fetch_and_explicit(vlc_cond_value(cond), -2, memory_order_relaxed);
> +    atomic_fetch_or_explicit(vlc_cond_value(cond), 1, memory_order_relaxed);
>      vlc_addr_broadcast(&cond->value);
>  }
>
>  void vlc_cond_wait(vlc_cond_t *cond, vlc_mutex_t *mutex)
>  {
> -    int value = atomic_fetch_or_explicit(vlc_cond_value(cond), 1,
> -                                         memory_order_relaxed) | 1;
> +    unsigned value = atomic_load_explicit(vlc_cond_value(cond),
> +                                     memory_order_relaxed);
> +    while (value & 1)
> +    {
> +        if (atomic_compare_exchange_weak_explicit(vlc_cond_value(cond), &value,
> +                                                  value + 1,
> +                                                  memory_order_relaxed,
> +                                                  memory_order_relaxed))
> +            value++;
> +    }
>
>      vlc_cancel_addr_prepare(&cond->value);
>      vlc_mutex_unlock(mutex);
> @@ -164,8 +170,16 @@ void vlc_cond_wait(vlc_cond_t *cond, vlc_mutex_t *mutex)
>  static int vlc_cond_wait_delay(vlc_cond_t *cond, vlc_mutex_t *mutex,
>                                 mtime_t delay)
>  {
> -    int value = atomic_fetch_or_explicit(vlc_cond_value(cond), 1,
> -                                         memory_order_relaxed) | 1;
> +    unsigned value = atomic_load_explicit(vlc_cond_value(cond),
> +                                          memory_order_relaxed);
> +    while (value & 1)
> +    {
> +        if (atomic_compare_exchange_weak_explicit(vlc_cond_value(cond), &value,
> +                                                  value + 1,
> +                                                  memory_order_relaxed,
> +                                                  memory_order_relaxed))
> +            value++;
> +    }
>
>      vlc_cancel_addr_prepare(&cond->value);
>      vlc_mutex_unlock(mutex);
> diff --git a/src/win32/thread.c b/src/win32/thread.c
> index 8d74d6f..88aaa5b 100644
> --- a/src/win32/thread.c
> +++ b/src/win32/thread.c
> @@ -640,7 +640,7 @@ void vlc_cancel (vlc_thread_t th)
>      EnterCriticalSection(&th->wait.lock);
>      if (th->wait.addr != NULL)
>      {
> -        atomic_fetch_and_explicit(th->wait.addr, -2, memory_order_relaxed);
> +        atomic_fetch_or_explicit(th->wait.addr, 1, memory_order_relaxed);
>          vlc_addr_broadcast(th->wait.addr);
>      }
>      LeaveCriticalSection(&th->wait.lock);
>
> _______________________________________________
> vlc-commits mailing list
> vlc-commits at videolan.org
> https://mailman.videolan.org/listinfo/vlc-commits


More information about the vlc-devel mailing list