Fix data race in v8::internal::UnboundQueue

Fix data race in v8::internal::UnboundQueue

This change modifies memory accesses to ensure proper load/store ordering.

BUG=249750
R=dvyukov@google.com, jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15219

[Dmitry Vyukov, 2013-06-17 19:51:57]

LGTM

https://codereview.chromium.org/17294004/diff/1/src/unbound-queue.h
File src/unbound-queue.h (right):

https://codereview.chromium.org/17294004/diff/1/src/unbound-queue.h#newcode51
src/unbound-queue.h:51: INLINE(bool IsEmpty()) { return divider_ == last_; }
this must use NoBarrier_Load's

[Dmitry Vyukov, 2013-06-17 19:54:33]

FYI, there is also an intrusive spsc queue algorithm:
https://groups.google.com/forum/?hl=ru&fromgroups#!topic/comp.programming.thr...
But in general I think array-based bounded queue is a better fit here. It does
not need to traverse linked lists, allocate and use mutex-based queue in
tcmalloc to pass the nodes back from consumer to producer.

[yurys, 2013-06-18 08:01:00]

Thank you for review! One question: am I right that these particular data races
should never be a problem on x86/AMD64 due to their memory ordering model?

https://codereview.chromium.org/17294004/diff/1/src/unbound-queue.h
File src/unbound-queue.h (right):

https://codereview.chromium.org/17294004/diff/1/src/unbound-queue.h#newcode51
src/unbound-queue.h:51: INLINE(bool IsEmpty()) { return divider_ == last_; }
On 2013/06/17 19:51:57, Dmitry Vyukov wrote:
> this must use NoBarrier_Load's

Done.

[Jakob Kummerow, 2013-06-18 08:29:57]

lgtm

[Dmitry Vyukov, 2013-06-18 09:17:59]

On 2013/06/18 08:01:00, Yury Semikhatsky wrote:
> Thank you for review! One question: am I right that these particular data
races
> should never be a problem on x86/AMD64 due to their memory ordering model?

x86/AMD64 memory model is relevant iff you program in x86/AMD64 machine codes.
Please read this:
http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could...

[yurys, 2013-06-18 10:19:55]

On 2013/06/18 09:17:59, Dmitry Vyukov wrote:
> On 2013/06/18 08:01:00, Yury Semikhatsky wrote:
> > Thank you for review! One question: am I right that these particular data
> races
> > should never be a problem on x86/AMD64 due to their memory ordering model?
> 
> x86/AMD64 memory model is relevant iff you program in x86/AMD64 machine codes.
> Please read this:
>
http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could...

Thanks for the pointer, it is an excellent write-up of the problem!

[Alexander Potapenko, 2013-06-18 12:49:24]

This patch doesn't apply to the bleeding edge branch.
Looking at
https://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/unbound...
I see that it's using OS::ReleaseStore, while the base file for this CL appears
to be using Release_Store()
My suggestion is to disemploy OS::ReleaseStore in favor of
v8::internal::Release_Store(), so if the mismatch is because of some other
upcoming patch doing so, that's great.

[yurys, 2013-06-18 13:10:05]

On 2013/06/18 12:49:24, Alexander Potapenko wrote:
> This patch doesn't apply to the bleeding edge branch.
> Looking at
>
https://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/unbound...
> I see that it's using OS::ReleaseStore, while the base file for this CL
appears
> to be using Release_Store()
> My suggestion is to disemploy OS::ReleaseStore in favor of
> v8::internal::Release_Store(), so if the mismatch is because of some other
> upcoming patch doing so, that's great.

This patch depends on https://codereview.chromium.org/17222004/

[Alexander Potapenko, 2013-06-18 14:25:49]

> This patch depends on https://codereview.chromium.org/17222004/
Thanks for the info!
I confirm that the race reports in UnboundQueue aren't reported with these two
patches.

[yurys, 2013-06-20 06:23:44]

Committed patchset #2 manually as r15219 (presubmit successful).