[FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
Wan-Teh Chang
wtc at google.com
Tue Nov 22 01:37:49 EET 2016
Make the one-time initialization in av_get_cpu_flags() thread-safe.
The fix assumes -1 is an invalid value for cpu flags.
The fix requires new atomic functions to get, set, and compare-and-swap
an integer without a memory barrier.
The data race fix is written by Dmitry Vyukov of Google.
Signed-off-by: Wan-Teh Chang <wtc at google.com>
---
libavutil/atomic.c | 40 ++++++++++++++++++++++++++++++++++++++++
libavutil/atomic.h | 34 ++++++++++++++++++++++++++++++++--
libavutil/atomic_gcc.h | 33 +++++++++++++++++++++++++++++++++
libavutil/atomic_suncc.h | 19 +++++++++++++++++++
libavutil/atomic_win32.h | 21 +++++++++++++++++++++
libavutil/cpu.c | 41 ++++++++++++++++++++++-------------------
libavutil/tests/atomic.c | 13 +++++++++++++
7 files changed, 180 insertions(+), 21 deletions(-)
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;
}
--
2.8.0.rc3.226.g39d4020
More information about the ffmpeg-devel
mailing list