Issue 138390043: code review 138390043: runtime: convert solaris netpoll to Go
Descriptionruntime: convert solaris netpoll to Go
One C file less.
This is the last of C netpolls, so all C support goes away as well.
Also revert onM calls in netpoll.go as they are not necessary (assuming that "139620043: runtime: convert windows netpoll to Go" is submitted).
Patch Set 1 #Patch Set 2 : diff -r d0331dfbc0539eae77723d8bc0bdb4c42a8b0c15 https://[email protected]@code.google.com/p/go/ #Patch Set 3 : diff -r d0331dfbc0539eae77723d8bc0bdb4c42a8b0c15 https://[email protected]@code.google.com/p/go/ #Patch Set 4 : diff -r d0331dfbc0539eae77723d8bc0bdb4c42a8b0c15 https://[email protected]@code.google.com/p/go/ #Patch Set 5 : diff -r d0331dfbc0539eae77723d8bc0bdb4c42a8b0c15 https://[email protected]@code.google.com/p/go/ #Patch Set 6 : diff -r d0331dfbc0539eae77723d8bc0bdb4c42a8b0c15 https://[email protected]@code.google.com/p/go/ #
Total comments: 17
Patch Set 7 : diff -r d0331dfbc0539eae77723d8bc0bdb4c42a8b0c15 https://[email protected]@code.google.com/p/go/ #Patch Set 8 : diff -r d0331dfbc0539eae77723d8bc0bdb4c42a8b0c15 https://[email protected]@code.google.com/p/go/ #
MessagesTotal messages: 14
Hello [email protected], [email protected], [email protected], [email protected] (cc: [email protected]), I'd like you to review this change to https://[email protected]@code.google.com/p/go/
Sign in to reply to this message.
I have not tested it, only did GOOS=solaris build. Aram, can you please test it on a solaris machine?
Sign in to reply to this message.
Thanks Dmitry, I'll test this today. On 13 Sep 2014 09:58, "dvyukov via golang-codereviews" < [email protected]> wrote: > I have not tested it, only did GOOS=solaris build. > Aram, can you please test it on a solaris machine? > > > https://codereview.appspot.com/138390043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews [email protected]. > For more options, visit https://groups.google.com/d/optout. >
Sign in to reply to this message.
On 2014/09/13 00:26:05, dfc wrote: > Thanks Dmitry, I'll test this today. Opps, I've hit one issue /home/dfc/go/src/runtime/netpoll_solaris.c:5 not a function /home/dfc/go/src/runtime/netpoll_solaris.c:5 syntax error, last name: runtime go tool dist: FAILED: /home/dfc/go/pkg/tool/solaris_amd64/6c -F -V -w -I $WORK -I /home/dfc/go/pkg/solaris_amd64 -D GOOS_solaris -D GOARCH_amd64 -D GOOS_GOARCH_solaris_amd64 -o $WORK/netpoll_solaris.o /home/dfc/go/src/runtime/netpoll_solaris.c I'll see if I can get around it and test this CL.
Sign in to reply to this message.
Sorry, this might be PEBCAK, my solaris install has an ancient version of mercurial which doesn't appear to do file renames properly; On Sat, Sep 13, 2014 at 12:09 PM, <[email protected]> wrote: > On 2014/09/13 00:26:05, dfc wrote: >> >> Thanks Dmitry, I'll test this today. > > > Opps, I've hit one issue > > /home/dfc/go/src/runtime/netpoll_solaris.c:5 not a function > /home/dfc/go/src/runtime/netpoll_solaris.c:5 syntax error, last name: > runtime > go tool dist: FAILED: /home/dfc/go/pkg/tool/solaris_amd64/6c -F -V -w -I > $WORK -I /home/dfc/go/pkg/solaris_amd64 -D GOOS_solaris -D GOARCH_amd64 > -D GOOS_GOARCH_solaris_amd64 -o $WORK/netpoll_solaris.o > /home/dfc/go/src/runtime/netpoll_solaris.c > > I'll see if I can get around it and test this CL. > > https://codereview.appspot.com/138390043/
Sign in to reply to this message.
netpoll_solaris.c is deleted by this CL (even if Rietveld does not show it).
Sign in to reply to this message.
Chnage looks good to me and passes on my solaris builder. Please wait for Aram or rsc/khr to take a look. Thanks Dave https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... File src/runtime/netpoll_solaris.go (right): https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:166: var netpolllasterr int32 could you please add a short comment. This was previously a cryptic static variable inside netpoll, now its an enigmatic package variable. // netpolllasterr holds the last error code returned by XXX during function XXX
Sign in to reply to this message.
LGTM https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... File src/runtime/netpoll_solaris.go (right): https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:70: func libcfcntlfunc() *libcFunc For all others libc.so functions we used libc_foo names, so let's do this here too. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:76: func errno() int32 { I have a WIP CL 138170043 that makes this unnecessary, but okay for now. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:81: return int32(sysvicall3(libcfcntlfunc(), uintptr(fd), uintptr(cmd), arg)) Please see comment in os_solaris.c. If you do that, this would become &libc_fcntl. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:109: print("netpollinit: failed to create port (", errno, ")\n") Doesn't errno need to be actually called here? https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:146: print("netpollupdate: failed to associate (", errno, ")\n") Same. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:199: pd := (*pollDesc)(unsafe.Pointer(ev.portev_user)) This will break with precise GC, but it's easy to fix with the map trick; the C code only holds a reference to this memory, it never dereferences it. https://codereview.appspot.com/138390043/diff/100001/src/runtime/os_solaris.c File src/runtime/os_solaris.c (right): https://codereview.appspot.com/138390043/diff/100001/src/runtime/os_solaris.c... src/runtime/os_solaris.c:569: #pragma textflag NOSPLIT These should be in assembly, see thunk_solaris.s. The reason is that once we get rid all C code that requires the address of libc.so functions, we can remove all special-case Solaris code from the linker, *and* we can change the assembly thunks to jump directly using a different, and better relocation, which in turn will allow us to remove around 2kLOC of syscall code that deals with dlopen/dlsym and other garbage like that.
Sign in to reply to this message.
PTAL I would appreciate another round of testing on solaris taking into account that the dashboard is down. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... File src/runtime/netpoll_solaris.go (right): https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:70: func libcfcntlfunc() *libcFunc On 2014/09/13 08:56:49, aram wrote: > For all others libc.so functions we used libc_foo names, so let's do this here > too. Done. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:76: func errno() int32 { On 2014/09/13 08:56:49, aram wrote: > I have a WIP CL 138170043 that makes this unnecessary, but okay for now. Acknowledged. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:81: return int32(sysvicall3(libcfcntlfunc(), uintptr(fd), uintptr(cmd), arg)) On 2014/09/13 08:56:49, aram wrote: > Please see comment in os_solaris.c. If you do that, this would become > &libc_fcntl. Done. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:109: print("netpollinit: failed to create port (", errno, ")\n") On 2014/09/13 08:56:49, aram wrote: > Doesn't errno need to be actually called here? > Done. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:146: print("netpollupdate: failed to associate (", errno, ")\n") On 2014/09/13 08:56:49, aram wrote: > Same. Done. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:166: var netpolllasterr int32 On 2014/09/13 02:27:06, dfc wrote: > could you please add a short comment. This was previously a cryptic static > variable inside netpoll, now its an enigmatic package variable. > > // netpolllasterr holds the last error code returned by XXX during function XXX Done. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:199: pd := (*pollDesc)(unsafe.Pointer(ev.portev_user)) On 2014/09/13 08:56:49, aram wrote: > This will break with precise GC, but it's easy to fix with the map trick; the C > code only holds a reference to this memory, it never dereferences it. Why will break? pollDesc's are allocated from non-movable non-GC type-stable memory, so they can be safely converted to integers and back. https://codereview.appspot.com/138390043/diff/100001/src/runtime/os_solaris.c File src/runtime/os_solaris.c (right): https://codereview.appspot.com/138390043/diff/100001/src/runtime/os_solaris.c... src/runtime/os_solaris.c:569: #pragma textflag NOSPLIT On 2014/09/13 08:56:49, aram wrote: > These should be in assembly, see thunk_solaris.s. > > The reason is that once we get rid all C code that requires the address of > libc.so functions, we can remove all special-case Solaris code from the linker, > *and* we can change the assembly thunks to jump directly using a different, and > better relocation, which in turn will allow us to remove around 2kLOC of syscall > code that deals with dlopen/dlsym and other garbage like that. Done.
Sign in to reply to this message.
LGTM Tested on 32-core Solaris in a loop for an hour, it works. https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... File src/runtime/netpoll_solaris.go (right): https://codereview.appspot.com/138390043/diff/100001/src/runtime/netpoll_sola... src/runtime/netpoll_solaris.go:199: pd := (*pollDesc)(unsafe.Pointer(ev.portev_user)) > Why will break? > pollDesc's are allocated from non-movable non-GC type-stable memory, so they can > be safely converted to integers and back. Ah, ok.
Sign in to reply to this message.
Why are we doing this now? Is it fixing any bugs? I can't see that it is. I thought the C to Go conversions were done for 1.4. Is this important enough to make an exception? It needs to be solving an actual problem to be worth the risk of modifying this code now. Russ
Sign in to reply to this message.
onM is only a stop gap, we generally don't to it unless there is no choice (e.g. we could just wrap channels into onM). So this change fixes it and does the actual conversion of netpoll. I would consider risks as low here. And we are still doing post-conversion cleanups, I see it in the commit history. On Sat, Sep 13, 2014 at 11:06 AM, Russ Cox <[email protected]> wrote: > Why are we doing this now? Is it fixing any bugs? I can't see that it is. > > I thought the C to Go conversions were done for 1.4. Is this important > enough to make an exception? It needs to be solving an actual problem to be > worth the risk of modifying this code now. > > Russ >
Sign in to reply to this message.
On Sat, Sep 13, 2014 at 2:40 PM, Dmitry Vyukov <[email protected]> wrote: > onM is only a stop gap, we generally don't to it unless there is no > choice (e.g. we could just wrap channels into onM). So this change > fixes it and does the actual conversion of netpoll. > I would consider risks as low here. And we are still doing > post-conversion cleanups, I see it in the commit history. > It sounds like this is not fixing any bugs. Please put it off until Go 1.4 is out. Russ
Sign in to reply to this message.
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/138390043 is best) in the description in your new CL. Thanks very much.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
