[vlc-devel] [PATCH 1/7] posix: add fallback vlc_atomic_* implementation

Thomas Guillem thomas at gllm.fr
Mon Feb 17 15:08:53 CET 2020


Indeed, this patch is self explanatory, you don't need to explain why. Still, I would have added some comments for the bucket of cond/lock.

But you don't explain why you do patch 2 and 3 (you only say what you do).


>From my review, I understand you want to add a timed_wait, that will help you remove some more cancel usage.
If it's the reason, I'm OK with it, but I think you should add it in commit messages.

On Mon, Feb 17, 2020, at 15:00, Rémi Denis-Courmont wrote:
> The description already says it's a fallback implementation for POSIX. There's nothing else to this patch than that and the existing API documentation.
> 
> Le 17 février 2020 15:47:52 GMT+02:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>> 
>> 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
>> 
> 
> -- 
> 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/f65b0bef/attachment.html>


More information about the vlc-devel mailing list