[FFmpeg-devel] [PATCH] Stop using _explicit atomic operations where not necessary.

[James Almer]

On 6/5/2021 11:27 AM, Anton Khirnov wrote:
> 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.

Performance is important for FIFO, though. Could they use a laxer order 
at least for the fetch_sub and fetch_add cases?

Also, commit fed50c4304e, which made cpu_flags an atomic type, states 
that ThreadSanitizer stopped reporting races on tests/cpu_init after 
that change, so maybe memory order is not important for it, only atomicity.
In any case these cpu and mem functions are hardly a bottleneck 
anywhere, and rarely called outside of debugging, so this change is ok.

> ---
>   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);
>