Re: [FFmpeg-devel] [PATCH] threadprogress: reorder instructions to silence tsan warning.

[Zhao Zhili <quinkblack-at-foxmail.com@ffmpeg.org>]

> On Feb 7, 2025, at 19:39, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> Andreas Rheinhardt:
>> Ronald S. Bultje:
>>> Fixes #11456.
>>> ---
>>> libavcodec/threadprogress.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>> 
>>> diff --git a/libavcodec/threadprogress.c b/libavcodec/threadprogress.c
>>> index 62c4fd898b..aa72ff80e7 100644
>>> --- a/libavcodec/threadprogress.c
>>> +++ b/libavcodec/threadprogress.c
>>> @@ -55,9 +55,8 @@ void ff_thread_progress_report(ThreadProgress *pro, int n)
>>>     if (atomic_load_explicit(&pro->progress, memory_order_relaxed) >= n)
>>>         return;
>>> 
>>> -    atomic_store_explicit(&pro->progress, n, memory_order_release);
>>> -
>>>     ff_mutex_lock(&pro->progress_mutex);
>>> +    atomic_store_explicit(&pro->progress, n, memory_order_release);
>>>     ff_cond_broadcast(&pro->progress_cond);
>>>     ff_mutex_unlock(&pro->progress_mutex);
>>> }
>> 
>> I don't really understand why this is supposed to fix a race; after all,
>> the synchronisation of ff_thread_progress_(report|await) is not supposed
>> to be provided by the mutex (which is avoided altogether in the fast
>> path in ff_thread_report_await()), but by storing and loading the
>> progress variable.
>> That's also the reason why I moved this outside of the mutex (compared
>> to ff_thread_report_progress(). (This way it is possible for a consumer
>> thread to see the new progress value earlier and possibly avoid the
>> mutex altogether.)
>> 
> 
> Damn, this optimization works, but only if the progress variable is
> always read with acquire-semantics; it is currently read via
> memory_order_relaxed inside the mutex (just like in
> ff_thread_await_progress()).
> 
> According to my understanding, this is what happens:
> Consumer thread waits for progress and finds that it is insufficient
> (fast path fails)
> Producer thread updates progress variable
> Consumer thread acquires the mutex and reads new progress via
> memory_order_relaxed
> Producer thread acquires mutex and broadcasts the new progress
> 
> I'd prefer to change these semantics so that we always perform
> synchronisation via the atomic progress variable (unless you know of a
> performance impact -- I only know that on x86, both memory_order_relaxed
> and memory_order_acquire are ordinary loads).

I have considered the solution too, by always use memory_order_acquire
in wait progress. memory_order_relaxed is normal load on ARM, while
memory_order_acquire isn’t. So there is real difference.

https://developer.arm.com/documentation/dui0801/l/A64-Data-Transfer-Instructions/LDAPR--A64-

Now it’s weird to use memory_order_acquire inside mutex lock.

> 
> Thanks for looking into this.
> 
> - Andreas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".