Anton Khirnov:
> Memory ordering constraints other than the default (sequentially
> consistent) can behave in very unintuitive and unexpected ways, and so
> should be avoided unless there is a strong (typically performance)
> reason for using them. This holds especially for memory_order_relaxed,
> which imposes no ordering constraints.
I disagree with this: Adding ordering constraints is wrong, as it can
provide synchronization between different threads and this can mask
races. Providing synchronization as a byproduct should be avoided: Users
should know what they get. And if a user wants an allocation limit in
thread A to be in effect for allocations in thread B, then the user
should provide the synchronization him/herself.
Here is an example of a program that contains a data race that is masked
by this patch: If all allocations happen before av_max_alloc(3), then
tsan will complain about a data race; otherwise, it is silent, as the
write in av_max_alloc() synchronizes with the read in av_malloc(), so
that race = 1 is visible in the main thread later.
#include <pthread.h>
#include <stdio.h>
#include "libavutil/mem.h"
int race;
static void *second_thread(void *arg)
{
race = 1;
av_max_alloc(3);
return NULL;
}
int main()
{
pthread_t thread;
pthread_create(&thread, NULL, second_thread, NULL);
for (int i = 0; i < 150; i++)
av_malloc(i);
printf("%d\n", race);
pthread_join(thread, NULL);
}
- Andreas
> ---
> libavformat/fifo.c | 8 ++++----
> libavutil/cpu.c | 8 ++++----
> libavutil/mem.c | 10 +++++-----
> 3 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/libavformat/fifo.c b/libavformat/fifo.c
> index 50656f78b7..2ac14207b8 100644
> --- a/libavformat/fifo.c
> +++ b/libavformat/fifo.c
> @@ -185,7 +185,7 @@ static int fifo_thread_write_packet(FifoThreadContext
> *ctx, AVPacket *pkt)
> int ret, s_idx;
>
> if (fifo->timeshift && pkt->dts != AV_NOPTS_VALUE)
> - atomic_fetch_sub_explicit(&fifo->queue_duration, next_duration(avf,
> pkt, &ctx->last_received_dts), memory_order_relaxed);
> + atomic_fetch_sub(&fifo->queue_duration, next_duration(avf, pkt,
> &ctx->last_received_dts));
>
> if (ctx->drop_until_keyframe) {
> if (pkt->flags & AV_PKT_FLAG_KEY) {
> @@ -454,7 +454,7 @@ static void *fifo_consumer_thread(void *data)
> av_log(avf, AV_LOG_INFO, "FIFO queue flushed\n");
>
> if (fifo->timeshift)
> - while (atomic_load_explicit(&fifo->queue_duration,
> memory_order_relaxed) < fifo->timeshift)
> + while (atomic_load(&fifo->queue_duration) < fifo->timeshift)
> av_usleep(10000);
>
> ret = av_thread_message_queue_recv(queue, &msg, 0);
> @@ -594,7 +594,7 @@ static int fifo_write_packet(AVFormatContext *avf,
> AVPacket *pkt)
> }
>
> if (fifo->timeshift && pkt && pkt->dts != AV_NOPTS_VALUE)
> - atomic_fetch_add_explicit(&fifo->queue_duration, next_duration(avf,
> pkt, &fifo->last_sent_dts), memory_order_relaxed);
> + atomic_fetch_add(&fifo->queue_duration, next_duration(avf, pkt,
> &fifo->last_sent_dts));
>
> return ret;
> fail:
> @@ -621,7 +621,7 @@ static int fifo_write_trailer(AVFormatContext *avf)
> } else {
> now += delay;
> }
> - atomic_fetch_add_explicit(&fifo->queue_duration, delay,
> memory_order_relaxed);
> + atomic_fetch_add(&fifo->queue_duration, delay);
> elapsed += delay;
> if (elapsed > fifo->timeshift)
> break;
> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> index 8960415d00..8a6af81ae4 100644
> --- a/libavutil/cpu.c
> +++ b/libavutil/cpu.c
> @@ -89,15 +89,15 @@ void av_force_cpu_flags(int arg){
> arg |= AV_CPU_FLAG_MMX;
> }
>
> - atomic_store_explicit(&cpu_flags, arg, memory_order_relaxed);
> + atomic_store(&cpu_flags, arg);
> }
>
> int av_get_cpu_flags(void)
> {
> - int flags = atomic_load_explicit(&cpu_flags, memory_order_relaxed);
> + int flags = atomic_load(&cpu_flags);
> if (flags == -1) {
> flags = get_cpu_flags();
> - atomic_store_explicit(&cpu_flags, flags, memory_order_relaxed);
> + atomic_store(&cpu_flags, flags);
> }
> return flags;
> }
> @@ -221,7 +221,7 @@ int av_cpu_count(void)
> nb_cpus = sysinfo.dwNumberOfProcessors;
> #endif
>
> - if (!atomic_exchange_explicit(&printed, 1, memory_order_relaxed))
> + if (!atomic_exchange(&printed, 1))
> av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
>
> return nb_cpus;
> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index a52d33d4a6..c216c0314f 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -72,14 +72,14 @@ void free(void *ptr);
> static atomic_size_t max_alloc_size = ATOMIC_VAR_INIT(INT_MAX);
>
> void av_max_alloc(size_t max){
> - atomic_store_explicit(&max_alloc_size, max, memory_order_relaxed);
> + atomic_store(&max_alloc_size, max);
> }
>
> void *av_malloc(size_t size)
> {
> void *ptr = NULL;
>
> - if (size > atomic_load_explicit(&max_alloc_size, memory_order_relaxed))
> + if (size > atomic_load(&max_alloc_size))
> return NULL;
>
> #if HAVE_POSIX_MEMALIGN
> @@ -135,7 +135,7 @@ void *av_malloc(size_t size)
> void *av_realloc(void *ptr, size_t size)
> {
> void *ret;
> - if (size > atomic_load_explicit(&max_alloc_size, memory_order_relaxed))
> + if (size > atomic_load(&max_alloc_size))
> return NULL;
>
> #if HAVE_ALIGNED_MALLOC
> @@ -489,7 +489,7 @@ void *av_fast_realloc(void *ptr, unsigned int *size,
> size_t min_size)
> if (min_size <= *size)
> return ptr;
>
> - max_size = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
> + max_size = atomic_load(&max_alloc_size);
>
> if (min_size > max_size) {
> *size = 0;
> @@ -521,7 +521,7 @@ static inline void fast_malloc(void *ptr, unsigned int
> *size, size_t min_size, i
> return;
> }
>
> - max_size = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
> + max_size = atomic_load(&max_alloc_size);
>
> if (min_size > max_size) {
> av_freep(ptr);
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
