[MPlayer-dev-eng] [PATCH] threaded cache, round 2

[Dan Oscarsson]

mån 2012-01-02 klockan 22:06 +0000 skrev Martin Simmons:
> >>>>> On Mon, 02 Jan 2012 17:59:19 +0100, Dan Oscarsson said:
> > 
> > +#ifdef PTHREAD_CACHE
> > +    if (!((cache_vars_t *)s->cache_data)->do_work) {
> > +        pthread_mutex_lock(&((cache_vars_t *)s->cache_data)->go_ahead_mutex);
> > +           ((cache_vars_t *)s->cache_data)->do_work = 1;
> > +        pthread_mutex_unlock(&((cache_vars_t *)s->cache_data)->go_ahead_mutex);
> > +        pthread_cond_signal(&((cache_vars_t *)s->cache_data)->go_ahead);
> 
> The last two lines are in the wrong order -- to avoid race conditions, you
> must signal while holding the lock.

No, not from my analysis. If cache thread detects that do_work is 1
before the signal is sent, it does not matter. Either the cache thread
detects that do_work is 1 and starts to work, or it locks the mutex
before do_work is set to 1, and waits for signal instead.

> 
> 
> > +#ifdef PTHREAD_CACHE
> > +    if (!s->do_work) {
> > +        pthread_mutex_lock(&s->go_ahead_mutex);
> > +           s->do_work = 1;
> > +        pthread_mutex_unlock(&s->go_ahead_mutex);
> > +        pthread_cond_signal(&s->go_ahead);
> 
> Ditto.
> 
> 
> > +#ifdef PTHREAD_CACHE
> > +            if (!s->do_work) {
> > +                pthread_mutex_lock(&s->go_ahead_mutex);
> > +                    while (!s->do_work) pthread_cond_wait(&s->go_ahead, &s->go_ahead_mutex);
> > +                pthread_mutex_unlock(&s->go_ahead_mutex);
> > +            }
> > +            s->do_work = 0;
> 
> Set s->do_work to 0 while holding the lock inside the if, also to avoid race
> conditions.

Quite often it is so, but not in this case (unless my analysis is
wrong). When cache thread sets do_work to 0 above, it is going to do
some work (reading data and command processing), so it does not matter
if do_work is set to 1 by main thread and that is erased by this setting
to zero.

I moved pthread_cond_signal and the setting of do_work = 0 outside the
mutex lock to reduce time spent inside the lock as I it looks like the
cannot be a race condition in this case.
We can move them back in even if nobody can show a case where it gets
wrong, just to be sure.

   Dan