On Fri, 26 Feb 2016 05:14:47 +0100
Michael Niedermayer <mich...@niedermayer.cc> wrote:
> 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.lepp...@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
I don't know about this case, but the frame threading code uses
improper double-checked logging. Basically it doesn't use proper
barrier or atomics, assumes x86 semantics, and hopes the compiler won't
get into the way.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
