code review 93280043: runtime, sync/atomic: add memory barriers in atomic ope...

runtime, sync/atomic: add memory barriers in atomic operations on ARM
Both runtime and sync/atomic atomic operations promise full memory barriers,
there are no barriers on ARM. Not sure how it was working before...
Pointed by Rui in https://codereview.appspot.com/91230048/
Have anybody seen unexplinable crashes on multicore ARM machines?
I can look into creating stress tests that should expose the missing barrier.
And I would appreciate if somebody tests it, because the change is completely blind on my side.

[dvyukov, 2014-05-12 17:07:28]

Hello golang-codereviews@googlegroups.com (cc: dave@cheney.net,
minux.ma@gmail.com, rsc@golang.org, ruiu@google.com),

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[dvyukov, 2014-05-12 17:08:03]

Now that I try to build it, I see that it won't work:
pkg/runtime/asm_arm.s:579 syntax error, last name: BYTE
We need to add DMB instruction to 5a.

[minux1, 2014-05-12 18:09:59]

https://codereview.appspot.com/93280043/diff/60001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/93280043/diff/60001/src/pkg/runtime/asm_arm.s#...
src/pkg/runtime/asm_arm.s:579: BYTE $0xf3; BYTE $0xbf; BYTE $0x8f; BYTE $0x5b //
DMB ISH
DMB is only supported on ARMv7.

for V6, we will need to use something like:
mcr p15, 0, REG, c7, c10, 5
for dmb (REG is an ARM register that must contain 0).

the problem is, how to switch between different versions here?
IIUC, armv7 also supports these (deprecated) cp15 instructions,
but i don't have time to try it out on real hardware now.

I guess we must have a stress test program to demonstrate the
problem so that we can be sure we've fixed the issue.

[minux1, 2014-05-12 18:10:39]

On 2014/05/12 17:08:03, dvyukov wrote:
> Now that I try to build it, I see that it won't work:
> pkg/runtime/asm_arm.s:579 syntax error, last name: BYTE
> We need to add DMB instruction to 5a.
use WORD $0x... for ARM

[ruiu, 2014-05-12 18:14:03]

I filed a bug for this: https://code.google.com/p/go/issues/detail?id=7977


On Mon, May 12, 2014 at 11:10 AM, <minux.ma@gmail.com> wrote:

> On 2014/05/12 17:08:03, dvyukov wrote:
>
>> Now that I try to build it, I see that it won't work:
>> pkg/runtime/asm_arm.s:579 syntax error, last name: BYTE
>> We need to add DMB instruction to 5a.
>>
> use WORD $0x... for ARM
>
> https://codereview.appspot.com/93280043/
>

[rsc, 2014-05-12 18:42:55]

NOT LGTM

I believe you that this could be a problem in theory, but it clearly isn't in
practice. Part of the reason is that we use kernel-provided primitives on Linux,
so Linux builds aren't using any of this code. It's also probably not a problem
on uniprocessors. So then the question becomes what's left of real Go usage on
ARM that is non-Linux multiprocessor systems?

I'm not comfortable making changes here this close to a release. The risk of
breaking real cases accidentally while fixing hypothetical ones seems too large.

Russ

[dvyukov, 2014-07-16 13:16:22]

On 2014/05/12 18:42:55, rsc wrote:
> NOT LGTM
> 
> I believe you that this could be a problem in theory, but it clearly isn't in
> practice. Part of the reason is that we use kernel-provided primitives on
Linux,
> so Linux builds aren't using any of this code. It's also probably not a
problem
> on uniprocessors. So then the question becomes what's left of real Go usage on
> ARM that is non-Linux multiprocessor systems?
> 
> I'm not comfortable making changes here this close to a release. The risk of
> breaking real cases accidentally while fixing hypothetical ones seems too
large.
> 
> Russ


I think that now it affects nacl/arm.

[minux, 2014-07-16 19:59:50]

On 2014/07/16 13:16:22, dvyukov wrote:
> I think that now it affects nacl/arm.
right, at least in theory:
nacl/arm only supports armv7a and we will be using plain LDREX/STREX for
atomics, it should be possible to demonstrate this problem with a stress
test.

could you please write one and I can test on the builder, which has two
processor cores.

once we have a failing test case, we can go on with this CL.

[dvyukov, 2014-07-16 20:03:39]

TestStoreLoadSeqCst and TestStoreLoadRelAcq should demonstrate missing
barriers. Are they executed on the builder? Try to run them lots of
time in non short mode.


