code review 4241041: sync/atomic: new package

sync/atomic: new package

Fixes issue 170.

[rsc, 2011-02-25 06:04:49]

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

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

[r, 2011-02-25 06:49:52]

does/should this affect the implementation of sync?

http://codereview.appspot.com/4241041/diff/1002/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/Makefile#newcode134
src/pkg/Makefile:134: sync/atomic\
sure you don't want sync/thermonuclear?

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_arm.s
File src/pkg/sync/atomic/asm_arm.s (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_arm.s...
src/pkg/sync/atomic/asm_arm.s:58: TEXT	·AddInt32(SB),7,$0
wrong indent

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_arm.s...
src/pkg/sync/atomic/asm_arm.s:80: TEXT	·AddUint64(SB),7,$0
wrong indent

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go
File src/pkg/sync/atomic/doc.go (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go#ne...
src/pkg/sync/atomic/doc.go:16: //
i'd like a few words of caution.

[rsc, 2011-02-25 07:02:19]

On Fri, Feb 25, 2011 at 01:49,  <r@golang.org> wrote:
> does/should this affect the implementation of sync?

i wish.  in theory we could make sync call these functions,
but the ancient arm systems screw things up.
on arm v5, there was no ll/sc, only a swap.
so when package sync wants to do an atomic op
on the arm v5, it simulates better atomicity using a tiny spin lock,
but that's only good for atomicity against other code that
also knows to use the tiny spin lock.

i intend sync/atomic to be real hw-supported atomic operations,
never simulated ones, so that you can use them to coordinate
with, say, c code.  so on the arm the sync/atomic operations
always use the atomic instructions, and if they're not there
on some machine, you get an invalid instruction fault.

if we toss arm v5 away, then sync can call into sync/atomic.
i don't know who cares about arm v5.  it is super old.
i will ask.

>
http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go#ne...
> src/pkg/sync/atomic/doc.go:16: //
> i'd like a few words of caution.

suggestions?

russ

[r2, 2011-02-25 08:05:24]

On Feb 24, 2011, at 11:02 PM, Russ Cox wrote:

> On Fri, Feb 25, 2011 at 01:49,  <r@golang.org> wrote:
>> does/should this affect the implementation of sync?
> 
> i wish.  in theory we could make sync call these functions,
> but the ancient arm systems screw things up.
> on arm v5, there was no ll/sc, only a swap.
> so when package sync wants to do an atomic op
> on the arm v5, it simulates better atomicity using a tiny spin lock,
> but that's only good for atomicity against other code that
> also knows to use the tiny spin lock.
> 
> i intend sync/atomic to be real hw-supported atomic operations,
> never simulated ones, so that you can use them to coordinate
> with, say, c code.  so on the arm the sync/atomic operations
> always use the atomic instructions, and if they're not there
> on some machine, you get an invalid instruction fault.
> 
> if we toss arm v5 away, then sync can call into sync/atomic.
> i don't know who cares about arm v5.  it is super old.
> i will ask.
> 
>>
http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go#ne...
>> src/pkg/sync/atomic/doc.go:16: //
>> i'd like a few words of caution.
> 
> suggestions?

These functions require great care to be used safely.  Except for special,
low-level
applications synchronization is better done with channels or the facilities of
the
sync package.  Share memory by communicating; don't communicate by sharing
memory.

-rob

[rsc, 2011-02-25 18:02:13]

Hello gri, iant, r, r2 (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-02-25 18:05:29]

I incorporated Rob's suggested comment, though I
changed safely to correctly: this package is type-safe
and memory-safe no matter how it is used.

I changed the ARM routines to be OS-specific, and
on Linux (the only ARM we have) they call the kernel-provided
functions at magic addresses.

I will update package sync in a separate CL.

Russ

[r2, 2011-02-25 18:08:06]

LGTM

http://codereview.appspot.com/4241041/diff/12/src/pkg/sync/atomic/asm_linux_a...
File src/pkg/sync/atomic/asm_linux_arm.s (right):

http://codereview.appspot.com/4241041/diff/12/src/pkg/sync/atomic/asm_linux_a...
src/pkg/sync/atomic/asm_linux_arm.s:8: // the kernel provides an appropriate
compare-and-swap
s/kernel/Linux kernel/
just to hammer the point home.

http://codereview.appspot.com/4241041/diff/12/src/pkg/sync/atomic/asm_linux_a...
src/pkg/sync/atomic/asm_linux_arm.s:15: TEXT cas<>(SB),7,$0
i don't remember what <> means.

http://codereview.appspot.com/4241041/diff/12/src/pkg/sync/atomic/atomic_test.go
File src/pkg/sync/atomic/atomic_test.go (right):

http://codereview.appspot.com/4241041/diff/12/src/pkg/sync/atomic/atomic_test...
src/pkg/sync/atomic/atomic_test.go:5: package atomic
be sure to run govet atomic_test.go

[rsc, 2011-02-25 18:10:05]

>
http://codereview.appspot.com/4241041/diff/12/src/pkg/sync/atomic/asm_linux_a...
> src/pkg/sync/atomic/asm_linux_arm.s:15: TEXT cas<>(SB),7,$0
> i don't remember what <> means.

it means C static (file local).

[r2, 2011-02-25 18:11:37]

On Feb 25, 2011, at 10:10 AM, Russ Cox wrote:

>>
http://codereview.appspot.com/4241041/diff/12/src/pkg/sync/atomic/asm_linux_a...
>> src/pkg/sync/atomic/asm_linux_arm.s:15: TEXT cas<>(SB),7,$0
>> i don't remember what <> means.
> 
> it means C static (file local).

ah yes, i remember now.

-rob

[gri, 2011-02-25 18:50:40]

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64.s
File src/pkg/sync/atomic/asm_amd64.s (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64...
src/pkg/sync/atomic/asm_amd64.s:6: JMP	·CompareAndSwapUint32(SB)
I guess there's no "fallthrough" (two entry points at the same address?) in this
assembler?

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64...
src/pkg/sync/atomic/asm_amd64.s:14: JZ 3(PC)
Couldn't you use a conditional move here and avoid the branch? (Cond. moves are
not available on all machines, but you already have restrictions in that
respect).

If the result were in a carry bit, one could use an ADC. Not sure if there's
another easy way to get the Z flag into a register.

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64...
src/pkg/sync/atomic/asm_amd64.s:64: MOVQ	CX, ret+8(FP)
+8? (not +16?)

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
File src/pkg/sync/atomic/atomic_test.go (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:23: // extend past the full word size.
This would find errors if a 32bit op uses a 64bit op internally. If there are
any +/- 1 address operations in the assembly, arguably one might also check that
the word before is not changed.

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:353: defer
runtime.GOMAXPROCS(runtime.GOMAXPROCS(4))
nice

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:353: defer
runtime.GOMAXPROCS(runtime.GOMAXPROCS(4))
this 4 here and the 4 below (for p) should be a const (I assume they are in
sync)

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:461: defer
runtime.GOMAXPROCS(runtime.GOMAXPROCS(4))
same here (constant 4)

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go
File src/pkg/sync/atomic/doc.go (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go#ne...
src/pkg/sync/atomic/doc.go:8: // The compare-and-swap operation, implemented by
the CompareAndSwap*
s/*/X/ ?

I was thinking pointer at first.

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go#ne...
src/pkg/sync/atomic/doc.go:26: func CompareAndSwapInt32(val *int32, old, new
int32) (swapped bool)
I assume the main reason for both Int32 and Uint32 variants is that otherwise
one would have to use an unpleasant pointer conversion via unsafe?

[rsc, 2011-02-25 18:54:51]

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64.s
File src/pkg/sync/atomic/asm_amd64.s (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64...
src/pkg/sync/atomic/asm_amd64.s:6: JMP	·CompareAndSwapUint32(SB)
On 2011/02/25 18:50:41, gri wrote:
> I guess there's no "fallthrough" (two entry points at the same address?) in
this
> assembler?

No.  The linker can reorder functions anyway.
It has enough information to rewrite direct calls,
though I don't know whether it does.  In any
event the locked bus transaction is orders of
magnitude more expensive than the jump.

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64...
src/pkg/sync/atomic/asm_amd64.s:14: JZ 3(PC)
On 2011/02/25 18:50:41, gri wrote:
> Couldn't you use a conditional move here and avoid the branch? (Cond. moves
are
> not available on all machines, but you already have restrictions in that
> respect).
> 
> If the result were in a carry bit, one could use an ADC. Not sure if there's
> another easy way to get the Z flag into a register.

Rewrote using SETEQ (SETE).

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/asm_amd64...
src/pkg/sync/atomic/asm_amd64.s:64: MOVQ	CX, ret+8(FP)
On 2011/02/25 18:50:41, gri wrote:
> +8? (not +16?)

Thanks.  Changed the tests to check return value of AddUint64
and fixed.

[rsc, 2011-02-25 19:04:41]

PTAL

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
File src/pkg/sync/atomic/atomic_test.go (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:23: // extend past the full word size.
On 2011/02/25 18:50:41, gri wrote:
> This would find errors if a 32bit op uses a 64bit op internally. If there are
> any +/- 1 address operations in the assembly, arguably one might also check
that
> the word before is not changed.

Done.

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:353: defer
runtime.GOMAXPROCS(runtime.GOMAXPROCS(4))
On 2011/02/25 18:50:41, gri wrote:
> this 4 here and the 4 below (for p) should be a const (I assume they are in
> sync)

Done.

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:461: defer
runtime.GOMAXPROCS(runtime.GOMAXPROCS(4))
On 2011/02/25 18:50:41, gri wrote:
> same here (constant 4)

Done.

[rsc, 2011-02-25 19:09:46]

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go
File src/pkg/sync/atomic/doc.go (right):

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go#ne...
src/pkg/sync/atomic/doc.go:8: // The compare-and-swap operation, implemented by
the CompareAndSwap*
On 2011/02/25 18:50:41, gri wrote:
> s/*/X/ ?
> 
> I was thinking pointer at first.

Changed to CompareAndSwapT

http://codereview.appspot.com/4241041/diff/1002/src/pkg/sync/atomic/doc.go#ne...
src/pkg/sync/atomic/doc.go:26: func CompareAndSwapInt32(val *int32, old, new
int32) (swapped bool)
On 2011/02/25 18:50:41, gri wrote:
> I assume the main reason for both Int32 and Uint32 variants is that otherwise
> one would have to use an unpleasant pointer conversion via unsafe?

Yes.  With the extra types there's no need to import "unsafe" to use
this package unless you are doing cas on pointers.

[gri, 2011-02-25 19:28:22]

LGTM

http://codereview.appspot.com/4241041/diff/3008/src/pkg/sync/atomic/atomic_te...
File src/pkg/sync/atomic/atomic_test.go (right):

http://codereview.appspot.com/4241041/diff/3008/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:22: // The struct field x.o checks that the
operations do not
This sentence needs to be adjusted.

[rsc, 2011-02-25 19:29:40]

*** Submitted as 9857d5ce1506 ***

sync/atomic: new package

Fixes issue 170.

R=gri, iant, r, r2
CC=golang-dev
http://codereview.appspot.com/4241041