[vlc-devel] [PATCH 1/7] posix: add fallback vlc_atomic_* implementation
Thomas Guillem
thomas at gllm.fr
Mon Feb 17 14:47:52 CET 2020
On Mon, Feb 17, 2020, at 14:35, Rémi Denis-Courmont wrote:
> I want the APIs to work on all platforms, and this adds a generic POSIX implementation. Exactly what it says already. I don't know what else you want to explain.
So why you don't write it in the commit log ?
Maybe it's just me, but I had trouble understanding this set. Explaining why you do a commit can be very helpful. Specially on set like this where you get the reason why only after the 4th or 5th commit.
Commit messages should explain 'why'. The 'what' should be in the diff/code/comment.
>
> Le 17 février 2020 14:39:08 GMT+02:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>>
>>
>> On Sun, Feb 16, 2020, at 18:50, Rémi Denis-Courmont wrote:
>>> src/Makefile.am | 3 ++
>>> src/posix/wait.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 131 insertions(+)
>>> create mode 100644 src/posix/wait.c
>>>
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index f2adf61095..0ad47fc105 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -482,6 +482,9 @@ libvlccore_la_SOURCES += \
>>> posix/plugin.c \
>>> posix/rand.c \
>>> posix/timer.c
>>> +if !HAVE_LINUX
>>> +libvlccore_la_SOURCES += posix/wait.c
>>> +endif
>>> if !HAVE_ANDROID
>>> libvlccore_la_SOURCES += posix/sort.c
>>> if !HAVE_DARWIN
>>> diff --git a/src/posix/wait.c b/src/posix/wait.c
>>> new file mode 100644
>>> index 0000000000..60f9c7626f
>>> --- /dev/null
>>> +++ b/src/posix/wait.c
>>> @@ -0,0 +1,128 @@
>>> +/*****************************************************************************
>>> + * wait.c : pthread back-end for vlc_atomic_wait
>>> + *****************************************************************************
>>> + * Copyright (C) 2019-2020 Rémi Denis-Courmont
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU Lesser General Public License as published by
>>> + * the Free Software Foundation; either version 2.1 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public License
>>> + * along with this program; if not, write to the Free Software Foundation,
>>> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
>>> + *****************************************************************************/
>>> +
>>> +#ifdef HAVE_CONFIG_H
>>> +# include "config.h"
>>> +#endif
>>> +
>>> +#include <vlc_common.h>
>>> +
>>> +#include <stdalign.h>
>>> +#include <stdatomic.h>
>>> +#include <errno.h>
>>> +#include <time.h>
>>> +
>>> +#include <sys/types.h>
>>> +#include <pthread.h>
>>> +
>>> +#define WAIT_BUCKET_INIT \
>>> + { PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER, 0 }
>>> +#define WAIT_BUCKET_INIT_2 WAIT_BUCKET_INIT, WAIT_BUCKET_INIT
>>> +#define WAIT_BUCKET_INIT_4 WAIT_BUCKET_INIT_2, WAIT_BUCKET_INIT_2
>>> +#define WAIT_BUCKET_INIT_8 WAIT_BUCKET_INIT_4, WAIT_BUCKET_INIT_4
>>> +#define WAIT_BUCKET_INIT_16 WAIT_BUCKET_INIT_8, WAIT_BUCKET_INIT_8
>>> +#define WAIT_BUCKET_INIT_32 WAIT_BUCKET_INIT_16, WAIT_BUCKET_INIT_16
>>
>>
>> If I understand correctly, this bucket is here to minimize the number of spurious wake, but it doesn't prevent it from happening, which is fine ?
>>
>>> +
>>> +static struct wait_bucket
>>> +{
>>> + pthread_mutex_t lock;
>>> + pthread_cond_t wait;
>>> + unsigned waiters;
>>> +} wait_buckets[32] = { WAIT_BUCKET_INIT_32 };
>>> +
>>> +static struct wait_bucket *wait_bucket_get(atomic_uint *addr)
>>> +{
>>> + uintptr_t u = (uintptr_t)addr;
>>> + size_t idx = (u / alignof (*addr)) % ARRAY_SIZE(wait_buckets);
>>> +
>>> + return &wait_buckets[idx];
>>> +}
>>> +
>>> +static struct wait_bucket *wait_bucket_enter(atomic_uint *addr)
>>> +{
>>> + struct wait_bucket *bucket = wait_bucket_get(addr);
>>> +
>>> + pthread_mutex_lock(&bucket->lock);
>>> + bucket->waiters++;
>>> + return bucket;
>>> +}
>>> +
>>> +static void wait_bucket_leave(void *data)
>>> +{
>>> + struct wait_bucket *bucket = data;
>>> +
>>> + bucket->waiters--;
>>> + pthread_mutex_unlock(&bucket->lock);
>>> +}
>>> +
>>> +void vlc_atomic_wait(void *addr, unsigned value)
>>> +{
>>> + atomic_uint *futex = addr;
>>> + struct wait_bucket *bucket = wait_bucket_enter(futex);
>>> +
>>> + pthread_cleanup_push(wait_bucket_leave, bucket);
>>> +
>>> + if (value == atomic_load_explicit(futex, memory_order_relaxed))
>>> + pthread_cond_wait(&bucket->wait, &bucket->lock);
>>> +
>>> + pthread_cleanup_pop(1);
>>> +}
>>> +
>>> +bool vlc_atomic_timedwait(void *addr, unsigned value, vlc_tick_t delay)
>>> +{
>>> + atomic_uint *futex = addr;
>>> + struct wait_bucket *bucket;
>>> + struct timespec ts;
>>> + lldiv_t d = lldiv(delay, CLOCK_FREQ);
>>> + int ret = 0;
>>> +
>>> + clock_gettime(CLOCK_MONOTONIC, &ts);
>>> + ts.tv_sec += d.quot;
>>> + ts.tv_nsec += NS_FROM_VLC_TICK(d.rem);
>>> +
>>> + if (ts.tv_nsec >= 1000000000) {
>>> + ts.tv_sec++;
>>> + ts.tv_nsec -= 1000000000;
>>> + }
>>> +
>>> + bucket = wait_bucket_enter(futex);
>>> + pthread_cleanup_push(wait_bucket_leave, bucket);
>>> +
>>> + if (value == atomic_load_explicit(futex, memory_order_relaxed))
>>> + ret = pthread_cond_timedwait(&bucket->wait, &bucket->lock, &ts);
>>> +
>>> + pthread_cleanup_pop(1);
>>> + return ret == 0;
>>> +}
>>> +
>>> +void vlc_atomic_notify_one(void *addr)
>>> +{
>>> + vlc_atomic_notify_all(addr);
>>> +}
>>> +
>>> +void vlc_atomic_notify_all(void *addr)
>>> +{
>>> + struct wait_bucket *bucket = wait_bucket_get(addr);
>>> +
>>> + pthread_mutex_lock(&bucket->lock);
>>> + if (bucket->waiters > 0)
>>> + pthread_cond_broadcast(&bucket->wait);
>>> + pthread_mutex_unlock(&bucket->lock);
>>> +}
>>
>> OK for me, but it deserves a git message and/or few comments. It took me some time to understand what you wanted to do.
>>
>>> --
>>> 2.25.0vlc-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é.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200217/985d6df6/attachment.html>
More information about the vlc-devel
mailing list