Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags()
On Tue, Dec 06, 2016 at 06:16:13PM -0800, Wan-Teh Chang wrote: > Make the one-time initialization in av_get_cpu_flags() thread-safe. The > static variable |cpu_flags| in libavutil/cpu.c is read and written using > normal load and store operations. These are considered as data races. > The fix is to use atomic load and store operations. > > The fix can be verified by running the libavutil/tests/cpu_init.c test > program under ThreadSanitizer: > ./configure --toolchain=clang-tsan > make libavutil/tests/cpu_init > libavutil/tests/cpu_init > > There should be no warnings from ThreadSanitizer. > > Co-author: Dmitry Vyukov of Google, who suggested the data race fix. > > Signed-off-by: Wan-Teh Chang > --- > libavutil/cpu.c | 12 +++- > libavutil/cpu.h | 2 -- > 2 files changed, 7 insertions(+), 7 deletions(-) applied thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Into a blind darkness they enter who follow after the Ignorance, they as if into a greater darkness enter who devote themselves to the Knowledge alone. -- Isha Upanishad signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags()
On Tue, Dec 6, 2016 at 10:41 AM, Wan-Teh Chang wrote:
> Make the one-time initialization in av_get_cpu_flags() thread-safe. The
> static variable |cpu_flags| in libavutil/cpu.c is read and written using
> normal load and store operations. These are considered as data races.
> The fix is to use atomic load and store operations.
>
> The fix can be verified by running the libavutil/tests/cpu_init.c test
> program under ThreadSanitizer:
> ./configure --toolchain=clang-tsan
> make libavutil/tests/cpu_init
> libavutil/tests/cpu_init
>
> There should be no warnings from ThreadSanitizer.
>
> Co-author: Dmitry Vyukov of Google, which suggested the data race fix.
Please ignore this patch. I just noticed that the last line of the
commit message has a grammatical error ("which" should be changed to
"who").
I will correct that and send a new patch.
Wan-Teh Chang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Thu, Nov 24, 2016 at 1:56 AM, wm4 wrote: > On Wed, 23 Nov 2016 11:40:25 -0800 > Wan-Teh Chang wrote: > >> On Tue, Nov 22, 2016 at 3:30 PM, wm4 wrote: >> > On Tue, 22 Nov 2016 23:57:15 +0100 >> > Michael Niedermayer wrote: >> > >> >> For example the progress code in the frame threading. >> > >> > Which was recently fixed in Libav AFAIR... >> >> You're right. libav/libavcodec/pthread_frame.c has code similar to my >> ffmpeg patch http://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/190454.html, >> and much more. >> >> Note: libav/libavcodec/pthread_frame.c uses unnecessary (too strong) >> memory barriers in ff_thread_report_progress(). We can fix those when >> the code is merged to ffmpeg. > > You could also just send a patch to them. Done. I sent a patch to libav-devel last week: https://lists.libav.org/pipermail/libav-devel/2016-November/080903.html Wan-Teh Chang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
Hi. Mozilla also fixed some data races in pthread_frames.c which you may want to look at. It's very similar to problem you mentioned We made the code conditional on being in an TSan build. https://dxr.mozilla.org/mozilla-central/source/media/ffvpx/libavcodec/pthread_frame.c#46 Sorry, using the new gmail app, not allowing me to reply inline. I can only top post. Jean-Yves Le jeu. 24 nov. 2016 à 06:47, Wan-Teh Chang a écrit : > On Tue, Nov 22, 2016 at 3:30 PM, wm4 wrote: > > On Tue, 22 Nov 2016 23:57:15 +0100 > > Michael Niedermayer wrote: > > > >> For example the progress code in the frame threading. > > > > Which was recently fixed in Libav AFAIR... > > You're right. libav/libavcodec/pthread_frame.c has code similar to my > ffmpeg patch > http://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/190454.html, > and much more. > > Note: libav/libavcodec/pthread_frame.c uses unnecessary (too strong) > memory barriers in ff_thread_report_progress(). We can fix those when > the code is merged to ffmpeg. > > Wan-Teh Chang > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Wed, 23 Nov 2016 11:44:04 -0800 Wan-Teh Chang wrote: > On Tue, Nov 22, 2016 at 3:32 PM, wm4 wrote: > > > > [libav/compat/atomics/] is emulation code for compilers which don't provide > > C11 atomics. > > All relevant compilers provide them natively (and presumably implement > > the weaker semantics). > > Thank you for pointing this out. > > Do you know when the atomics code in libav will be merged to ffmpeg? > > I don't want add new code that will interfere with the merge. On the > other hand, the new code I wanted to add will be easy to rewrite using > C11 atomics. So this all depends on when the merge will be performed. Merges are about 600 commits and 4 months behind, so cherry-picking those Libav changes as michaelni suggested might be a quicker way to get them in. It's a quite painful situation. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Wed, 23 Nov 2016 11:40:25 -0800 Wan-Teh Chang wrote: > On Tue, Nov 22, 2016 at 3:30 PM, wm4 wrote: > > On Tue, 22 Nov 2016 23:57:15 +0100 > > Michael Niedermayer wrote: > > > >> For example the progress code in the frame threading. > > > > Which was recently fixed in Libav AFAIR... > > You're right. libav/libavcodec/pthread_frame.c has code similar to my > ffmpeg patch http://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/190454.html, > and much more. > > Note: libav/libavcodec/pthread_frame.c uses unnecessary (too strong) > memory barriers in ff_thread_report_progress(). We can fix those when > the code is merged to ffmpeg. You could also just send a patch to them. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Wed, Nov 23, 2016 at 11:44:04AM -0800, Wan-Teh Chang wrote: > On Tue, Nov 22, 2016 at 3:32 PM, wm4 wrote: > > > > [libav/compat/atomics/] is emulation code for compilers which don't provide > > C11 atomics. > > All relevant compilers provide them natively (and presumably implement > > the weaker semantics). > > Thank you for pointing this out. > > Do you know when the atomics code in libav will be merged to ffmpeg? You can just cherry pick what you need and submit it as patchset to ffmpeg-devel make sure though all author fields are 100% correct > > I don't want add new code that will interfere with the merge. On the > other hand, the new code I wanted to add will be easy to rewrite using > C11 atomics. So this all depends on when the merge will be performed. > > Thanks, > Wan-Teh Chang > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Tue, Nov 22, 2016 at 3:30 PM, wm4 wrote: > On Tue, 22 Nov 2016 23:57:15 +0100 > Michael Niedermayer wrote: > >> For example the progress code in the frame threading. > > Which was recently fixed in Libav AFAIR... You're right. libav/libavcodec/pthread_frame.c has code similar to my ffmpeg patch http://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/190454.html, and much more. Note: libav/libavcodec/pthread_frame.c uses unnecessary (too strong) memory barriers in ff_thread_report_progress(). We can fix those when the code is merged to ffmpeg. Wan-Teh Chang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Tue, Nov 22, 2016 at 3:32 PM, wm4 wrote: > > [libav/compat/atomics/] is emulation code for compilers which don't provide > C11 atomics. > All relevant compilers provide them natively (and presumably implement > the weaker semantics). Thank you for pointing this out. Do you know when the atomics code in libav will be merged to ffmpeg? I don't want add new code that will interfere with the merge. On the other hand, the new code I wanted to add will be easy to rewrite using C11 atomics. So this all depends on when the merge will be performed. Thanks, Wan-Teh Chang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Tue, 22 Nov 2016 15:05:36 -0800
Wan-Teh Chang wrote:
> Hi wm4,
>
> On Tue, Nov 22, 2016 at 1:41 PM, wm4 wrote:
> >
> > Again, once the atomics changes in Libav are merged, the avpriv_atomic_
> > additions will have to be deleted, and code using it rewritten.
>
> Thanks for the heads-up. Is there an atomics patch for libav being
> reviewed right now?
>
> I inspected the libav code this morning after I read your earlier
> message. The current atomics code in libav ignores the memory order
> argument, so the "relaxed" memory order isn't implemented in the
> current libav.
>
> For example, the current revision of libav/compat/atomics/gcc/stdatomic.h has:
>
> ==
> #define atomic_store(object, desired) \
> do {\
> *(object) = (desired); \
> __sync_synchronize(); \
> } while (0)
>
> #define atomic_store_explicit(object, desired, order) \
> atomic_store(object, desired)
>
> #define atomic_load(object) \
> (__sync_synchronize(), *(object))
>
> #define atomic_load_explicit(object, order) \
> atomic_load(object)
> ==
>
> So I am wondering if there is a libav patch that implements the
> |order| argument for atomic_store_explicit() and
> atomic_load_explicit().
>
This is emulation code for compilers which don't provide C11 atomics.
All relevant compilers provide them natively (and presumably implement
the weaker semantics). Except MSVC I guess, but it's well known that
libavcodec runs slower when compiled with MSVC anyway.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Tue, 22 Nov 2016 23:57:15 +0100 Michael Niedermayer wrote: > On Tue, Nov 22, 2016 at 02:00:12PM -0800, Wan-Teh Chang wrote: > > Hi Michael, > > > > On Tue, Nov 22, 2016 at 1:22 PM, Michael Niedermayer > > wrote: > > > > > > ok, i see th race but do you really need the atomic operations ? > > > isnt merging the 2 variables into 1 as you do enough ? > > > (i mean the tools might still complain but would there be an actual > > > race remaining?) > > > > The atomic operations avoid the "undefined behavior" resulting from > > the data races in the C source code. ThreadSanitizer analyzes the C > > source code, therefore it must warn about what may be undefined > > behavior according to the C standard, even though for a particular > > compiler and processor, the generated code is the same. > > > > Here is a small test program that shows gcc generates the same x86_64 > > assembly code for the normal and atomic (with relaxed memory ordering) > > load and store operations: > > we have plenty of code that accesses fields without the extra layer > of weak atomics. > For example the progress code in the frame threading. Which was recently fixed in Libav AFAIR... > > Can you just merge the varible ? > > this also would be easier to backport if anyone wants to backport > > If you still want to add weak atomics as in the patch, afterwards on > top iam not against but that is me, i wont apply it if theres no > consensus or clear majority in favour though > > [...] > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
Hi Michael, On Tue, Nov 22, 2016 at 2:57 PM, Michael Niedermayer wrote: > > we have plenty of code that accesses fields without the extra layer > of weak atomics. > For example the progress code in the frame threading. The progress code in the frame threading also has data races as defined by the C standard, so ThreadSanitizer also warns about it. I submitted patches for that code to use atomic operations before. > Can you just merge the varible ? Yes, I will create a patch to just merge the variables. My goal is to fix the data races, so I still need to create another patch on top to use atomic operations. Wan-Teh Chang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
Hi wm4,
On Tue, Nov 22, 2016 at 1:41 PM, wm4 wrote:
>
> Again, once the atomics changes in Libav are merged, the avpriv_atomic_
> additions will have to be deleted, and code using it rewritten.
Thanks for the heads-up. Is there an atomics patch for libav being
reviewed right now?
I inspected the libav code this morning after I read your earlier
message. The current atomics code in libav ignores the memory order
argument, so the "relaxed" memory order isn't implemented in the
current libav.
For example, the current revision of libav/compat/atomics/gcc/stdatomic.h has:
==
#define atomic_store(object, desired) \
do {\
*(object) = (desired); \
__sync_synchronize(); \
} while (0)
#define atomic_store_explicit(object, desired, order) \
atomic_store(object, desired)
#define atomic_load(object) \
(__sync_synchronize(), *(object))
#define atomic_load_explicit(object, order) \
atomic_load(object)
==
So I am wondering if there is a libav patch that implements the
|order| argument for atomic_store_explicit() and
atomic_load_explicit().
Thanks,
Wan-Teh Chang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Tue, Nov 22, 2016 at 02:00:12PM -0800, Wan-Teh Chang wrote: > Hi Michael, > > On Tue, Nov 22, 2016 at 1:22 PM, Michael Niedermayer > wrote: > > > > ok, i see th race but do you really need the atomic operations ? > > isnt merging the 2 variables into 1 as you do enough ? > > (i mean the tools might still complain but would there be an actual > > race remaining?) > > The atomic operations avoid the "undefined behavior" resulting from > the data races in the C source code. ThreadSanitizer analyzes the C > source code, therefore it must warn about what may be undefined > behavior according to the C standard, even though for a particular > compiler and processor, the generated code is the same. > > Here is a small test program that shows gcc generates the same x86_64 > assembly code for the normal and atomic (with relaxed memory ordering) > load and store operations: we have plenty of code that accesses fields without the extra layer of weak atomics. For example the progress code in the frame threading. Can you just merge the varible ? this also would be easier to backport if anyone wants to backport If you still want to add weak atomics as in the patch, afterwards on top iam not against but that is me, i wont apply it if theres no consensus or clear majority in favour though [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
Hi Michael,
On Tue, Nov 22, 2016 at 1:22 PM, Michael Niedermayer
wrote:
>
> ok, i see th race but do you really need the atomic operations ?
> isnt merging the 2 variables into 1 as you do enough ?
> (i mean the tools might still complain but would there be an actual
> race remaining?)
The atomic operations avoid the "undefined behavior" resulting from
the data races in the C source code. ThreadSanitizer analyzes the C
source code, therefore it must warn about what may be undefined
behavior according to the C standard, even though for a particular
compiler and processor, the generated code is the same.
Here is a small test program that shows gcc generates the same x86_64
assembly code for the normal and atomic (with relaxed memory ordering)
load and store operations:
==
$ gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ cat flags.c
static int flags;
int get_flags_nonatomic(void)
{
return flags;
}
int get_flags_atomic_relaxed(void)
{
return __atomic_load_n(&flags, __ATOMIC_RELAXED);
}
void set_flags_nonatomic(int val)
{
flags = val;
}
void set_flags_atomic_relaxed(int val)
{
__atomic_store_n(&flags, val, __ATOMIC_RELAXED);
}
$ gcc -Wall -O3 -std=c11 -S -c flags.c
$ cat flags.s
.file "flags.c"
.text
.p2align 4,,15
.globl get_flags_nonatomic
.type get_flags_nonatomic, @function
get_flags_nonatomic:
.LFB0:
.cfi_startproc
movl flags(%rip), %eax
ret
.cfi_endproc
.LFE0:
.size get_flags_nonatomic, .-get_flags_nonatomic
.p2align 4,,15
.globl get_flags_atomic_relaxed
.type get_flags_atomic_relaxed, @function
get_flags_atomic_relaxed:
.LFB1:
.cfi_startproc
movl flags(%rip), %eax
ret
.cfi_endproc
.LFE1:
.size get_flags_atomic_relaxed, .-get_flags_atomic_relaxed
.p2align 4,,15
.globl set_flags_nonatomic
.type set_flags_nonatomic, @function
set_flags_nonatomic:
.LFB2:
.cfi_startproc
movl %edi, flags(%rip)
ret
.cfi_endproc
.LFE2:
.size set_flags_nonatomic, .-set_flags_nonatomic
.p2align 4,,15
.globl set_flags_atomic_relaxed
.type set_flags_atomic_relaxed, @function
set_flags_atomic_relaxed:
.LFB3:
.cfi_startproc
movl %edi, flags(%rip)
ret
.cfi_endproc
.LFE3:
.size set_flags_atomic_relaxed, .-set_flags_atomic_relaxed
.local flags
.comm flags,4,16
.ident "GCC: (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4"
.section .note.GNU-stack,"",@progbits
==
Wan-Teh Chang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Tue, 22 Nov 2016 13:36:11 -0800
Wan-Teh Chang wrote:
> Make the one-time initialization in av_get_cpu_flags() thread-safe. The
> static variables |flags| and |checked| in libavutil/cpu.c are read and
> written using normal load and store operations. These are considered as
> data races. The fix is to use atomic load and store operations. The fix
> also eliminates the |checked| variable because the invalid value of -1
> for |flags| can be used to indicate the same condition.
>
> The fix requires a new atomic integer compare and swap function. Since
> the fix does not need memory barriers, "relaxed" variants of
> avpriv_atomic_int_get() and avpriv_atomic_int_set() are also added.
>
> Add a test program libavutil/tests/cpu_init.c to verify the one-time
> initialization in av_get_cpu_flags() is thread-safe.
>
> Co-author: Dmitry Vyukov of Google, which suggested the data race fix.
>
> Signed-off-by: Wan-Teh Chang
> ---
> libavutil/Makefile | 1 +
> 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/cpu.h| 2 --
> libavutil/tests/atomic.c | 13 +
> libavutil/tests/cpu_init.c | 72
> ++
> 10 files changed, 253 insertions(+), 23 deletions(-)
> create mode 100644 libavutil/tests/cpu_init.c
>
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 0fa90fe..732961a 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -187,6 +187,7 @@ TESTPROGS = adler32
> \
> camellia\
> color_utils \
> cpu \
> +cpu_init\
> crc \
> des \
> dict\
> 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
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Tue, Nov 22, 2016 at 11:28:48AM -0800, Wan-Teh Chang wrote: > Hi Michael, > > On Mon, Nov 21, 2016 at 4:35 PM, Michael Niedermayer > wrote: > > On Mon, Nov 21, 2016 at 03:37:49PM -0800, Wan-Teh Chang wrote: > >> Make the one-time initialization in av_get_cpu_flags() thread-safe. > >> The fix assumes -1 is an invalid value for cpu flags. > > > > please explain the bug / race that occurs and how it is fixed > > Note: I will include the following explanation in the next version of my > patch. > > The static variables |flags| and |checked| in libavutil/cpu.c are read > and written using normal load and store instructions. These are > considered as data races. The fix is to use atomic load and store > instructions. The fix also eliminates the |checked| variable because > the invalid value of -1 for |flags| can be used to indicate the same > condition. > > Our application runs into the data races because two threads call > avformat_find_stream_info() at the same time. > avformat_find_stream_info() calls av_parser_init(), which may > eventually call av_get_cpu_flag(), depending on the codec. > > I just wrote a minimal test case (libavutil/tests/cpu_init.c) that > reproduces the data races. The test program creates two threads that > call av_get_cpu_flags(). When built with --toolchain=clang-tsan, the > test program triggers two ThreadSanitizer warnings: ok, i see th race but do you really need the atomic operations ? isnt merging the 2 variables into 1 as you do enough ? (i mean the tools might still complain but would there be an actual race remaining?) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct answer. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
Hi Michael,
On Mon, Nov 21, 2016 at 4:35 PM, Michael Niedermayer
wrote:
> On Mon, Nov 21, 2016 at 03:37:49PM -0800, Wan-Teh Chang wrote:
>> Make the one-time initialization in av_get_cpu_flags() thread-safe.
>> The fix assumes -1 is an invalid value for cpu flags.
>
> please explain the bug / race that occurs and how it is fixed
Note: I will include the following explanation in the next version of my patch.
The static variables |flags| and |checked| in libavutil/cpu.c are read
and written using normal load and store instructions. These are
considered as data races. The fix is to use atomic load and store
instructions. The fix also eliminates the |checked| variable because
the invalid value of -1 for |flags| can be used to indicate the same
condition.
Our application runs into the data races because two threads call
avformat_find_stream_info() at the same time.
avformat_find_stream_info() calls av_parser_init(), which may
eventually call av_get_cpu_flag(), depending on the codec.
I just wrote a minimal test case (libavutil/tests/cpu_init.c) that
reproduces the data races. The test program creates two threads that
call av_get_cpu_flags(). When built with --toolchain=clang-tsan, the
test program triggers two ThreadSanitizer warnings:
$ libavutil/tests/cpu_init
==
WARNING: ThreadSanitizer: data race (pid=66608)
Read of size 4 at 0x7f7aa8c15d40 by thread T2:
#0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:78
(exe+0x000a8536)
#1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x000a8494)
Previous write of size 4 at 0x7f7aa8c15d40 by thread T1:
#0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:90
(exe+0x000a8578)
#1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x000a8494)
Thread T2 (tid=66611, running) created by main thread at:
#0 pthread_create ??:0 (exe+0x0004d9bb)
#1 main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:51
(exe+0x000a83cb)
Thread T1 (tid=66610, running) created by main thread at:
#0 pthread_create ??:0 (exe+0x0004d9bb)
#1 main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:47
(exe+0x000a83a9)
SUMMARY: ThreadSanitizer: data race
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:78
av_get_cpu_flags
==
==
WARNING: ThreadSanitizer: data race (pid=66608)
Read of size 4 at 0x7f7aa8c15d3c by thread T2:
#0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:79
(exe+0x000a854b)
#1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x000a8494)
Previous write of size 4 at 0x7f7aa8c15d3c by thread T1:
#0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:88
(exe+0x000a8566)
#1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x000a8494)
Thread T2 (tid=66611, running) created by main thread at:
#0 pthread_create ??:0 (exe+0x0004d9bb)
#1 main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:51
(exe+0x000a83cb)
Thread T1 (tid=66610, running) created by main thread at:
#0 pthread_create ??:0 (exe+0x0004d9bb)
#1 main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:47
(exe+0x000a83a9)
SUMMARY: ThreadSanitizer: data race
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:79
av_get_cpu_flags
==
ThreadSanitizer: reported 2 warnings
>> The fix requires new atomic functions to get, set, and compare-and-swap
>> an integer without a memory barrier.
>
> why ?
The fix needs a compare-and-swap function for an atomic integer. There
is no such function in libavutil/atomic.h.
Although the fix can use avpriv_atomic_int_get() and
avpriv_atomic_int_set(), these two functions execute a memory barrier
with sequentially-consistent ordering, which the fix doesn't need. To
improve performance, I added avpriv_atomic_int_get_relaxed() and
avpriv_atomic_int_set_relaxed()
>> The data race fix is written by Dmitry Vyukov of Google.
>
> Then the author for the git patch should be set accordingly
I will look into this. I may just identify him as a co-author.
>> @@ -44,7 +45,20 @@
>> #include
>> #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)
>> +
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Mon, 21 Nov 2016 15:37:48 -0800 Wan-Teh Chang wrote: > Hi, > > This patch makes the one-time initialization in av_get_cpu_flags() > thread-safe. The data race was reported by ThreadSanitizer. > > Wan-Teh Chang (1): > avutil: fix data race in av_get_cpu_flags(). > > 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(-) > Seems like a good idea, but it's probably better until the C11 atomics from Libav are merged. These can provide relaxed semantics as well. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
On Mon, Nov 21, 2016 at 03:37:49PM -0800, Wan-Teh Chang wrote:
> Make the one-time initialization in av_get_cpu_flags() thread-safe.
> The fix assumes -1 is an invalid value for cpu flags.
please explain the bug / race that occurs and how it is fixed
>
> The fix requires new atomic functions to get, set, and compare-and-swap
> an integer without a memory barrier.
why ?
>
> The data race fix is written by Dmitry Vyukov of Google.
Then the author for the git patch should be set accordingly
[...]
> @@ -44,7 +45,20 @@
> #include
> #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. */
src/libavutil/cpu.c: In function ‘get_cpu_flags’:
src/libavutil/cpu.c:61: error: control reaches end of non-void function
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have often repented speaking, but never of holding my tongue.
-- Xenocrates
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
