[FFmpeg-devel] avutil: fix data race in av_get_cpu_flags().

Message ID	1479771469-60379-2-git-send-email-wtc@google.com
State	Superseded
Headers	show Delivered-To: ffmpegpatchwork@gmail.com Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; From: Wan-Teh Chang <wtc-at-google.com@ffmpeg.org> To: ffmpeg-devel@ffmpeg.org Date: Mon, 21 Nov 2016 15:37:49 -0800 Message-Id: <1479771469-60379-2-git-send-email-wtc@google.com> In-Reply-To: <1479771469-60379-1-git-send-email-wtc@google.com> References: <1479771469-60379-1-git-send-email-wtc@google.com> Subject: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags(). Precedence: list Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>

diff --git a/libavutil/atomic.c b/libavutil/atomic.c index 64cff25..7bce013 100644 --- a/libavutil/atomic.c +++ b/libavutil/atomic.c @@ -40,6 +40,11 @@ int avpriv_atomic_int_get(volatile int *ptr) return res; } +int avpriv_atomic_int_get_relaxed(volatile int *ptr) +{ + return avpriv_atomic_int_get(ptr); +} + void avpriv_atomic_int_set(volatile int *ptr, int val) { pthread_mutex_lock(&atomic_lock); @@ -47,6 +52,11 @@ void avpriv_atomic_int_set(volatile int *ptr, int val) pthread_mutex_unlock(&atomic_lock); } +void avpriv_atomic_int_set_relaxed(volatile int *ptr, int val) +{ + avpriv_atomic_int_set(ptr, val); +} + int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc) { int res; @@ -59,6 +69,17 @@ int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc) return res; } +int avpriv_atomic_int_cas_relaxed(volatile int *ptr, int oldval, int newval) +{ + int ret; + pthread_mutex_lock(&atomic_lock); + ret = *ptr; + if (ret == oldval) + *ptr = newval; + pthread_mutex_unlock(&atomic_lock); + return ret; +} + void *avpriv_atomic_ptr_cas(void * volatile *ptr, void *oldval, void *newval) { void *ret; @@ -77,17 +98,36 @@ int avpriv_atomic_int_get(volatile int *ptr) return *ptr; } +int avpriv_atomic_int_get_relaxed(volatile int *ptr) +{ + return avpriv_atomic_int_get(ptr); +} + void avpriv_atomic_int_set(volatile int *ptr, int val) { *ptr = val; } +void avpriv_atomic_int_set_relaxed(volatile int *ptr, int val) +{ + avpriv_atomic_int_set(ptr, val); +} + int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc) { *ptr += inc; return *ptr; } +int avpriv_atomic_int_cas_relaxed(volatile int *ptr, int oldval, int newval) +{ + if (*ptr == oldval) { + *ptr = newval; + return oldval; + } + return *ptr; +} + void *avpriv_atomic_ptr_cas(void * volatile *ptr, void *oldval, void *newval) { if (*ptr == oldval) { diff --git a/libavutil/atomic.h b/libavutil/atomic.h index 15906d2..c5aa1a4 100644 --- a/libavutil/atomic.h +++ b/libavutil/atomic.h @@ -40,20 +40,38 @@ * * @param ptr atomic integer * @return the current value of the atomic integer - * @note This acts as a memory barrier. + * @note This acts as a memory barrier with sequentially-consistent ordering. */ int avpriv_atomic_int_get(volatile int *ptr); /** + * Load the current value stored in an atomic integer. + * + * @param ptr atomic integer + * @return the current value of the atomic integer + * @note This does NOT act as a memory barrier. + */ +int avpriv_atomic_int_get_relaxed(volatile int *ptr); + +/** * Store a new value in an atomic integer. * * @param ptr atomic integer * @param val the value to store in the atomic integer - * @note This acts as a memory barrier. + * @note This acts as a memory barrier with sequentially-consistent ordering. */ void avpriv_atomic_int_set(volatile int *ptr, int val); /** + * Store a new value in an atomic integer. + * + * @param ptr atomic integer + * @param val the value to store in the atomic integer + * @note This does NOT act as a memory barrier. + */ +void avpriv_atomic_int_set_relaxed(volatile int *ptr, int val); + +/** * Add a value to an atomic integer. * * @param ptr atomic integer @@ -65,12 +83,24 @@ void avpriv_atomic_int_set(volatile int *ptr, int val); int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc); /** + * Atomic integer compare and swap. + * + * @param ptr pointer to the integer to operate on + * @param oldval do the swap if the current value of *ptr equals to oldval + * @param newval value to replace *ptr with + * @return the value of *ptr before comparison + * @note This does NOT act as a memory barrier. + */ +int avpriv_atomic_int_cas_relaxed(volatile int *ptr, int oldval, int newval); + +/** * Atomic pointer compare and swap. * * @param ptr pointer to the pointer to operate on * @param oldval do the swap if the current value of *ptr equals to oldval * @param newval value to replace *ptr with * @return the value of *ptr before comparison + * @note This acts as a memory barrier with sequentially-consistent ordering. */ void *avpriv_atomic_ptr_cas(void * volatile *ptr, void *oldval, void *newval); diff --git a/libavutil/atomic_gcc.h b/libavutil/atomic_gcc.h index 5f9fc49..15dd508 100644 --- a/libavutil/atomic_gcc.h +++ b/libavutil/atomic_gcc.h @@ -36,6 +36,16 @@ static inline int atomic_int_get_gcc(volatile int *ptr) #endif } +#define avpriv_atomic_int_get_relaxed atomic_int_get_relaxed_gcc +static inline int atomic_int_get_relaxed_gcc(volatile int *ptr) +{ +#if HAVE_ATOMIC_COMPARE_EXCHANGE + return __atomic_load_n(ptr, __ATOMIC_RELAXED); +#else + return *ptr; +#endif +} + #define avpriv_atomic_int_set atomic_int_set_gcc static inline void atomic_int_set_gcc(volatile int *ptr, int val) { @@ -47,6 +57,16 @@ static inline void atomic_int_set_gcc(volatile int *ptr, int val) #endif } +#define avpriv_atomic_int_set_relaxed atomic_int_set_relaxed_gcc +static inline void atomic_int_set_relaxed_gcc(volatile int *ptr, int val) +{ +#if HAVE_ATOMIC_COMPARE_EXCHANGE + __atomic_store_n(ptr, val, __ATOMIC_RELAXED); +#else + *ptr = val; +#endif +} + #define avpriv_atomic_int_add_and_fetch atomic_int_add_and_fetch_gcc static inline int atomic_int_add_and_fetch_gcc(volatile int *ptr, int inc) { @@ -57,6 +77,19 @@ static inline int atomic_int_add_and_fetch_gcc(volatile int *ptr, int inc) #endif } +#define avpriv_atomic_int_cas_relaxed atomic_int_cas_relaxed_gcc +static inline int atomic_int_cas_relaxed_gcc(volatile int *ptr, + int oldval, int newval) +{ +#if HAVE_SYNC_VAL_COMPARE_AND_SWAP + return __sync_val_compare_and_swap(ptr, oldval, newval); +#else + __atomic_compare_exchange_n(ptr, &oldval, newval, 0, __ATOMIC_RELAXED, + __ATOMIC_RELAXED); + return oldval; +#endif +} + #define avpriv_atomic_ptr_cas atomic_ptr_cas_gcc static inline void *atomic_ptr_cas_gcc(void * volatile *ptr, void *oldval, void *newval) diff --git a/libavutil/atomic_suncc.h b/libavutil/atomic_suncc.h index a75a37b..0997f23 100644 --- a/libavutil/atomic_suncc.h +++ b/libavutil/atomic_suncc.h @@ -31,6 +31,12 @@ static inline int atomic_int_get_suncc(volatile int *ptr) return *ptr; } +#define avpriv_atomic_int_get_relaxed atomic_int_get_relaxed_suncc +static inline int atomic_int_get_relaxed_suncc(volatile int *ptr) +{ + return *ptr; +} + #define avpriv_atomic_int_set atomic_int_set_suncc static inline void atomic_int_set_suncc(volatile int *ptr, int val) { @@ -38,12 +44,25 @@ static inline void atomic_int_set_suncc(volatile int *ptr, int val) __machine_rw_barrier(); } +#define avpriv_atomic_int_set_relaxed atomic_int_set_relaxed_suncc +static inline void atomic_int_set_relaxed_suncc(volatile int *ptr, int val) +{ + *ptr = val; +} + #define avpriv_atomic_int_add_and_fetch atomic_int_add_and_fetch_suncc static inline int atomic_int_add_and_fetch_suncc(volatile int *ptr, int inc) { return atomic_add_int_nv(ptr, inc); } +#define avpriv_atomic_int_cas_relaxed atomic_int_cas_relaxed_suncc +static inline int atomic_int_cas_relaxed_suncc(volatile int *ptr, + int oldval, int newval) +{ + return atomic_cas_uint((volatile uint_t *)ptr, oldval, newval); +} + #define avpriv_atomic_ptr_cas atomic_ptr_cas_suncc static inline void *atomic_ptr_cas_suncc(void * volatile *ptr, void *oldval, void *newval) diff --git a/libavutil/atomic_win32.h b/libavutil/atomic_win32.h index f729933..7ea0a56 100644 --- a/libavutil/atomic_win32.h +++ b/libavutil/atomic_win32.h @@ -31,6 +31,12 @@ static inline int atomic_int_get_win32(volatile int *ptr) return *ptr; } +#define avpriv_atomic_int_get_relaxed atomic_int_get_relaxed_win32 +static inline int atomic_int_get_relaxed_win32(volatile int *ptr) +{ + return *ptr; +} + #define avpriv_atomic_int_set atomic_int_set_win32 static inline void atomic_int_set_win32(volatile int *ptr, int val) { @@ -38,12 +44,27 @@ static inline void atomic_int_set_win32(volatile int *ptr, int val) MemoryBarrier(); } +#define avpriv_atomic_int_set_relaxed atomic_int_set_relaxed_win32 +static inline void atomic_int_set_relaxed_win32(volatile int *ptr, int val) +{ + *ptr = val; +} + #define avpriv_atomic_int_add_and_fetch atomic_int_add_and_fetch_win32 static inline int atomic_int_add_and_fetch_win32(volatile int *ptr, int inc) { return inc + InterlockedExchangeAdd(ptr, inc); } +#define avpriv_atomic_int_cas_relaxed atomic_int_cas_relaxed_win32 +static inline int atomic_int_cas_relaxed_win32(volatile int *ptr, + int oldval, int newval) +{ + /* InterlockedCompareExchangeNoFence is available on Windows 8 or later + * only. */ + return InterlockedCompareExchange((volatile LONG *)ptr, newval, oldval); +} + #define avpriv_atomic_ptr_cas atomic_ptr_cas_win32 static inline void *atomic_ptr_cas_win32(void * volatile *ptr, void *oldval, void *newval) diff --git a/libavutil/cpu.c b/libavutil/cpu.c index f5785fc..cbcca4c 100644 --- a/libavutil/cpu.c +++ b/libavutil/cpu.c @@ -23,6 +23,7 @@ #include "config.h" #include "opt.h" #include "common.h" +#include "atomic.h" #if HAVE_SCHED_GETAFFINITY #ifndef _GNU_SOURCE @@ -44,7 +45,20 @@ #include <unistd.h> #endif -static int flags, checked; +static int cpu_flags = -1; + +static int get_cpu_flags(void) +{ + if (ARCH_AARCH64) + return ff_get_cpu_flags_aarch64(); + if (ARCH_ARM) + return ff_get_cpu_flags_arm(); + if (ARCH_PPC) + return ff_get_cpu_flags_ppc(); + if (ARCH_X86) + return ff_get_cpu_flags_x86(); + /* Not reached. */ +} void av_force_cpu_flags(int arg){ if ( (arg & ( AV_CPU_FLAG_3DNOW | @@ -69,33 +83,22 @@ void av_force_cpu_flags(int arg){ arg |= AV_CPU_FLAG_MMX; } - flags = arg; - checked = arg != -1; + avpriv_atomic_int_set_relaxed(&cpu_flags, arg); } int av_get_cpu_flags(void) { - if (checked) - return flags; - - if (ARCH_AARCH64) - flags = ff_get_cpu_flags_aarch64(); - if (ARCH_ARM) - flags = ff_get_cpu_flags_arm(); - if (ARCH_PPC) - flags = ff_get_cpu_flags_ppc(); - if (ARCH_X86) - flags = ff_get_cpu_flags_x86(); - - checked = 1; + int flags = avpriv_atomic_int_get_relaxed(&cpu_flags); + if (flags == -1) { + flags = get_cpu_flags(); + avpriv_atomic_int_cas_relaxed(&cpu_flags, -1, flags); + } return flags; } void av_set_cpu_flags_mask(int mask) { - checked = 0; - flags = av_get_cpu_flags() & mask; - checked = 1; + avpriv_atomic_int_set_relaxed(&cpu_flags, get_cpu_flags() & mask); } int av_parse_cpu_flags(const char *s) diff --git a/libavutil/tests/atomic.c b/libavutil/tests/atomic.c index c92f220..5a12439 100644 --- a/libavutil/tests/atomic.c +++ b/libavutil/tests/atomic.c @@ -26,9 +26,22 @@ int main(void) res = avpriv_atomic_int_add_and_fetch(&val, 1); av_assert0(res == 2); + res = avpriv_atomic_int_add_and_fetch(&val, -1); + av_assert0(res == 1); avpriv_atomic_int_set(&val, 3); res = avpriv_atomic_int_get(&val); av_assert0(res == 3); + avpriv_atomic_int_set_relaxed(&val, 4); + res = avpriv_atomic_int_get_relaxed(&val); + av_assert0(res == 4); + res = avpriv_atomic_int_cas_relaxed(&val, 3, 5); + av_assert0(res == 4); + res = avpriv_atomic_int_get_relaxed(&val); + av_assert0(res == 4); + res = avpriv_atomic_int_cas_relaxed(&val, 4, 5); + av_assert0(res == 4); + res = avpriv_atomic_int_get_relaxed(&val); + av_assert0(res == 5); return 0; }

[FFmpeg-devel] avutil: fix data race in av_get_cpu_flags().

Commit Message

Comments

Patch