code review 132910043: runtime: convert netpoll to Go

runtime: convert netpoll to Go
The common code is converted, epoll and kqueue are converted.
Windows and solaris are still C.

[dvyukov, 2014-08-26 17:26:49]

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

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

[rsc, 2014-08-26 19:46:33]

i have a pending change that is going to mean no one has to edit any more
assembly during this conversion. please wait for that.

[dave_cheney.net, 2014-08-27 04:57:36]

https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll.go
File src/pkg/runtime/netpoll.go (right):

https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:57: gothrow("runtime_pollOpen: blocked write on free
descriptor")
runtime_netpollOpen

https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:60: gothrow("runtime_pollOpen: blocked read on free
descriptor")
same

https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:80: gothrow("runtime_pollClose: blocked write on
closing descriptor")
runtime_netpollClose

https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:99: pdSize := unsafe.Sizeof(polldesc{})
can this be 

const pdSize = unsafe.Sizeof(polldesc{})

[dvyukov, 2014-08-28 19:42:40]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com, iant@golang.org, khr@golang.org,
rsc@golang.org),

Please take another look.

[dvyukov, 2014-08-28 19:43:17]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com, iant@golang.org, khr@golang.org,
rsc@golang.org),

Please take another look.

[dvyukov, 2014-08-28 19:46:31]

it all must be done
PTAL

On Wed, Aug 27, 2014 at 8:57 AM,  <dave@cheney.net> wrote:
>
>
https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll.go
> File src/pkg/runtime/netpoll.go (right):
>
>
https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
> src/pkg/runtime/netpoll.go:57: gothrow("runtime_pollOpen: blocked write
> on free descriptor")
> runtime_netpollOpen
>
>
https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
> src/pkg/runtime/netpoll.go:60: gothrow("runtime_pollOpen: blocked read
> on free descriptor")
> same
>
>
https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
> src/pkg/runtime/netpoll.go:80: gothrow("runtime_pollClose: blocked write
> on closing descriptor")
> runtime_netpollClose
>
>
https://codereview.appspot.com/132910043/diff/140001/src/pkg/runtime/netpoll....
> src/pkg/runtime/netpoll.go:99: pdSize := unsafe.Sizeof(polldesc{})
> can this be
>
> const pdSize = unsafe.Sizeof(polldesc{})
>
> https://codereview.appspot.com/132910043/

[rsc, 2014-08-29 18:18:48]

PLEASE PLEASE PLEASE think about what the diffs are going to look like when you
are doing these.

- do NOT rename things unnecessarily
- do NOT shuffle pieces of the file around unnecessarily

the side-by-side diffs here are nearly useless because of this.

i will review it anyway but please do not make it so hard next time.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/net/fd_poll_runt...
File src/pkg/net/fd_poll_runtime.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/net/fd_poll_runt...
src/pkg/net/fd_poll_runtime.go:25: func runtime_pollEvict(ctx uintptr)
this can stay but PLEASE PLEASE PLEASE do not mix cleanup into translations.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll.go
File src/pkg/runtime/netpoll.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:50: // netpollReset, netpollWait, netpollWaitCanceled
and netpollReady (IO rediness notification)
'readiness'. or just 'ready'

[rsc, 2014-08-29 18:53:11]

I take it back: I did not review netpoll.go.
Please reorder things to make the side-by-side diffs smaller.
('hg upload' and check the diffs before sending it out again.)

I am going to arrange for the constants and structs you need
to be defined automatically. Please remove them from this CL.
The constants will be _OriginalName.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll.go
File src/pkg/runtime/netpoll.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:48: // This fully covers seq, rt and wt variables.
Unbreak this line and the next.
In general do not reformat things that don't need reformatting.
They are the anchors that the side-by-side diffs latch onto and make these code
reviews readable.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:395: type pollCache struct {
Move this up where it was, so it doesn't break the diff.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_epoll.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:58: memmove(unsafe.Pointer(&ev.data),
unsafe.Pointer(&pd), unsafe.Sizeof(pd))
If you're calling memmove, you're doing something wrong.

ev.data = uint64(uintptr(unsafe.Pointer(pd)))

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:68: }
gothrow("unused")

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:77: if block {
The old code did if !block.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:82: for {
Please put the retry: label back and use goto at the bottom.
Go will allow it, and it will make the diffs smaller. 
We can do cleanup later.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:91: continue
This is supposed to be in the } on the previous line.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:92: for i := int32(0); i < n; i++ {
Why is i an int32 here? Why not an int?

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:107: rg, wg := netpollReady(pd, mode)
What is all this? It's not in the original.
The original just says

netpollReady(&gp, ev.data, mode)

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:123: return
return gp

this function is too long to use an implicit return value

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_kqueue.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_kqueue.go:25: type keventstruct struct {
This and timespec do not belong here. They are system-dependent. I am sure you
know this. Instead of cutting corners like this and leaving them to find in the
code review, please bring it up ahead of time so we can find a better solution.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_kqueue.go:86: }
gothrow("unused")

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_kqueue.go:99: const nEvent = 64
this isn't used enough to make it a constant.
var events [64]keventstruct
and then use len(events) below

same in netpoll_epoll.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_kqueue.go:101: for {
please use label+goto to match original code

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_kqueue.go:110: for i := int32(0); i < n; i++ {
why is this an int32?

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_kqueue.go:120: rg, wg :=
netpollReady((*pollDesc)(ev.udata), mode)
what is this?

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_nacl.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_nacl.go:21: func netpollArm(pd *pollDesc, mode int) {
Delete.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_windows.c (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_windows.c:87: }
add throw back

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
File src/pkg/runtime/sys_linux_386.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
src/pkg/runtime/sys_linux_386.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
Delete all these new files.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (left):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:12: // If this struct changes, adjust
../time/sleep.go:/runtimeTimer and netpoll.goc:/timer.
Yay!

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:25: seq    uintptr
This seems unrelated. Remove.

[dvyukov, 2014-08-30 12:27:39]

I fixed diffs as much as I can, hg uploaded and checked the diffs in rietveld.
More pieces match so that old and new code is on the same screen.
Please take another look.

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_epoll.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:58: memmove(unsafe.Pointer(&ev.data),
unsafe.Pointer(&pd), unsafe.Sizeof(pd))
On 2014/08/29 18:53:10, rsc wrote:
> If you're calling memmove, you're doing something wrong.
> 
> ev.data = uint64(uintptr(unsafe.Pointer(pd)))

data is [8]byte, unaligned for uintptr
C compiler should have been recognized the unaligned uintptr field and emit
proper code for unaligned access on target arch
I doubt that Go compiler does that (there is no legal way to create unaligned
fields), so I have to use memmove (a smart compiler will replace such memove's
back with single move instruction on architectures where it is safe)

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:92: for i := int32(0); i < n; i++ {
On 2014/08/29 18:53:09, rsc wrote:
> Why is i an int32 here? Why not an int?

because n is int32, and it comes from the syscall
so I need a cast in one place or in another

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:107: rg, wg := netpollReady(pd, mode)
this is https://code.google.com/p/go/issues/detail?id=7710
the original approach does not compile in Go

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_kqueue.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_kqueue.go:120: rg, wg :=
netpollReady((*pollDesc)(ev.udata), mode)
On 2014/08/29 18:53:10, rsc wrote:
> what is this?

this is https://code.google.com/p/go/issues/detail?id=7710

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_nacl.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_nacl.go:21: func netpollArm(pd *pollDesc, mode int) {
On 2014/08/29 18:53:10, rsc wrote:
> Delete.

The code does not build w/o it anymore. I guess C compiler did not manage to
check that a function exists if it is not going to emit call. But Go compiler
says:
pkg/runtime/netpoll.go:154: undefined: netpollarm

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
File src/pkg/runtime/sys_linux_386.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
src/pkg/runtime/sys_linux_386.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
On 2014/08/29 18:53:11, rsc wrote:
> Delete all these new files.

zruntime_defs contains incorrect definition of this struct.

#pragma pack on
struct EpollEvent {
	uint32	events;
	uint64	data;
};
#pragma pack off

is not

type epollevent struct {
        events  uint32
        data    uint64
}

it is

type epollEvent struct {
         events uint32
         data   [8]byte
}

so I still need this (unless you fix the tool)

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:25: seq    uintptr
On 2014/08/29 18:53:11, rsc wrote:
> This seems unrelated. Remove.

This is required for Go netpoll. Previously I stored PollDesc* into arg.data,
and seq into arg.type. Now *pollDesc occupies whole arg.

[rsc, 2014-08-30 17:40:19]

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
File src/pkg/runtime/sys_linux_386.go (right):

https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
src/pkg/runtime/sys_linux_386.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
You asked me about this and I already explained you what needed to be done: the
C structs need to be written so they do not use #pragma pack. Then the
conversion will be fine. I will make the C compiler complain if #pragma pack is
not a no-op.

[dvyukov, 2014-08-30 18:06:01]

On Sat, Aug 30, 2014 at 9:40 PM,  <rsc@golang.org> wrote:
>
>
https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
> File src/pkg/runtime/sys_linux_386.go (right):
>
>
https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/sys_linu...
> src/pkg/runtime/sys_linux_386.go:1: // Copyright 2014 The Go Authors.
> All rights reserved.
> You asked me about this and I already explained you what needed to be
> done: the C structs need to be written so they do not use #pragma pack.
> Then the conversion will be fine. I will make the C compiler complain if
> #pragma pack is not a no-op.

These structs are not written by hand, they are generated by cgo.

[rsc, 2014-08-30 18:09:38]

On Sat, Aug 30, 2014 at 2:05 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> These structs are not written by hand, they are generated by cgo.
>

They're checked in. We can edit them for now. We'll never run cgo to
generate C again.

[dvyukov, 2014-08-30 18:48:46]

On 2014/08/30 18:09:38, rsc wrote:
> On Sat, Aug 30, 2014 at 2:05 PM, Dmitry Vyukov <mailto:dvyukov@google.com>
wrote:
> 
> > These structs are not written by hand, they are generated by cgo.
> >
> 
> They're checked in. We can edit them for now. We'll never run cgo to
> generate C again.

DONE
PTAL

[rsc, 2014-09-02 03:46:07]

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll.go
File src/pkg/runtime/netpoll.go (right):

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:31: func pdReady() *g {
Don't do this. If the garbage collector ever sees this pointer it will blow up.
Instead, use uintptrs

const (
    pdReady uintptr = 1
    pdWait uintptr = 2
)

Then you have to make the rg and wg fields type uintptr.
The test in netpollOpen becomes 'pd.wg != 0 && pd.wg != pdReady' and so on.
When you want to actually use one of the *g fields, then you have to convert it,
as in (*g)(unsafe.Pointer(pd.wg)).

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:141: if theGoos == "solaris" {
s/theGoos/GOOS/
use the exported constant instead of the implementation detail

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:281: func netpollready(rg, wg **g, pd *pollDesc, mode
int) {
Please make this function look like it used to.
If at the call site you get an error about &gp escaping, use noescape at the
call site.
Keep the code looking like it did before.

We are skating on thin ice doing all this so late. The translation absolutely
must follow the originals line by line as far as possible.

For example, at the call site:
netpollready((**g)(noescape(unsafe.Pointer(&gp))), pd, mode)

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:301: return caspd((**g)(gpp), pdWait(), gp)
gp should be a *uintptr, and this should be casuintptr.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:322: if caspd(gpp, nil, pdWait()) {
casuintptr

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:419: func caspd(gpp **g, old *g, new *g) bool {
Delete.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_epoll.go (right):

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:43: memmove(unsafe.Pointer(&ev.data),
unsafe.Pointer(&pd), unsafe.Sizeof(pd))
*(**pollDesc)(unsafe.Pointer(&ev.data)) = pd

basically, never call memmove. it means something is wrong.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:44: err := epollctl(epfd, _EPOLL_CTL_ADD,
int32(fd), &ev)
return -epollctl(epfd, _EPOLL_CTL_ADD< int32(fd), &ev)

That's what the old code says, and epollctl hasn't changed, so the new code
should say the same thing.
If you think the code is wrong, great, but make that change in a separate CL.
It is hard enough to make sure that the translation is accurate. Don't mix in
possible semantic changes.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:53: err := epollctl(epfd, _EPOLL_CTL_DEL,
int32(fd), &ev)
same: return -epollctl...

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:98: memmove(unsafe.Pointer(&pd),
unsafe.Pointer(&ev.data), unsafe.Sizeof(pd))
pd := *(**pollDesc)(unsafe.Pointer(&ev.data))

[dvyukov, 2014-09-02 06:55:35]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com, iant@golang.org, khr@golang.org,
rsc@golang.org),

Please take another look.

[dvyukov, 2014-09-02 06:56:06]

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll.go
File src/pkg/runtime/netpoll.go (right):

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:31: func pdReady() *g {
On 2014/09/02 03:46:06, rsc wrote:
> Don't do this. If the garbage collector ever sees this pointer it will blow
up.
> Instead, use uintptrs
> 
> const (
>     pdReady uintptr = 1
>     pdWait uintptr = 2
> )
> 
> Then you have to make the rg and wg fields type uintptr.
> The test in netpollOpen becomes 'pd.wg != 0 && pd.wg != pdReady' and so on.
> When you want to actually use one of the *g fields, then you have to convert
it,
> as in (*g)(unsafe.Pointer(pd.wg)).

Done.

Do I need to rename them to _READY/_WAIT?

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:141: if theGoos == "solaris" {
On 2014/09/02 03:46:06, rsc wrote:
> s/theGoos/GOOS/
> use the exported constant instead of the implementation detail

Done.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:281: func netpollready(rg, wg **g, pd *pollDesc, mode
int) {
On 2014/09/02 03:46:06, rsc wrote:
> Please make this function look like it used to.
> If at the call site you get an error about &gp escaping, use noescape at the
> call site.
> Keep the code looking like it did before.
> 
> We are skating on thin ice doing all this so late. The translation absolutely
> must follow the originals line by line as far as possible.
> 
> For example, at the call site:
> netpollready((**g)(noescape(unsafe.Pointer(&gp))), pd, mode)

Done.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:301: return caspd((**g)(gpp), pdWait(), gp)
On 2014/09/02 03:46:06, rsc wrote:
> gp should be a *uintptr, and this should be casuintptr.

Done.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:322: if caspd(gpp, nil, pdWait()) {
On 2014/09/02 03:46:07, rsc wrote:
> casuintptr

Done.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:419: func caspd(gpp **g, old *g, new *g) bool {
On 2014/09/02 03:46:07, rsc wrote:
> Delete.

Done.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_epoll.go (right):

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:43: memmove(unsafe.Pointer(&ev.data),
unsafe.Pointer(&pd), unsafe.Sizeof(pd))
On 2014/09/02 03:46:07, rsc wrote:
> *(**pollDesc)(unsafe.Pointer(&ev.data)) = pd
> 
> basically, never call memmove. it means something is wrong.

I've already answered this one.
What you propose does not respect alignment requirements. You blamed me for not
respecting endianess in GC code, now you incline me to not respect alignment. I
do not understand the system. Unaligned accesses generally crash or are
extremely slow due to OS fixup.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:44: err := epollctl(epfd, _EPOLL_CTL_ADD,
int32(fd), &ev)
On 2014/09/02 03:46:07, rsc wrote:
> return -epollctl(epfd, _EPOLL_CTL_ADD< int32(fd), &ev)
> 
> That's what the old code says, and epollctl hasn't changed, so the new code
> should say the same thing.
> If you think the code is wrong, great, but make that change in a separate CL.
> It is hard enough to make sure that the translation is accurate. Don't mix in
> possible semantic changes.

Done.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:53: err := epollctl(epfd, _EPOLL_CTL_DEL,
int32(fd), &ev)
On 2014/09/02 03:46:07, rsc wrote:
> same: return -epollctl...

Done.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:98: memmove(unsafe.Pointer(&pd),
unsafe.Pointer(&ev.data), unsafe.Sizeof(pd))
On 2014/09/02 03:46:07, rsc wrote:
> pd := *(**pollDesc)(unsafe.Pointer(&ev.data))

see above

[rsc, 2014-09-03 17:01:47]

LGTM once memmove is gone

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll.go
File src/pkg/runtime/netpoll.go (right):

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:31: func pdReady() *g {
On 2014/09/02 06:56:06, dvyukov wrote:
> On 2014/09/02 03:46:06, rsc wrote:
> > Don't do this. If the garbage collector ever sees this pointer it will blow
> up.
> > Instead, use uintptrs
> > 
> > const (
> >     pdReady uintptr = 1
> >     pdWait uintptr = 2
> > )
> > 
> > Then you have to make the rg and wg fields type uintptr.
> > The test in netpollOpen becomes 'pd.wg != 0 && pd.wg != pdReady' and so on.
> > When you want to actually use one of the *g fields, then you have to convert
> it,
> > as in (*g)(unsafe.Pointer(pd.wg)).
> 
> Done.
> 
> Do I need to rename them to _READY/_WAIT?

I don't mind writing Go constants using Go-like names.
What I don't want to do yet is start changing C code.
Since this is Go-only, pdReady/pdWait are fine as names.
And since there is no file-scoping anymore, the pd prefix is useful.

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_epoll.go (right):

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:43: memmove(unsafe.Pointer(&ev.data),
unsafe.Pointer(&pd), unsafe.Sizeof(pd))
On 2014/09/02 06:56:06, dvyukov wrote:
> On 2014/09/02 03:46:07, rsc wrote:
> > *(**pollDesc)(unsafe.Pointer(&ev.data)) = pd
> > 
> > basically, never call memmove. it means something is wrong.
> 
> I've already answered this one.
> What you propose does not respect alignment requirements. You blamed me for
not
> respecting endianess in GC code, now you incline me to not respect alignment.
I
> do not understand the system. Unaligned accesses generally crash or are
> extremely slow due to OS fixup.

The only system with a misaligned struct here is linux/amd64: the 64-bit pointer
is only 32-bit aligned. The x86 has no problem executing that, and the OS does
not get involved. I agree that the write will be slower than an aligned write,
but the single MOVQ instruction is still certainly faster than invoking memmove
and all its more complex machinery.

Please change it to stop using memmove. The real reason is not performance but
that memmove is too unsafe. We are going to have to go through and vet every
memmove to find moves of pointers and such. The fewer memmoves in the source
tree, the better.

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_386....
src/pkg/runtime/asm_386.s:588: JMP	runtime·xchgp(SB)
s/xchgp/xchg/ please
i am trying to make sure that pointer code only deals with pointers. since this
is the integer variant, it should jump to the integer equivalent. i know that
there are still other places that confuse pointers and integers, i'm just trying
to avoid introducing new ones.

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_amd6...
src/pkg/runtime/asm_amd64.s:699: JMP	runtime·xchgp(SB)
xchg64

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_amd6...
File src/pkg/runtime/asm_amd64p32.s (right):

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_amd6...
src/pkg/runtime/asm_amd64p32.s:663: JMP	runtime·xchgp(SB)
xchg

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/atomic_a...
File src/pkg/runtime/atomic_arm.c (right):

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/atomic_a...
src/pkg/runtime/atomic_arm.c:61: return runtime·xchgp((uintptr*)addr,
(uintptr)v);
s/xchgp/xchg/
s/uintptr/uint32/

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/netpoll.go
File src/pkg/runtime/netpoll.go (right):

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.go:48: // that will blow up when GC starts moving
objects.
Or we could just not allow moving g's. That seems okay.

[dvyukov, 2014-09-04 06:02:44]

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
File src/pkg/runtime/netpoll_epoll.go (right):

https://codereview.appspot.com/132910043/diff/420001/src/pkg/runtime/netpoll_...
src/pkg/runtime/netpoll_epoll.go:43: memmove(unsafe.Pointer(&ev.data),
unsafe.Pointer(&pd), unsafe.Sizeof(pd))
On 2014/09/03 17:01:46, rsc wrote:
> On 2014/09/02 06:56:06, dvyukov wrote:
> > On 2014/09/02 03:46:07, rsc wrote:
> > > *(**pollDesc)(unsafe.Pointer(&ev.data)) = pd
> > > 
> > > basically, never call memmove. it means something is wrong.
> > 
> > I've already answered this one.
> > What you propose does not respect alignment requirements. You blamed me for
> not
> > respecting endianess in GC code, now you incline me to not respect
alignment.
> I
> > do not understand the system. Unaligned accesses generally crash or are
> > extremely slow due to OS fixup.
> 
> The only system with a misaligned struct here is linux/amd64: the 64-bit
pointer
> is only 32-bit aligned. The x86 has no problem executing that, and the OS does
> not get involved. I agree that the write will be slower than an aligned write,
> but the single MOVQ instruction is still certainly faster than invoking
memmove
> and all its more complex machinery.
> 
> Please change it to stop using memmove. The real reason is not performance but
> that memmove is too unsafe. We are going to have to go through and vet every
> memmove to find moves of pointers and such. The fewer memmoves in the source
> tree, the better.

Done.

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_386....
src/pkg/runtime/asm_386.s:588: JMP	runtime·xchgp(SB)
On 2014/09/03 17:01:46, rsc wrote:
> s/xchgp/xchg/ please
> i am trying to make sure that pointer code only deals with pointers. since
this
> is the integer variant, it should jump to the integer equivalent. i know that
> there are still other places that confuse pointers and integers, i'm just
trying
> to avoid introducing new ones.
> 

Done.

https://codereview.appspot.com/132910043/diff/480001/src/pkg/runtime/asm_386....
src/pkg/runtime/asm_386.s:588: JMP	runtime·xchgp(SB)
On 2014/09/03 17:01:46, rsc wrote:
> s/xchgp/xchg/ please
> i am trying to make sure that pointer code only deals with pointers. since
this
> is the integer variant, it should jump to the integer equivalent. i know that
> there are still other places that confuse pointers and integers, i'm just
trying
> to avoid introducing new ones.
> 

Done.

[dvyukov, 2014-09-04 06:04:13]

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

runtime: convert netpoll to Go
The common code is converted, epoll and kqueue are converted.
Windows and solaris are still C.

LGTM=rsc
R=golang-codereviews, rsc, dave
CC=golang-codereviews, iant, khr, rsc
https://codereview.appspot.com/132910043

[gobot, 2014-09-04 06:06:42]

This CL appears to have broken the android-arm-crawshaw builder.
See http://build.golang.org/log/7e2ab96e76b8eb0cff1eb7c778a0089b4d3d69c3

[rsc, 2014-09-04 06:14:13]

arm build failure is real; please fix. (GOOS=linux GOARCH=arm make.bash
will reproduce it on a non-arm system.)