[FFmpeg-devel] [PATCH] Always read frame progress values under progress_mutex.

[Michael Niedermayer]

On Thu, Feb 25, 2016 at 04:42:22PM -0800, Wan-Teh Chang wrote:
> On Thu, Feb 25, 2016 at 4:15 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >
> > Similar to the other patch, please explain what this actually fixes,
> > those sanitizer tools produce a lot of false positives and nonsensical
> > warnings.
> 
> Yes, certainly.
> 
> In libavcodec/pthread_frame.c, we have:
> 
>  46 /**
>  47  * Context used by codec threads and stored in their
> AVCodecInternal thread_    ctx.
>  48  */
>  49 typedef struct PerThreadContext {
>  ...
>  59     pthread_mutex_t progress_mutex; ///< Mutex used to protect
> frame progress values and progress_cond.
> 
> The comment on line 59 documents that progress_mutex guards frame
> progress values and progress_cond. (In fact, progress_mutex also
> guards the |state| field in many places, but that's not the subject of
> this patch.)
> 
> I assume "frame progess values" mean f->progress->data, where |f|
> points to a ThreadFrame. I inspected libavcodec/pthread_frame.c and
> concluded the code (ff_thread_report_progress and
> ff_thread_await_progress) matches that comment, except that it also
> does a double-checked locking style performance optimization. For
> example, here is ff_thread_report_progress:
> 
> 472 void ff_thread_report_progress(ThreadFrame *f, int n, int field)
> 473 {
> 474     PerThreadContext *p;
> 475     volatile int *progress = f->progress ? (int*)f->progress->data : NULL;
> 476
> 477     if (!progress || progress[field] >= n) return;
> 478
> 479     p = f->owner->internal->thread_ctx;
> 480
> 481     if (f->owner->debug&FF_DEBUG_THREADS)
> 482         av_log(f->owner, AV_LOG_DEBUG, "%p finished %d field
> %d\n", progress    , n, field);
> 483
> 484     pthread_mutex_lock(&p->progress_mutex);
> 485     progress[field] = n;
> 486     pthread_cond_broadcast(&p->progress_cond);
> 487     pthread_mutex_unlock(&p->progress_mutex);
> 488 }
> 
> progress[field] can only increase. So, both ff_thread_report_progress
> and ff_thread_await_progress have a fast code path (line 477 and line
> 495, respectively) that reads progress[field] without acquiring
> progress_mutex. If the speculatively read value of progress[field] is
> >= |n|, then the functions return immediately. Otherwise, the
> functions take the slow code path, which acquires progress_mutex
> properly and does what the functions need to do.
> 
> The fast code path is in the style of double-checked locking and has a
> data race. ThreadSanitizer is right in this case.

ThreadSanitizer is right about that this is a data race yes, but
isnt all lock-free code technically containing a data race

But is there a bug?
Its basically a simple 2 thread thing, one says "iam finished" the
other checks if that is set.
In what case does this benefit from a lock ?
also note the write is always under a lock, which id assume does
any needed memory barriers

also considering this is performance relevant code, please provide
a benchmark

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160226/e39f4dc7/attachment.sig>