code review 4673055: runtime: fix data race during Itab hash update/lookup

runtime: fix data race during Itab hash update/lookup
The data race is on newly published Itab nodes, which are
both unsafely published and unsafely acquired. It can
break on IA-32/Intel64 due to compiler optimizations
(most likely not an issue as of now) and on ARM due to
hardware memory access reorderings.

[dvyukov, 2011-07-11 15:41:44]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-07-12 05:33:12]

Nice

http://codereview.appspot.com/4673055/diff/8002/src/pkg/runtime/arm/atomic.c
File src/pkg/runtime/arm/atomic.c (right):

http://codereview.appspot.com/4673055/diff/8002/src/pkg/runtime/arm/atomic.c#...
src/pkg/runtime/arm/atomic.c:18: void* v;
return (void*)runtime·xadd((uint32 volatile*)addr, 0);

http://codereview.appspot.com/4673055/diff/8002/src/pkg/runtime/iface.c
File src/pkg/runtime/iface.c (right):

http://codereview.appspot.com/4673055/diff/8002/src/pkg/runtime/iface.c#newco...
src/pkg/runtime/iface.c:150: runtime·panicstring("itab: concurrent Itab hash
update");
The casp seems like overkill.
I'd just do

out:
    if(!locked)
        runtime·throw("invalid itab locking");
    m->link = hash[h];
    hash[h] = m;
    if(m->bad) ...

[dvyukov, 2011-07-12 10:52:00]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dvyukov, 2011-07-12 10:59:38]

>
http://codereview.appspot.com/4673055/diff/8002/src/pkg/runtime/arm/atomic.c#...
> src/pkg/runtime/arm/atomic.c:18: void* v;
> return (void*)runtime·xadd((uint32 volatile*)addr, 0);

Aha! ARM is always 32-bit!

> http://codereview.appspot.com/4673055/diff/8002/src/pkg/runtime/iface.c
> File src/pkg/runtime/iface.c (right):
> 
>
http://codereview.appspot.com/4673055/diff/8002/src/pkg/runtime/iface.c#newco...
> src/pkg/runtime/iface.c:150: runtime·panicstring("itab: concurrent Itab hash
> update");
> The casp seems like overkill.
> I'd just do
> 
> out:
>     if(!locked)
>         runtime·throw("invalid itab locking");
>     m->link = hash[h];
>     hash[h] = m;
>     if(m->bad) ...

What we need here is:

m = atomic_load(&hash[i], memory_order_consume/acquire);
...

...
atomic_store(&hash[i], m, memory_order_release);

As far as I understand (I am not an expert on ARM), on ARM it is:

LOAD &hash[i], m
DMB
...

...
DMB
STORE m, &hash[i]

That is, both writer and reader need to execute DMB instruction. So plain
load/store won't do.

[rsc, 2011-07-12 15:51:59]

okay, then please add atomicstorep.

[dvyukov, 2011-07-13 09:09:16]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-07-13 18:21:58]

LGTM

[rsc, 2011-07-13 18:22:44]

*** Submitted as http://code.google.com/p/go/source/detail?r=de991447d8ea ***

runtime: fix data race during Itab hash update/lookup
The data race is on newly published Itab nodes, which are
both unsafely published and unsafely acquired. It can
break on IA-32/Intel64 due to compiler optimizations
(most likely not an issue as of now) and on ARM due to
hardware memory access reorderings.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4673055

Committer: Russ Cox <rsc@golang.org>