On Wed, Jul 16, 2014 at 11:59 PM,  <minux@golang.org> wrote:
> On 2014/07/16 13:16:22, dvyukov wrote:
>>
>> I think that now it affects nacl/arm.
>
> right, at least in theory:
> nacl/arm only supports armv7a and we will be using plain LDREX/STREX for
> atomics, it should be possible to demonstrate this problem with a stress
> test.
>
> could you please write one and I can test on the builder, which has two
> processor cores.
>
> once we have a failing test case, we can go on with this CL.
>
> https://codereview.appspot.com/93280043/

[minux, 2014-07-16 20:08:01]

On Wed, Jul 16, 2014 at 4:03 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> TestStoreLoadSeqCst and TestStoreLoadRelAcq should demonstrate missing
> barriers. Are they executed on the builder? Try to run them lots of
> time in non short mode.

Yes, they are run on the builder. I will try running them in a loop.

[minux, 2014-07-16 20:11:29]

On Wed, Jul 16, 2014 at 4:07 PM, minux <minux@golang.org> wrote:

> On Wed, Jul 16, 2014 at 4:03 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> TestStoreLoadSeqCst and TestStoreLoadRelAcq should demonstrate missing
>> barriers. Are they executed on the builder? Try to run them lots of
>> time in non short mode.
>
> Yes, they are run on the builder. I will try running them in a loop.
>
Scratch that. They are NOT run on the builder because runtime.NumCPU() will
always
be 1 on nacl.

I will modify the runtime to report 2 for NumCPU() and then run the tests
in a loop.

[minux, 2014-07-17 17:39:10]

On Wed, Jul 16, 2014 at 4:11 PM, minux <minux@golang.org> wrote:

> On Wed, Jul 16, 2014 at 4:07 PM, minux <minux@golang.org> wrote:
>
>> On Wed, Jul 16, 2014 at 4:03 PM, Dmitry Vyukov <dvyukov@google.com>
>> wrote:
>>
>>> TestStoreLoadSeqCst and TestStoreLoadRelAcq should demonstrate missing
>>> barriers. Are they executed on the builder? Try to run them lots of
>>> time in non short mode.
>>
>> Yes, they are run on the builder. I will try running them in a loop.
>>
> Scratch that. They are NOT run on the builder because runtime.NumCPU()
> will always
> be 1 on nacl.
>
> I will modify the runtime to report 2 for NumCPU() and then run the tests
> in a loop.
>
I modified runtime to always report 2 for NumCPU(), and then run those 4
tests in a loop for
500168 times. Not a single failure. Perhaps this is a nacl restriction that
we can't reach
the required level of concurrency.

I will modify the linux code to always use the generic implementation and
try again.

[rsc, 2014-07-17 17:40:01]

This doesn't really matter. It's still a hypothetical.

[minux, 2014-07-18 06:55:19]

On Thu, Jul 17, 2014 at 1:40 PM, Russ Cox <rsc@golang.org> wrote:

> This doesn't really matter. It's still a hypothetical.
>
Yeah, after running atomic.test with GOMAXPROCS==4 on SMP machine with
asm_linux_arm.s replaced with
asm_netbsd_arm.s more than 7000 times and not a single failure. I think
either our stress test is not enough
to demonstate the problem, or we're safe to not use dmb.

However, the linux kernel's implementation of cas64 helper does use dmb on
SMP systems:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm...

[dvyukov, 2014-07-18 08:52:34]

I can say that TestStoreLoadSeqCst reliably fails on amd64 if you
implement AtomicStore as plain MOV. I don't know what happens on ARM.
I see that we implement all loads and stores as CAS-loop (which is
quite an overkill), so maybe LDREX/STREX have an implicit DMB on your
machine...


On Fri, Jul 18, 2014 at 10:54 AM, minux <minux@golang.org> wrote:
>
> On Thu, Jul 17, 2014 at 1:40 PM, Russ Cox <rsc@golang.org> wrote:
>>
>> This doesn't really matter. It's still a hypothetical.
>
> Yeah, after running atomic.test with GOMAXPROCS==4 on SMP machine with
> asm_linux_arm.s replaced with
> asm_netbsd_arm.s more than 7000 times and not a single failure. I think
> either our stress test is not enough
> to demonstate the problem, or we're safe to not use dmb.
>
> However, the linux kernel's implementation of cas64 helper does use dmb on
> SMP systems:
>
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm...

[gobot, 2014-12-19 05:19:06]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/93280043 is best) in the description in your
new CL.

Thanks very much.