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

[Steve Lhomme]

On Mon, Jun 6, 2016 at 10:51 AM, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 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.

Indeed. In a later version I already don't test with vlc_thread_id()
but simply 0 or not. The next logical step is the bool you mention.

> 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.

OK

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