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

[Rémi Denis-Courmont]

Le 2016-06-06 10:19, Steve Lhomme a écrit :
> For non-recursive locks we keep track of the thread having the lock 
> and make
> sure it's not the current thread that already had the lock.
>
> lock_thread_id & b_recursive are always accessed in the 
> CriticalSection
> ---
>  include/vlc_threads.h |  9 ++++++++-
>  src/win32/thread.c    | 18 +++++++++++++++---
>  2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
> index 33b9b53..becd9d6 100644
> --- a/include/vlc_threads.h
> +++ b/include/vlc_threads.h
> @@ -65,7 +65,14 @@ typedef struct
>              bool locked;
>              unsigned long contention;
>          };
> -        CRITICAL_SECTION mutex;
> +        struct
> +        {
> +            CRITICAL_SECTION mutex;
> +
> +            /* non-recursive mutex */
> +            bool b_recursive;
> +            unsigned long lock_thread_id;

You need two booleans, of which one (locked) already exists in the 
static mutex case.

Moving all three booleans together at the top of the struct would 
provide better packing.

> +        };
>      };
>  } vlc_mutex_t;
>  #define VLC_STATIC_MUTEX { false, { { false, 0 } } }
> diff --git a/src/win32/thread.c b/src/win32/thread.c
> index 963a8d7..de611e8 100644
> --- a/src/win32/thread.c
> +++ b/src/win32/thread.c
> @@ -69,16 +69,16 @@ struct vlc_thread
>  /*** 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->dynamic = true;
> +    p_mutex->b_recursive = false;
>  }
>
>  void vlc_mutex_init_recursive( vlc_mutex_t *p_mutex )
>  {
>      InitializeCriticalSection( &p_mutex->mutex );
>      p_mutex->dynamic = true;
> +    p_mutex->b_recursive = true;
>  }
>
>
> @@ -109,6 +109,8 @@ void vlc_mutex_lock (vlc_mutex_t *p_mutex)
>      }
>
>      EnterCriticalSection (&p_mutex->mutex);
> +    assert(p_mutex->b_recursive || p_mutex->lock_thread_id !=
> vlc_thread_id());
> +    p_mutex->lock_thread_id = vlc_thread_id();
>  }
>
>  int vlc_mutex_trylock (vlc_mutex_t *p_mutex)
> @@ -128,7 +130,16 @@ int vlc_mutex_trylock (vlc_mutex_t *p_mutex)
>          return ret;
>      }
>
> -    return TryEnterCriticalSection (&p_mutex->mutex) ? 0 : EBUSY;
> +    int ret = TryEnterCriticalSection (&p_mutex->mutex);
> +    assert(p_mutex->b_recursive || p_mutex->lock_thread_id !=
> vlc_thread_id());
> +    if (!ret)
> +        return EBUSY;
> +
> +    if (!p_mutex->b_recursive && p_mutex->lock_thread_id == 
> vlc_thread_id())
> +        return EBUSY; /* we already had the non-recursive lock */
> +
> +    p_mutex->lock_thread_id = vlc_thread_id();
> +    return 0;

I suspect that if (recursive) ... else ... would be more legible.

>  }
>
>  void vlc_mutex_unlock (vlc_mutex_t *p_mutex)
> @@ -146,6 +157,7 @@ void vlc_mutex_unlock (vlc_mutex_t *p_mutex)
>          return;
>      }
>
> +    p_mutex->lock_thread_id = 0;
>      LeaveCriticalSection (&p_mutex->mutex);
>  }

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