[vlc-devel] [PATCH 1/2] win32: implement non-recursive locks

[Rémi Denis-Courmont]

Le 2016-06-21 12:56, Steve Lhomme a écrit :
> locked & b_recursive are always accessed in the CriticalSection

It probably works, but I still don't see why you need a counter, 
insofar as you use a Win32 critical section. The recursion counter is 
already provided by Win32. AFAICT, a boolean is enough.

>
> --
> simple rebase of https://patches.videolan.org/patch/13641/
> ---
>  include/vlc_threads.h | 12 +++++-------
>  src/win32/thread.c    | 30 ++++++++++++++++++++++++------
>  2 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
> index 33b9b53..1fe22bf 100644
> --- a/include/vlc_threads.h
> +++ b/include/vlc_threads.h
> @@ -57,18 +57,16 @@ typedef struct vlc_thread *vlc_thread_t;
>  # define LIBVLC_NEED_SLEEP
>  typedef struct
>  {
> +    int locked;
> +    bool b_recursive;
>      bool dynamic;
>      union
>      {
> -        struct
> -        {
> -            bool locked;
> -            unsigned long contention;
> -        };
> -        CRITICAL_SECTION mutex;
> +        unsigned long contention; /* static */
> +        CRITICAL_SECTION mutex;   /* dynamic */
>      };
>  } vlc_mutex_t;
> -#define VLC_STATIC_MUTEX { false, { { false, 0 } } }
> +#define VLC_STATIC_MUTEX { 0, false, false, { 0 } }
>  #define LIBVLC_NEED_CONDVAR
>  #define LIBVLC_NEED_SEMAPHORE
>  #define LIBVLC_NEED_RWLOCK
> diff --git a/src/win32/thread.c b/src/win32/thread.c
> index 9eb3db7..8d53185 100644
> --- a/src/win32/thread.c
> +++ b/src/win32/thread.c
> @@ -96,15 +96,17 @@ static BOOL WINAPI
> SleepConditionVariableFallback(CONDITION_VARIABLE *cv,
>  /*** Mutexes ***/
>  void vlc_mutex_init( vlc_mutex_t *p_mutex )
>  {
> -    /* This creates a recursive mutex. This is OK as fast mutexes 
> have
> -     * no defined behavior in case of recursive locking. */
>      InitializeCriticalSection (&p_mutex->mutex);
> +    p_mutex->locked = 0;
> +    p_mutex->b_recursive = false;
>      p_mutex->dynamic = true;
>  }
>
>  void vlc_mutex_init_recursive( vlc_mutex_t *p_mutex )
>  {
>      InitializeCriticalSection( &p_mutex->mutex );
> +    p_mutex->locked = 0;
> +    p_mutex->b_recursive = true;
>      p_mutex->dynamic = true;
>  }
>
> @@ -126,12 +128,14 @@ void vlc_mutex_lock (vlc_mutex_t *p_mutex)
>              SleepConditionVariableCS(&super_variable, &super_mutex,
> INFINITE);
>              p_mutex->contention--;
>          }
> -        p_mutex->locked = true;
> +        p_mutex->locked = 1;
>          LeaveCriticalSection(&super_mutex);
>          return;
>      }
>
>      EnterCriticalSection (&p_mutex->mutex);
> +    assert(!p_mutex->locked || p_mutex->b_recursive);
> +    p_mutex->locked++;
>  }
>
>  int vlc_mutex_trylock (vlc_mutex_t *p_mutex)
> @@ -143,14 +147,26 @@ int vlc_mutex_trylock (vlc_mutex_t *p_mutex)
>          EnterCriticalSection(&super_mutex);
>          if (!p_mutex->locked)
>          {
> -            p_mutex->locked = true;
> +            p_mutex->locked = 1;
>              ret = 0;
>          }
>          LeaveCriticalSection(&super_mutex);
>          return ret;
>      }
>
> -    return TryEnterCriticalSection (&p_mutex->mutex) ? 0 : EBUSY;
> +    int ret = TryEnterCriticalSection (&p_mutex->mutex);
> +    if (ret == 0)
> +        return EBUSY;
> +
> +    assert(!p_mutex->locked || p_mutex->b_recursive);
> +    if (unlikely(p_mutex->locked && !p_mutex->b_recursive))
> +    {
> +        LeaveCriticalSection (&p_mutex->mutex);
> +        return EDEADLK; /* we already had the non-recursive lock */
> +    }
> +
> +    p_mutex->locked++;
> +    return 0;
>  }
>
>  void vlc_mutex_unlock (vlc_mutex_t *p_mutex)
> @@ -159,13 +175,15 @@ void vlc_mutex_unlock (vlc_mutex_t *p_mutex)
>      {   /* static mutexes */
>          EnterCriticalSection(&super_mutex);
>          assert (p_mutex->locked);
> -        p_mutex->locked = false;
> +        p_mutex->locked = 0;
>          if (p_mutex->contention)
>              WakeAllConditionVariable(&super_variable);
>          LeaveCriticalSection(&super_mutex);
>          return;
>      }
>
> +    p_mutex->locked--;
> +    assert(p_mutex->locked >= 0);
>      LeaveCriticalSection (&p_mutex->mutex);
>  }

-- 
Rémi Denis-Courmont
http://www.remlab.net/