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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
