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

[Steve Lhomme]

On Fri, Jun 24, 2016 at 4:49 AM, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 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.

The counter allows this assert in vlc_mutex_unlock()
assert(p_mutex->locked >= 0);

So we can catch un-matched unlocks. Also if we don't we can end up in
harder to decypher blocking issues. Here's the doc of
LeaveCriticalSection()

"If a thread calls LeaveCriticalSection when it does not have
ownership of the specified critical section object, an error occurs
that may cause another thread using EnterCriticalSection to wait
indefinitely."

>>
>> --
>> 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/
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel