code review 7001056: sync/atomic: document that users must deal with 64-bit ...

sync/atomic: document that users must deal with 64-bit alignment

Update issue 599.

[rsc, 2012-12-22 23:53:48]

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

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

[iant, 2012-12-23 06:59:47]

LGTM

Can we provide any guarantees about int64 global variables?

[dvyukov, 2012-12-23 10:05:24]

We can fix it in the library. It's possible to implement atomic
operations on non-aligned vars. All LOCK prefixed operations are
atomic regardless of alignment. It will incur one additional branch
for aligned vars, for non-aligned var it will LOCK prefixed
instructions + there is huge overhead for LOCK prefixed operations
that occupy 2 cache lines (it can be 5000+ cycles vs 15 cycles).
I don't know about LL/SC on ARM.

On Sun, Dec 23, 2012 at 10:59 AM,  <iant@golang.org> wrote:
> LGTM
>
> Can we provide any guarantees about int64 global variables?
>
>
> https://codereview.appspot.com/7001056/

[minux1, 2012-12-23 13:14:18]

https://codereview.appspot.com/7001056/diff/5001/src/pkg/sync/atomic/doc.go
File src/pkg/sync/atomic/doc.go (right):

https://codereview.appspot.com/7001056/diff/5001/src/pkg/sync/atomic/doc.go#n...
src/pkg/sync/atomic/doc.go:41: // BUG(rsc): On ARM, the 64-bit functions use
instructions unavailable before ARM 11.
while you're at it, please remove this BUG, as it's no longer true
for Linux/ARM (though it's still true for FreeBSD/ARM).

[minux1, 2012-12-23 13:21:58]

On 2012/12/23 10:05:24, dvyukov wrote:
> We can fix it in the library. It's possible to implement atomic
> operations on non-aligned vars. All LOCK prefixed operations are
> atomic regardless of alignment. It will incur one additional branch
> for aligned vars, for non-aligned var it will LOCK prefixed
> instructions + there is huge overhead for LOCK prefixed operations
> that occupy 2 cache lines (it can be 5000+ cycles vs 15 cycles).
> I don't know about LL/SC on ARM.
ARM ARM specifies LDREXD/STREXD must have 64-bit alignment,
or alignment fault will result.

[rsc, 2012-12-23 19:21:24]

I'm happy to add global variables to the list of aligned things. We align
all >=8-byte variables to 8-byte boundaries.

"We can fix it" + "huge overhead" doesn't sound like a fix. I think it's
fine to tell people who use sync/atomic that they have to work a little bit
for it. Especially since it might not be possible on ARM or other future
platforms.

Russ

[dvyukov, 2012-12-24 08:07:00]

On Sun, Dec 23, 2012 at 11:21 PM, Russ Cox <rsc@golang.org> wrote:
> I'm happy to add global variables to the list of aligned things. We align
> all >=8-byte variables to 8-byte boundaries.
>
> "We can fix it" + "huge overhead" doesn't sound like a fix. I think it's
> fine to tell people who use sync/atomic that they have to work a little bit
> for it. Especially since it might not be possible on ARM or other future
> platforms.

It's possible on any platform with global mutex hashtable. But it will
be slower and not "lock-free".

What I afraid is that it can lead to extremely hard to diagnose issues
and the comment won't actually help people. Perhaps add a runtime
check for alignment.

[minux1, 2012-12-24 08:21:04]

On Monday, December 24, 2012, Dmitry Vyukov wrote:

> On Sun, Dec 23, 2012 at 11:21 PM, Russ Cox <rsc@golang.org <javascript:;>>
> wrote:
> > I'm happy to add global variables to the list of aligned things. We align
> > all >=8-byte variables to 8-byte boundaries.
> >
> > "We can fix it" + "huge overhead" doesn't sound like a fix. I think it's
> > fine to tell people who use sync/atomic that they have to work a little
> bit
> > for it. Especially since it might not be possible on ARM or other future
> > platforms.
>
> It's possible on any platform with global mutex hashtable. But it will
> be slower and not "lock-free".

using that defeated the very purpose of using atomic instructions.
(we already uses that for 64-bit arm atomics in runtime unconditionally,
and we should fix that to use native instr. or kernel helper if avail.
i want to move part or all of core atomics package into runtime,
as their functionality is currently duplicated in runtime. what do you
think?)

>
> What I afraid is that it can lead to extremely hard to diagnose issues
> and the comment won't actually help people. Perhaps add a runtime
> check for alignment.
>
agreed. we should panic if the address is not properly aligned.
the problem is, should we panic on x86 where unaligned atomics
is tolerated? i think we should, to provide an uniform api and help
people who develop on 386 for arm.

[dvyukov, 2012-12-24 08:33:15]

On Mon, Dec 24, 2012 at 12:21 PM, minux <minux.ma@gmail.com> wrote:
>> > I'm happy to add global variables to the list of aligned things. We
>> > align
>> > all >=8-byte variables to 8-byte boundaries.
>> >
>> > "We can fix it" + "huge overhead" doesn't sound like a fix. I think it's
>> > fine to tell people who use sync/atomic that they have to work a little
>> > bit
>> > for it. Especially since it might not be possible on ARM or other future
>> > platforms.
>>
>> It's possible on any platform with global mutex hashtable. But it will
>> be slower and not "lock-free".
>
> using that defeated the very purpose of using atomic instructions.

It's a difficult question, because hardware uses mutexes in implement
atomic ops anyway :)
However, of course if we add software mutexes, it will bring blocking
to OS level.

> (we already uses that for 64-bit arm atomics in runtime unconditionally,
> and we should fix that to use native instr. or kernel helper if avail.
> i want to move part or all of core atomics package into runtime,
> as their functionality is currently duplicated in runtime. what do you
> think?)

When I proposed it some time ago, Russ said that he does not want to
unify them because one is C called and another is Go called.
It's possible with an additional level of abstraction :) Maybe Russ
has changed his mind...


>> What I afraid is that it can lead to extremely hard to diagnose issues
>> and the comment won't actually help people. Perhaps add a runtime
>> check for alignment.
>
> agreed. we should panic if the address is not properly aligned.
> the problem is, should we panic on x86 where unaligned atomics
> is tolerated? i think we should, to provide an uniform api and help
> people who develop on 386 for arm.

64-bit atomic load/store are not atomic on 386/amd64 (when unaligned).
That was the original issue than I hit with GC on 386 with
GOMAXPROCS>1. So there is the same problem.

[minux1, 2012-12-24 09:23:56]

On Monday, December 24, 2012, Dmitry Vyukov wrote:
>
> 64-bit atomic load/store are not atomic on 386/amd64 (when unaligned).
> That was the original issue than I hit with GC on 386 with
> GOMAXPROCS>1. So there is the same problem.
>
i was confused by your saying "All LOCK prefixed operations are
atomic regardless of alignment", so you mean the instruction itself
is atomic whereas the memory access is not?

[dvyukov, 2012-12-24 09:28:24]

On Mon, Dec 24, 2012 at 1:23 PM, minux <minux.ma@gmail.com> wrote:
>
> On Monday, December 24, 2012, Dmitry Vyukov wrote:
>>
>> 64-bit atomic load/store are not atomic on 386/amd64 (when unaligned).
>> That was the original issue than I hit with GC on 386 with
>> GOMAXPROCS>1. So there is the same problem.
>
> i was confused by your saying "All LOCK prefixed operations are
> atomic regardless of alignment", so you mean the instruction itself
> is atomic whereas the memory access is not?

64-bit atomic load on amd64/386 does not use LOCK prefixed instructions.

[minux1, 2012-12-24 09:37:09]

On Monday, December 24, 2012, Dmitry Vyukov wrote:

> On Mon, Dec 24, 2012 at 1:23 PM, minux <minux.ma@gmail.com <javascript:;>>
> wrote:
> > On Monday, December 24, 2012, Dmitry Vyukov wrote:
> >> 64-bit atomic load/store are not atomic on 386/amd64 (when unaligned).
> >> That was the original issue than I hit with GC on 386 with
> >> GOMAXPROCS>1. So there is the same problem.
> > i was confused by your saying "All LOCK prefixed operations are
> > atomic regardless of alignment", so you mean the instruction itself
> > is atomic whereas the memory access is not?
> 64-bit atomic load on amd64/386 does not use LOCK prefixed instructions.
>
but cas does use lock prefix. i think we can use cas to provide
atomic loads just like we do on ARM? is that what you have in mind
when saying we can fix the library?

[dvyukov, 2012-12-24 10:05:18]

On Mon, Dec 24, 2012 at 1:37 PM, minux <minux.ma@gmail.com> wrote:
>
> On Monday, December 24, 2012, Dmitry Vyukov wrote:
>>
>> On Mon, Dec 24, 2012 at 1:23 PM, minux <minux.ma@gmail.com> wrote:
>> > On Monday, December 24, 2012, Dmitry Vyukov wrote:
>> >> 64-bit atomic load/store are not atomic on 386/amd64 (when unaligned).
>> >> That was the original issue than I hit with GC on 386 with
>> >> GOMAXPROCS>1. So there is the same problem.
>> > i was confused by your saying "All LOCK prefixed operations are
>> > atomic regardless of alignment", so you mean the instruction itself
>> > is atomic whereas the memory access is not?
>> 64-bit atomic load on amd64/386 does not use LOCK prefixed instructions.
>
> but cas does use lock prefix. i think we can use cas to provide
> atomic loads just like we do on ARM? is that what you have in mind
> when saying we can fix the library?

Yes, that's what I meant. It can hundreds of times slower.

I think on ARM one needs just LDX, not the whole CAS. However I
understand that there are issues with portability.

[rsc, 2013-01-02 20:41:48]

I added the global variable note and removed the ARM 64-bit note.
Will submit now. We can talk about relaxing the constraint in a followup
if there is a compelling reason, but we'd need to believe that it will be
possible to relax on every architecture, present and future.

[rsc, 2013-01-02 20:44:04]

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

sync/atomic: document that users must deal with 64-bit alignment

Update issue 599.

R=dvyukov, iant, minux.ma
CC=golang-dev
https://codereview.appspot.com/7001056