[FFmpeg-devel] [PATCH] pthread: Avoid crashes/odd behavior caused by spurious wakeups

[Ben Jackson]

On Thu, Sep 13, 2012 at 09:41:37PM +0200, Michael Niedermayer wrote:
> 
> > @@ -214,7 +216,9 @@ static void* attribute_align_arg worker(void *v)
> >              if (c->current_job == thread_count + c->job_count)
> >                  pthread_cond_signal(&c->last_job_cond);
> >  
> > -            pthread_cond_wait(&c->current_job_cond, &c->current_job_lock);
> > +            while (last_execute == c->current_execute && !c->done)
> > +                pthread_cond_wait(&c->current_job_cond, &c->current_job_lock);
> > +            last_execute = c->current_execute;
> >              our_job = self_id;
> >  
> >              if (c->done) {
> 
> this will fail in 1 of 1<<32 cases due to current_execute being 0

All of the threads are made at the same time as ThreadContext.  Both
last_execute (in all workers) and current_execute start as 0.  Thereafter
all last_execute track current_execute.  Even if it wraps it doesn't
matter.

> > @@ -234,7 +238,8 @@ static void* attribute_align_arg worker(void *v)
> >  
> >  static av_always_inline void avcodec_thread_park_workers(ThreadContext *c, int thread_count)
> >  {
> > -    pthread_cond_wait(&c->last_job_cond, &c->current_job_lock);
> > +    while (c->current_job != thread_count + c->job_count)
> > +        pthread_cond_wait(&c->last_job_cond, &c->current_job_lock);
> >      pthread_mutex_unlock(&c->current_job_lock);
> >  }
> 
> I somehow think this should reset job_count to 0
> and then job_count can be used in the other test avoiding the
> current_execute issues

The job_count is tied up with stopping the workers.  The mechanism that
controls the workers is strange and I didn't want to mess with it.  I
found it interesting that thread_count (not available through
ThreadContext *c but necessary to test for completeness) was already
passed in -- I wondered if someone had deleted code like mine in the
past not understanding the necessity.

> > @@ -283,6 +288,7 @@ static int avcodec_thread_execute(AVCodecContext *avctx, action_func* func, void
> >          c->rets = &dummy_ret;
> >          c->rets_count = 1;
> >      }
> > +    c->current_execute++;
> 
> signed integer overflow and undefined behavior

Really?  Remember it only matters that it changes the value from execute
to execute.  I could just as easily have written '^= 1' instead of '++'.

I was just trying to make a minimal change to fix the problem.  To me
these comments treat the patch more harshly than the original code.  If
you'll only be happy with a rewrite of the slice worker code please say
so directly...

-- 
Ben Jackson AD7GD
<ben at ben.com>
http://www.ben.com/