Add a lock around getting and setting of source_ to avoid possible compiler reorderings.

Add a lock around getting and setting of source_ to avoid possible compiler reorderings.

Also makes it very explicit that multi-threaded access is occuring.

BUG=24801
TEST=will see how TSan handles this.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102067

[awong, 2011-09-14 21:00:21]

Carlos, I can't see how this is could deadlock, unless the RenderCallback needs
to be reentrant.  Is there some documentation somewhere that actually says it
has to be reentrant?

Cause, excluding the same thread trying to grab the lock twice, I can't see any
possible deadlock.

[awong, 2011-09-14 21:07:11]

Frig. I just saw your comment.  Okay, fine.  So it may reenter. Frig.

On 2011/09/14 21:00:21, awong wrote:
> Carlos, I can't see how this is could deadlock, unless the RenderCallback
needs
> to be reentrant.  Is there some documentation somewhere that actually says it
> has to be reentrant?
> 
> Cause, excluding the same thread trying to grab the lock twice, I can't see
any
> possible deadlock.

[awong, 2011-09-14 21:17:51]

Looking at the documentation for AudioQueueNewInput:

http://developer.apple.com/library/mac/#documentation/MusicAudio/Reference/Au...

I'm not seeing a mention of this possibly reentrancy behavior.  Also, is the
style of reentrancy where the current executing function can be preempted before
it returns on the same execution stack possible? That seems like it'd break all
sorts of stuff inside our callback.

Basically, is the comment being overly conservative?

-Albert


On 2011/09/14 21:07:11, awong wrote:
> Frig. I just saw your comment.  Okay, fine.  So it may reenter. Frig.
> 
> On 2011/09/14 21:00:21, awong wrote:
> > Carlos, I can't see how this is could deadlock, unless the RenderCallback
> needs
> > to be reentrant.  Is there some documentation somewhere that actually says
it
> > has to be reentrant?
> > 
> > Cause, excluding the same thread trying to grab the lock twice, I can't see
> any
> > possible deadlock.

[cpu_(ooo_6.6-7.5), 2011-09-15 04:19:33]

That pattern is equivalent to an atomic set, I mean there is no need for a lock
member,  MemoryBarrier() from atomicops.h will do exactly the same, faster and
without having to carry a lock in for each object.

But in reality the code is such that it is an aligned write to natural word size
so, my best guess is that the current code is the same but even faster than a
barrier.

So that leaves us with the case of the compiler randomly using your member
variables as scratchpad, aka source_ = 42

In that 'volatile' will solve the problem.

[cpu_(ooo_6.6-7.5), 2011-09-15 04:24:26]

The one way a lock will actually do something would be if it extends past the
set/get of source_, like for example also enclosing the code of line 325
(AudioQueueStop)

However that (I think but not 100% sure) it will cause a deadlock. In particular
deadlock by lock inversion since the OS is probably executing the callback with
its audiolock taken.

That is what I meant by deadlock.

[awong, 2011-09-15 17:26:41]

Hey Carlos,

Thanks for the explanation.  Makes a lot more sense, and matches the memory
model I was originally thinking about.

Ami and I actually talked over using a memory barrier + volatile yesterday to
avoid locks in this code, but after some discussion decided that trading a mutex
for a barrier, while making the code faster, increases cognitive overhead
significantly cause lots of people get the heebie-jeebies when they try to
reason about threading

For instance, I know my first reaction was to question memory alignment over all
architectures.  And even for volatile, I had to go relookup the reordering rules
and convince myself that even though they can be re-ordered with respect to
non-volatile statements inside the same block, this was still good enough in our
case.

That's a long way of saying, since this lock is never contended except for at
shutdown, I'd rather choose readability in this case.

If you feel strongly enough that this is wrong, we can discuss over IM.

-Albert


On 2011/09/15 04:24:26, cpu wrote:
> The one way a lock will actually do something would be if it extends past the
> set/get of source_, like for example also enclosing the code of line 325
> (AudioQueueStop)
> 
> However that (I think but not 100% sure) it will cause a deadlock. In
particular
> deadlock by lock inversion since the OS is probably executing the callback
with
> its audiolock taken.
> 
> That is what I meant by deadlock.

[cpu_(ooo_6.6-7.5), 2011-09-15 18:28:21]

lgtm.

Sorry for making so many people waste time. It was a learning experience.

[commit-bot: I haz the power, 2011-09-21 01:53:28]

Change committed as 102067