[vlc-devel] [PATCH 3/5] threads: VLC_CANCEL_ADDR_SET/CLEAR always use an atomic_int internally
Rémi Denis-Courmont
remi at remlab.net
Mon Feb 10 11:43:34 CET 2020
Hi,
In hindsight, those two functions are not used, and probably should not be used, outside the core, so they could be de-inlined and split from vlc_control_cancel() as internal functions.
The Linux/Android atomic_wait backend only supports 32-bits. I have untested implementations for FreeBSD and OpenBSD with the same limitation, that I could post.
Le 10 février 2020 10:36:37 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>On 2020-02-07 16:38, Rémi Denis-Courmont wrote:
>> Hi,
>>
>> Where do you get that from?
>
> From
>https://code.videolan.org/videolan/vlc/blob/master/src/win32/thread.c#L68
>
>It's not clear for Android though
>https://code.videolan.org/videolan/vlc/blob/master/src/android/thread.c#L142
>But in the end that's also what is used:
>https://code.videolan.org/videolan/vlc/blob/master/src/android/thread.c#L287
>
>> The Windows backend can handle any suitably
>> aligned 32-bits blob. And unsigned poses less concerns with wrapping.
>
>I think it's better to have this in the signature, even if it involves
>a
>conscious cast when the types don't exactly match.
>
>There's currently no check on the caller (ultimately the caller is
>vlc_cancel_addr_prepare). Resulting in passing a plain unsigned pointer
>
>where an atomic 32 bits pointer (or can it be another size on other
>platforms ?) is expected.
>
>The only check there is is that the condition value one can fit in the
>type that is assumed on all the backends. And even then if it was an
>atomic char it would probably pass the static assert but fail at
>runtime.
>
>I'm OK with using atomic_uint instead of atomic_int for this patch, and
>
>update the backends accordingly. We only set the value to 1.
>
>> Le 7 février 2020 17:02:08 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz>
>a
>> écrit :
>>
>> Not a atomic_uint.
>>
>------------------------------------------------------------------------
>> include/vlc_threads.h | 4 ++--
>> src/android/thread.c | 4 ++--
>> src/misc/threads.c | 26 +++++++++++++-------------
>> src/win32/thread.c | 4 ++--
>> 4 files changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
>> index 8a39c610870..7ae25931b72 100644
>> --- a/include/vlc_threads.h
>> +++ b/include/vlc_threads.h
>> @@ -1028,8 +1028,8 @@ enum
>> {
>> VLC_CLEANUP_PUSH,
>> VLC_CLEANUP_POP,
>> - VLC_CANCEL_ADDR_SET,
>> - VLC_CANCEL_ADDR_CLEAR,
>> + VLC_CANCEL_ADDR_SET, /* atomic_int* */
>> + VLC_CANCEL_ADDR_CLEAR, /* atomic_int* */
>> };
>>
>> #if defined (LIBVLC_USE_PTHREAD_CLEANUP)
>> diff --git a/src/android/thread.c b/src/android/thread.c
>> index ca715418d41..aec539b4f03 100644
>> --- a/src/android/thread.c
>> +++ b/src/android/thread.c
>> @@ -341,7 +341,7 @@ void vlc_control_cancel(int cmd, ...)
>> {
>> case VLC_CANCEL_ADDR_SET:
>> {
>> - void *addr = va_arg(ap, void *);
>> + void *addr = va_arg(ap, atomic_int *);
>>
>> vlc_mutex_lock(&th->wait.lock);
>> assert(th->wait.addr == NULL);
>> @@ -352,7 +352,7 @@ void vlc_control_cancel(int cmd, ...)
>>
>> case VLC_CANCEL_ADDR_CLEAR:
>> {
>> - void *addr = va_arg(ap, void *);
>> + void *addr = va_arg(ap, atomic_int *);
>>
>> vlc_mutex_lock(&th->wait.lock);
>> assert(th->wait.addr == addr);
>> diff --git a/src/misc/threads.c b/src/misc/threads.c
>> index 501a17ac9b3..87c3da03d9e 100644
>> --- a/src/misc/threads.c
>> +++ b/src/misc/threads.c
>> @@ -149,7 +149,7 @@ bool vlc_mutex_marked(const vlc_mutex_t
>*mutex)
>> #if defined(LIBVLC_NEED_SLEEP) || defined(LIBVLC_NEED_CONDVAR)
>> #include <stdatomic.h>
>>
>> -static void vlc_cancel_addr_prepare(void *addr)
>> +static void vlc_cancel_addr_prepare(atomic_int *addr)
>> {
>> /* Let thread subsystem on address to broadcast for
>cancellation */
>> vlc_cancel_addr_set(addr);
>> @@ -159,7 +159,7 @@ static void vlc_cancel_addr_prepare(void
>*addr)
>> vlc_cleanup_pop();
>> }
>>
>> -static void vlc_cancel_addr_finish(void *addr)
>> +static void vlc_cancel_addr_finish(atomic_int *addr)
>> {
>> vlc_cancel_addr_clear(addr);
>> /* Act on cancellation as potential wake-up source */
>> @@ -193,14 +193,14 @@ void (vlc_tick_sleep)(vlc_tick_t delay)
>> #ifdef LIBVLC_NEED_CONDVAR
>> #include <stdalign.h>
>>
>> -static inline atomic_uint *vlc_cond_value(vlc_cond_t *cond)
>> +static inline atomic_int *vlc_cond_value(vlc_cond_t *cond)
>> {
>> /* XXX: ugly but avoids including stdatomic.h in
>vlc_threads.h */
>> - static_assert (sizeof (cond->value) <= sizeof (atomic_uint),
>> + static_assert (sizeof (cond->value) <= sizeof (atomic_int),
>> "Size mismatch!");
>> - static_assert ((alignof (cond->value) % alignof
>(atomic_uint)) == 0,
>> + static_assert ((alignof (cond->value) % alignof
>(atomic_int)) == 0,
>> "Alignment mismatch");
>> - return (atomic_uint *)&cond->value;
>> + return (atomic_int *)&cond->value;
>> }
>>
>> void vlc_cond_init(vlc_cond_t *cond)
>> @@ -247,7 +247,7 @@ void vlc_cond_broadcast(vlc_cond_t *cond)
>>
>> void vlc_cond_wait(vlc_cond_t *cond, vlc_mutex_t *mutex)
>> {
>> - unsigned value = atomic_load_explicit(vlc_cond_value(cond),
>> + int value = atomic_load_explicit(vlc_cond_value(cond),
>> memory_order_relaxed);
>> while (value & 1)
>> {
>> @@ -258,20 +258,20 @@ void vlc_cond_wait(vlc_cond_t *cond,
>vlc_mutex_t *mutex)
>> value++;
>> }
>>
>> - vlc_cancel_addr_prepare(&cond->value);
>> + vlc_cancel_addr_prepare(vlc_cond_value(cond));
>> vlc_mutex_unlock(mutex);
>>
>> vlc_atomic_wait(&cond->value, value);
>>
>> vlc_mutex_lock(mutex);
>> - vlc_cancel_addr_finish(&cond->value);
>> + vlc_cancel_addr_finish(vlc_cond_value(cond));
>> }
>>
>> static int vlc_cond_wait_delay(vlc_cond_t *cond, vlc_mutex_t
>*mutex,
>> vlc_tick_t delay)
>> {
>> - unsigned value = atomic_load_explicit(vlc_cond_value(cond),
>> - memory_order_relaxed);
>> + int 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,
>> @@ -281,7 +281,7 @@ static int vlc_cond_wait_delay(vlc_cond_t
>*cond, vlc_mutex_t *mutex,
>> value++;
>> }
>>
>> - vlc_cancel_addr_prepare(&cond->value);
>> + vlc_cancel_addr_prepare(vlc_cond_value(cond));
>> vlc_mutex_unlock(mutex);
>>
>> if (delay > 0)
>> @@ -290,7 +290,7 @@ static int vlc_cond_wait_delay(vlc_cond_t
>*cond, vlc_mutex_t *mutex,
>> value = 0;
>>
>> vlc_mutex_lock(mutex);
>> - vlc_cancel_addr_finish(&cond->value);
>> + vlc_cancel_addr_finish(vlc_cond_value(cond));
>>
>> return value ? 0 : ETIMEDOUT;
>> }
>> diff --git a/src/win32/thread.c b/src/win32/thread.c
>> index 6af2eb83852..10c36e9ed82 100644
>> --- a/src/win32/thread.c
>> +++ b/src/win32/thread.c
>> @@ -669,7 +669,7 @@ void vlc_control_cancel (int cmd, ...)
>>
>> case VLC_CANCEL_ADDR_SET:
>> {
>> - void *addr = va_arg(ap, void *);
>> + void *addr = va_arg(ap, atomic_int *);
>>
>> EnterCriticalSection(&th->wait.lock);
>> assert(th->wait.addr == NULL);
>> @@ -680,7 +680,7 @@ void vlc_control_cancel (int cmd, ...)
>>
>> case VLC_CANCEL_ADDR_CLEAR:
>> {
>> - void *addr = va_arg(ap, void *);
>> + void *addr = va_arg(ap, atomic_int *);
>>
>> EnterCriticalSection(&th->wait.lock);
>> assert(th->wait.addr == addr);
>>
>>
>> --
>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>excuser
>> ma brièveté.
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200210/3fa950d4/attachment.html>
More information about the vlc-devel
mailing list