Grokbase › Groups › Go › golang-dev › November 2012
FAQ
Reviewers: rsc, mikio, dvyukov, minux,

Message:
Hello rsc@golang.org, mikioh.mikioh@gmail.com, dvyukov@google.com,
minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

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


Description:
undo CL 6855110 / 869253ef7009

64bit atomics are broken on 32bit systems. This is issue 599.

linux/arm builders all broke with this change, I am concerned that the
other 32bit builders are silently impacted.

««« original CL description
net: fix data races on deadline vars

Fixes issue 4434.

R=mikioh.mikioh, bradfitz, dvyukov, alex.brainman
CC=golang-dev
https://codereview.appspot.com/6855110
»»»


Please review this at https://codereview.appspot.com/6852105/

Affected files:
R src/pkg/net/fd_posix_test.go
M src/pkg/net/fd_unix.go
M src/pkg/net/fd_windows.go
M src/pkg/net/sendfile_freebsd.go
M src/pkg/net/sendfile_linux.go
M src/pkg/net/sock_posix.go
M src/pkg/net/sockopt_posix.go

Search Discussions

  • Dave at Nov 30, 2012 at 9:02 am ⇧
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=697f36fec52c ***

    undo CL 6855110 / 869253ef7009

    64bit atomics are broken on 32bit systems. This is issue 599.

    linux/arm builders all broke with this change, I am concerned that the
    other 32bit builders are silently impacted.

    ««« original CL description
    net: fix data races on deadline vars

    Fixes issue 4434.

    R=mikioh.mikioh, bradfitz, dvyukov, alex.brainman
    CC=golang-dev
    https://codereview.appspot.com/6855110
    »»»

    R=rsc, mikioh.mikioh, dvyukov, minux.ma
    CC=golang-dev
    https://codereview.appspot.com/6852105


    https://codereview.appspot.com/6852105/
  • Mikioh Mikioh at Nov 30, 2012 at 9:07 am ⇧
    LGTM

    https://codereview.appspot.com/6852105/
  • Minux at Nov 30, 2012 at 9:06 am ⇧
    if only the alignment of two deadline variables is the issue,
    maybe you can manually adjust the alignment by introduce
    a blank field before them and some comments.

    then file an issue to remind we remove the field when the
    64-bit alignment issue is resolved.
  • Dave Cheney at Nov 30, 2012 at 9:12 am ⇧
    I don't think that will work, malloc will only align the struct to a 4
    byte bounday.
    On Fri, Nov 30, 2012 at 8:06 PM, minux wrote:
    if only the alignment of two deadline variables is the issue,
    maybe you can manually adjust the alignment by introduce
    a blank field before them and some comments.

    then file an issue to remind we remove the field when the
    64-bit alignment issue is resolved.
  • Minux at Nov 30, 2012 at 9:19 am ⇧

    On Friday, November 30, 2012, Dave Cheney wrote:

    I don't think that will work, malloc will only align the struct to a 4
    byte bounday.
    if we pad the whole structure to 8-byte boundary and make the overall size
    fall into one of the size classes, then i think it will work.
    On Fri, Nov 30, 2012 at 8:06 PM, minux <minux.ma@gmail.com <javascript:;>>
    wrote:
    if only the alignment of two deadline variables is the issue,
    maybe you can manually adjust the alignment by introduce
    a blank field before them and some comments.

    then file an issue to remind we remove the field when the
    64-bit alignment issue is resolved.
  • Minux at Nov 30, 2012 at 9:24 am ⇧

    On Friday, November 30, 2012, Dave Cheney wrote:

    I don't think that will work, malloc will only align the struct to a 4
    byte bounday.
    after re-read the comments in pkg/runtime/msize.c, i think
    the allocator will always provide at least a 8-byte alignment.
  • Dave Cheney at Nov 30, 2012 at 9:36 am ⇧
    If that is the case, then moving the deadline fields to the top of the
    struct would solve the problem for the moment.

    I am testing now.
    On Fri, Nov 30, 2012 at 8:24 PM, minux wrote:
    On Friday, November 30, 2012, Dave Cheney wrote:

    I don't think that will work, malloc will only align the struct to a 4
    byte bounday.
    after re-read the comments in pkg/runtime/msize.c, i think
    the allocator will always provide at least a 8-byte alignment.
  • Dvyukov at Nov 30, 2012 at 9:26 am ⇧

    On 2012/11/30 09:02:49, dfc wrote:
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=697f36fec52c ***
    undo CL 6855110 / 869253ef7009
    64bit atomics are broken on 32bit systems. This is issue 599.
    linux/arm builders all broke with this change, I am concerned that the other
    32bit builders are silently impacted.
    ««« original CL description
    net: fix data races on deadline vars
    Fixes issue 4434.
    R=mikioh.mikioh, bradfitz, dvyukov, alex.brainman
    CC=golang-dev
    https://codereview.appspot.com/6855110
    »»»
    R=rsc, mikioh.mikioh, dvyukov, minux.ma
    CC=golang-dev
    https://codereview.appspot.com/6852105
    The issue 599 causes non-atomicity of 64-bit atomics on 386. It leads to
    crashes only if you you heavily stress atomicity. I do not think that it
    should causes instant crashes in this case. Moreover, ARM seems to be
    using STREX/LDREX for 64-bit atomic loads/stores, so most likely it does
    not even have the problem.
    Perhaps ARM builders are affected by something else.


    https://codereview.appspot.com/6852105/
  • Minux at Nov 30, 2012 at 9:36 am ⇧

    On Friday, November 30, 2012, dvyukov wrote:
    The issue 599 causes non-atomicity of 64-bit atomics on 386. It leads to
    crashes only if you you heavily stress atomicity. I do not think that it
    should causes instant crashes in this case. Moreover, ARM seems to be
    using STREX/LDREX for 64-bit atomic loads/stores, so most likely it does
    not even have the problem.
    they uses ldrexd and strexd.
    and ldrexd/strexd both require 64-bit data alignment.
    Perhaps ARM builders are affected by something else.
  • Dave Cheney at Nov 30, 2012 at 9:50 am ⇧
    But on arm5 we defer all those operations to the kernel.

    And indeed, rev 869253ef7009 passes on arm5.

    Moving rdeadline, wdeadline to the top of the netFD struct fixed the panicing.
    On Fri, Nov 30, 2012 at 8:36 PM, minux wrote:
    On Friday, November 30, 2012, dvyukov wrote:

    The issue 599 causes non-atomicity of 64-bit atomics on 386. It leads to
    crashes only if you you heavily stress atomicity. I do not think that it
    should causes instant crashes in this case. Moreover, ARM seems to be
    using STREX/LDREX for 64-bit atomic loads/stores, so most likely it does
    not even have the problem.
    they uses ldrexd and strexd.
    and ldrexd/strexd both require 64-bit data alignment.
    Perhaps ARM builders are affected by something else.
  • Minux at Nov 30, 2012 at 4:08 pm ⇧

    On Fri, Nov 30, 2012 at 5:50 PM, Dave Cheney wrote:

    But on arm5 we defer all those operations to the kernel.
    we can use __kuser_cmpxchg64 on linux version 3.1 and up.
    for the rest of arm v5 systems we use runtime's lock based emulation
    which doesn't care about 64-bit alignment.
    Moving rdeadline, wdeadline to the top of the netFD struct fixed the
    panicing.
    good. do you plan to revert this revert?
  • Dave Cheney at Nov 30, 2012 at 7:19 pm ⇧

    we can use __kuser_cmpxchg64 on linux version 3.1 and up.
    for the rest of arm v5 systems we use runtime's lock based emulation
    which doesn't care about 64-bit alignment.
    Can we detect that with runtime.auxv, or do we have to spelunk in
    /proc for the kernel version ?
    Moving rdeadline, wdeadline to the top of the netFD struct fixed the
    panicing.
    good. do you plan to revert this revert?
    I'd like to hear from rsc. I'm concerned there are places where
    netFD's could still be misaligned, ie []netFD, or if it were hoisted
    onto the stack via escape analysis.
  • Dave Cheney at Dec 1, 2012 at 1:59 am ⇧
    Here is a proposed workaround.

    https://codereview.appspot.com/6842127
    On Sat, Dec 1, 2012 at 6:19 AM, Dave Cheney wrote:
    we can use __kuser_cmpxchg64 on linux version 3.1 and up.
    for the rest of arm v5 systems we use runtime's lock based emulation
    which doesn't care about 64-bit alignment.
    Can we detect that with runtime.auxv, or do we have to spelunk in
    /proc for the kernel version ?
    Moving rdeadline, wdeadline to the top of the netFD struct fixed the
    panicing.
    good. do you plan to revert this revert?
    I'd like to hear from rsc. I'm concerned there are places where
    netFD's could still be misaligned, ie []netFD, or if it were hoisted
    onto the stack via escape analysis.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 30, '12 at 8:56a
activeDec 1, '12 at 1:59a
posts14
users4
websitegolang.org

4 users in discussion

Dave Cheney: 7 posts Minux: 5 posts Dvyukov: 1 post Mikioh Mikioh: 1 post

Content

People

Support

Translate

site design / logo © 2022 Grokbase