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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Zhao Zhili:
> 
> 
>> On Feb 7, 2025, at 19:22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
>>
>> 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.)
> 
> As I understand it, there is no real race condition, that’s why the patch
> says “silence tsan warning”.

There is a race (in fact, both a data race and a race condition (the
latter doesn't happen on x86 with its strong memory model though); see
my other mail for an explanation.

> 
> I have considered another idea to keep tsan clean and keep the benefit
> of set progress earlier: use another non-atomic progress together with
> mutex/cond, so atomic and mutex/cond are used separately. Not sure
> whether it’s worth the complexity.
> 

So we would have one atomic and one non-atomic progress variable and a
mutex? And the atomic progress is set as now and only read for the fast
path and the nonatomic variable is set and read inside the mutex? I
don't really think this is better than any of the alternatives.

- 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".