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

[Reimar Döffinger]

On Tue, Jan 03, 2012 at 09:58:04AM +0100, Dan Oscarsson wrote:
> > 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 don't think that will work, on some architectures there is no
guarantee when writes will happen, so the following is possible without
locks:

do_work = 0 is executed
cache processing is done
on a different CPU, do_work = 1 is executed and due to cache pressures
flushed out into RAM
only now is the write of do_work = 0 executed.

End result is a deadlock.
Note that this _should_ not be possible on x86 since it has quite a
strict coherency model (even though it is still confusing and risky
to rely on such stuff).
However in the case of your code you run into another issue:
you did not mark do_work as volatile, thus since the assignment is
outside the locks, the _compiler_ is perfectly free to actually move
the assignment to after the cache reading.