[vlc-devel] [PATCH 1/7] posix: add fallback vlc_atomic_* implementation
Alexandre Janniaux
ajanni at videolabs.io
Mon Feb 17 14:58:07 CET 2020
Hi,
On Mon, Feb 17, 2020 at 03:35:40PM +0200, 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.
>
You could probably describe the intent (in which case is it need
and why?) and give a general guideline to help review.
What you add is always supposed to be written in the diff section
of the patch itself or the naming is bad. Here it is correct but
I feel the intent and guidelines could be helpful.
Regards,
--
Alexandre Janniaux
Videolabs
> 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.0
> >>
> >> _______________________________________________
> >> 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é.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list