FAQ
Reviewers: bradfitz, minux, rsc,

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

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


Description:
net: align deadline fields on 8 byte boundaries

This is a workaround for issue 599. Assuming the test in
fd_unix_test.go passes, then 64bit atomics should be safe
to use on the deadline fields, nee CL 6855110.

sizeof netFD on 386 is 112 bytes.

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

Affected files:
M src/pkg/net/fd_unix.go
M src/pkg/net/fd_unix_test.go


Index: src/pkg/net/fd_unix.go
===================================================================
--- a/src/pkg/net/fd_unix.go
+++ b/src/pkg/net/fd_unix.go
@@ -17,6 +17,20 @@

// Network file descriptor.
type netFD struct {
+ // 64 bit atomic operations require the value to be aligned on an
+ // 8 byte boundary. Placing these fields at the top of the struct
+ // guarantees they will be aligned so long as the heap allocator
+ // aligns to 8 byte boundaries. The length of the struct must
+ // also be a multiple of 8 to ensure that an array of netFD
+ // structs is correctly aligned. A test in fd_unix_test.go ensures
+ // this holds true on all architectures.
+ //
+ // TODO(dfc) move these fields back to their original position
+ // when 599 is fixed.
+
+ // read and write deadlines
+ rdeadline, wdeadline int64 // nsec since 1970, 0 if not set
+
// locking/lifetime of sysfd
sysmu sync.Mutex
sysref int
@@ -37,11 +51,8 @@
laddr Addr
raddr Addr

- // owned by client
- rdeadline int64
- rio sync.Mutex
- wdeadline int64
- wio sync.Mutex
+ // serialize access to Read and Write methods
+ rio, wio sync.Mutex

// owned by fd wait server
ncr, ncw int
Index: src/pkg/net/fd_unix_test.go
===================================================================
--- a/src/pkg/net/fd_unix_test.go
+++ b/src/pkg/net/fd_unix_test.go
@@ -10,6 +10,7 @@
"io"
"syscall"
"testing"
+ "unsafe"
)

// Issue 3590. netFd.AddFD should return an error
@@ -104,3 +105,17 @@
}
}
}
+
+// see comment in fd_unix.go
+func TestDeadlineFieldAlignment(t *testing.T) {
+ var fd netFD
+ if size := unsafe.Sizeof(fd) % 8; size != 0 {
+ t.Errorf("size of fd %% 8 is %d not 0", size)
+ }
+ if offset := unsafe.Offsetof(fd.rdeadline) % 8; offset != 0 {
+ t.Errorf("offset of fd.rdeadline %% 8 is %d not 0", offset)
+ }
+ if offset := unsafe.Offsetof(fd.wdeadline) % 8; offset != 0 {
+ t.Errorf("offset of fd.wdeadline %% 8 is %d not 0", offset)
+ }
+}

Search Discussions

  • Brad Fitzpatrick at Dec 1, 2012 at 6:36 am
    LGTM
    On Fri, Nov 30, 2012 at 10:31 PM, wrote:

    Reviewers: bradfitz, minux, rsc,

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

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


    Description:
    net: align deadline fields on 8 byte boundaries

    This is a workaround for issue 599. Assuming the test in
    fd_unix_test.go passes, then 64bit atomics should be safe
    to use on the deadline fields, nee CL 6855110.

    sizeof netFD on 386 is 112 bytes.

    Please review this at https://codereview.appspot.**com/6842127/<https://codereview.appspot.com/6842127/>

    Affected files:
    M src/pkg/net/fd_unix.go
    M src/pkg/net/fd_unix_test.go


    Index: src/pkg/net/fd_unix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/fd_unix.go
    +++ b/src/pkg/net/fd_unix.go
    @@ -17,6 +17,20 @@

    // Network file descriptor.
    type netFD struct {
    + // 64 bit atomic operations require the value to be aligned on an
    + // 8 byte boundary. Placing these fields at the top of the struct
    + // guarantees they will be aligned so long as the heap allocator
    + // aligns to 8 byte boundaries. The length of the struct must
    + // also be a multiple of 8 to ensure that an array of netFD
    + // structs is correctly aligned. A test in fd_unix_test.go ensures
    + // this holds true on all architectures.
    + //
    + // TODO(dfc) move these fields back to their original position
    + // when 599 is fixed.
    +
    + // read and write deadlines
    + rdeadline, wdeadline int64 // nsec since 1970, 0 if not set
    +
    // locking/lifetime of sysfd
    sysmu sync.Mutex
    sysref int
    @@ -37,11 +51,8 @@
    laddr Addr
    raddr Addr

    - // owned by client
    - rdeadline int64
    - rio sync.Mutex
    - wdeadline int64
    - wio sync.Mutex
    + // serialize access to Read and Write methods
    + rio, wio sync.Mutex

    // owned by fd wait server
    ncr, ncw int
    Index: src/pkg/net/fd_unix_test.go
    ==============================**==============================**=======
    --- a/src/pkg/net/fd_unix_test.go
    +++ b/src/pkg/net/fd_unix_test.go
    @@ -10,6 +10,7 @@
    "io"
    "syscall"
    "testing"
    + "unsafe"
    )

    // Issue 3590. netFd.AddFD should return an error
    @@ -104,3 +105,17 @@
    }
    }
    }
    +
    +// see comment in fd_unix.go
    +func TestDeadlineFieldAlignment(t *testing.T) {
    + var fd netFD
    + if size := unsafe.Sizeof(fd) % 8; size != 0 {
    + t.Errorf("size of fd %% 8 is %d not 0", size)
    + }
    + if offset := unsafe.Offsetof(fd.rdeadline) % 8; offset != 0 {
    + t.Errorf("offset of fd.rdeadline %% 8 is %d not 0", offset)
    + }
    + if offset := unsafe.Offsetof(fd.wdeadline) % 8; offset != 0 {
    + t.Errorf("offset of fd.wdeadline %% 8 is %d not 0", offset)
    + }
    +}

  • Dave at Dec 2, 2012 at 4:40 am
    Tested on arm6, sizeof netFD is 112 bytes (as expected).

    https://codereview.appspot.com/6842127/
  • Minux at Dec 2, 2012 at 9:32 am
    LGTM.
  • Rémy Oudompheng at Dec 2, 2012 at 9:37 am
    I'm worried by the fact that fixing the compiler is a smaller patch than
    the workaround.
  • Dave Cheney at Dec 2, 2012 at 9:47 am
    I am also concerned about this fix, and am open to alternative suggestions.

    On Sun, Dec 2, 2012 at 8:37 PM, Rémy Oudompheng
    wrote:
    I'm worried by the fact that fixing the compiler is a smaller patch than the
    workaround.
  • Daniel Morsing at Dec 2, 2012 at 9:48 am

    On Sun, Dec 2, 2012 at 10:37 AM, Rémy Oudompheng wrote:
    I'm worried by the fact that fixing the compiler is a smaller patch than the
    workaround.
    The compiler fix is trivial. The hard part is making the runtime
    return memory/stacks that are 8 byte aligned.
  • Minux at Dec 2, 2012 at 9:52 am

    On Sunday, December 2, 2012, Daniel Morsing wrote:

    On Sun, Dec 2, 2012 at 10:37 AM, Rémy Oudompheng
    <remyoudompheng@gmail.com <javascript:;>> wrote:
    I'm worried by the fact that fixing the compiler is a smaller patch than the
    workaround.
    The compiler fix is trivial. The hard part is making the runtime
    return memory/stacks that are 8 byte aligned.
    we don't need to worry about heap allocations,
    the stack issue is more serious.

    consider this:
    func f() uint64

    this will require the return value to be 8-byte aligned, the runtime
    doesn't expect this. we effectively requires sp to be always 8-byte
    aligned instead of 4.
  • Remyoudompheng at Dec 2, 2012 at 1:07 pm
    Fixing the whole toolchain seems short but hard, so LGTM

    https://codereview.appspot.com/6842127/
  • Dave Cheney at Dec 3, 2012 at 12:23 am
    Maybe it would just be simpler if I used a sync.Mutex. The cost should
    be comparable as they use cas under the hood, but the case on a
    machine word, which should be easier on 32 bit hosts.
    On Mon, Dec 3, 2012 at 12:07 AM, wrote:
    Fixing the whole toolchain seems short but hard, so LGTM

    https://codereview.appspot.com/6842127/
  • Brad Fitzpatrick at Dec 3, 2012 at 2:23 am
    That works too. Easy to measure.
    On Sun, Dec 2, 2012 at 4:23 PM, Dave Cheney wrote:

    Maybe it would just be simpler if I used a sync.Mutex. The cost should
    be comparable as they use cas under the hood, but the case on a
    machine word, which should be easier on 32 bit hosts.
    On Mon, Dec 3, 2012 at 12:07 AM, wrote:
    Fixing the whole toolchain seems short but hard, so LGTM

    https://codereview.appspot.com/6842127/
  • Russ Cox at Dec 3, 2012 at 4:44 pm
    Moving the fields is fine.

    If you believe that fixing the compiler is that easy, please do so. I
    started a while back and decided it was harder than it looked, but I
    could easily have been wrong.

    Russ
  • Minux at Dec 3, 2012 at 4:49 pm

    On Tue, Dec 4, 2012 at 12:44 AM, Russ Cox wrote:

    If you believe that fixing the compiler is that easy, please do so. I
    started a while back and decided it was harder than it looked, but I
    could easily have been wrong.
    Russ, do we align the stack pointer to any particular value now?
    I think the major changes is the stack alignment issues. fixing
    the compiler for struct alignment and heap allocation alignment (already
    done) shouldn't be hard IMO.

    or, perhaps, we can say when uint64's 8-byte alignment really is
    significant, the user shouldn't allocate the uint64 on stack anyway.
  • Russ Cox at Dec 3, 2012 at 5:19 pm
    The Go compiler and C compiler need to be in sync, and the struct and
    function call parameter layout need to be in sync.

    Russ

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 1, '12 at 6:31a
activeDec 3, '12 at 5:19p
posts14
users6
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase