Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(123)

Issue 4253054: code review 4253054: runtime: scheduler, cgo reorganization (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years ago by rsc
Modified:
14 years ago
Reviewers:
CC:
iant, golang-dev
Visibility:
Public.

Description

runtime: scheduler, cgo reorganization * Change use of m->g0 stack (aka scheduler stack). * Provide runtime.mcall(f) to invoke f() on m->g0 stack. * Replace scheduler loop entry with runtime.mcall(schedule). Runtime.mcall eliminates the need for fake scheduler states that exist just to run a bit of code on the m->g0 stack (Grecovery, Gstackalloc). The elimination of the scheduler as a loop that stops and starts using gosave and gogo fixes a bad interaction with the way cgo uses the m->g0 stack. Cgo runs external (gcc-compiled) C functions on that stack, and then when calling back into Go, it sets m->g0->sched.sp below the added call frames, so that other uses of m->g0's stack will not interfere with those frames. Unfortunately, gogo (longjmp) back to the scheduler loop at this point would end up running scheduler with the lower sp, which no longer points at a valid stack frame for a call to scheduler. If scheduler then wrote any function call arguments or local variables to where it expected the stack frame to be, it would overwrite other data on the stack. I realized this possibility while debugging a problem with calling complex Go code in a Go -> C -> Go cgo callback. This wasn't the bug I was looking for, it turns out, but I believe it is a real bug nonetheless. Switching to runtime.mcall, which only adds new frames to the stack and never jumps into functions running in existing ones, fixes this bug. * Move cgo-related code out of proc.c into cgocall.c. * Add very large comment describing cgo call sequences. * Simpilify, regularize cgo function implementations and names. * Add test suite as misc/cgo/test. Now the Go -> C path calls cgocall, which calls asmcgocall, and the C -> Go path calls cgocallback, which calls cgocallbackg. The shuffling, which affects mainly the callback case, moves most of the callback implementation to cgocallback running on the m->curg stack (not the m->g0 scheduler stack) and only while accounted for with $GOMAXPROCS (between calls to exitsyscall and entersyscall). The previous callback code did not block in startcgocallback's approximation to exitsyscall, so if, say, the garbage collector were running, it would still barge in and start doing things like call malloc. Similarly endcgocallback's approximation of entersyscall did not call matchmg to kick off new OS threads when necessary, which caused the bug in issue 1560. Fixes issue 1560.

Patch Set 1 #

Patch Set 2 : diff -r 41e9fbdc1a43 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 41e9fbdc1a43 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 41e9fbdc1a43 https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 5 : diff -r cf5e73d3d332 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r 6e0c3b77928a https://go.googlecode.com/hg/ #

Total comments: 24

Patch Set 7 : diff -r 7e7ceffb8515 https://go.googlecode.com/hg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+904 lines, -416 lines) Patch
M misc/cgo/stdio/Makefile View 1 2 3 4 1 chunk +0 lines, -3 lines 0 comments Download
M misc/cgo/stdio/hello.go View 1 2 3 4 1 chunk +1 line, -19 lines 0 comments Download
A misc/cgo/test/Makefile View 1 2 3 4 1 chunk +23 lines, -0 lines 0 comments Download
M misc/cgo/test/align.go View 1 2 3 4 2 chunks +6 lines, -12 lines 0 comments Download
M misc/cgo/test/basic.go View 1 2 3 4 4 chunks +13 lines, -23 lines 0 comments Download
A misc/cgo/test/callback.go View 1 2 3 4 1 chunk +136 lines, -0 lines 0 comments Download
A misc/cgo/test/callback_c.c View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
A misc/cgo/test/cgo_test.go View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M misc/cgo/test/issue1222.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
A misc/cgo/test/issue1328.go View 1 2 3 4 5 6 1 chunk +30 lines, -0 lines 0 comments Download
A misc/cgo/test/issue1560.go View 1 2 3 4 1 chunk +46 lines, -0 lines 0 comments Download
A misc/cgo/test/runtime.c View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments Download
M src/clean.bash View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/386/asm.s View 1 2 3 4 5 6 8 chunks +165 lines, -88 lines 0 comments Download
M src/pkg/runtime/amd64/asm.s View 1 2 3 4 5 6 8 chunks +133 lines, -55 lines 0 comments Download
M src/pkg/runtime/arm/asm.s View 1 2 3 4 5 6 8 chunks +37 lines, -28 lines 0 comments Download
M src/pkg/runtime/cgocall.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/cgocall.c View 1 2 3 4 5 6 4 chunks +174 lines, -45 lines 0 comments Download
M src/pkg/runtime/mgc0.c View 1 1 chunk +0 lines, -2 lines 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 4 5 20 chunks +95 lines, -131 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 4 chunks +3 lines, -8 lines 0 comments Download

Messages

Total messages: 6
iant
http://codereview.appspot.com/4253054/diff/5002/src/cmd/cgo/out.go File src/cmd/cgo/out.go (right): http://codereview.appspot.com/4253054/diff/5002/src/cmd/cgo/out.go#newcode470 src/cmd/cgo/out.go:470: fmt.Fprintf(fc, "\truntime·asmcgocallback(·%s, a, n);\n", goname) Changing the name of ...
14 years ago (2011-03-04 16:44:34 UTC) #1
rsc
> Changing the name of this function will force another incompatible > change to SWIG. ...
14 years ago (2011-03-04 16:55:37 UTC) #2
rsc
Hello iant (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
14 years ago (2011-03-06 21:42:59 UTC) #3
iant
LGTM http://codereview.appspot.com/4253054/diff/13001/misc/cgo/test/issue1328.go File misc/cgo/test/issue1328.go (right): http://codereview.appspot.com/4253054/diff/13001/misc/cgo/test/issue1328.go#newcode18 misc/cgo/test/issue1328.go:18: xvariadic(x) // Comment this out and the code ...
14 years ago (2011-03-07 03:53:46 UTC) #4
rsc
http://codereview.appspot.com/4253054/diff/13001/misc/cgo/test/issue1328.go File misc/cgo/test/issue1328.go (right): http://codereview.appspot.com/4253054/diff/13001/misc/cgo/test/issue1328.go#newcode18 misc/cgo/test/issue1328.go:18: xvariadic(x) // Comment this out and the code runs ...
14 years ago (2011-03-07 04:02:00 UTC) #5
rsc
14 years ago (2011-03-07 15:37:45 UTC) #6
*** Submitted as a45a5a3e3458 ***

runtime: scheduler, cgo reorganization

* Change use of m->g0 stack (aka scheduler stack).
* Provide runtime.mcall(f) to invoke f() on m->g0 stack.
* Replace scheduler loop entry with runtime.mcall(schedule).

Runtime.mcall eliminates the need for fake scheduler states that
exist just to run a bit of code on the m->g0 stack
(Grecovery, Gstackalloc).

The elimination of the scheduler as a loop that stops and
starts using gosave and gogo fixes a bad interaction with the
way cgo uses the m->g0 stack.  Cgo runs external (gcc-compiled)
C functions on that stack, and then when calling back into Go,
it sets m->g0->sched.sp below the added call frames, so that
other uses of m->g0's stack will not interfere with those frames.
Unfortunately, gogo (longjmp) back to the scheduler loop at
this point would end up running scheduler with the lower
sp, which no longer points at a valid stack frame for
a call to scheduler.  If scheduler then wrote any function call
arguments or local variables to where it expected the stack
frame to be, it would overwrite other data on the stack.
I realized this possibility while debugging a problem with
calling complex Go code in a Go -> C -> Go cgo callback.
This wasn't the bug I was looking for, it turns out, but I believe
it is a real bug nonetheless.  Switching to runtime.mcall, which
only adds new frames to the stack and never jumps into
functions running in existing ones, fixes this bug.

* Move cgo-related code out of proc.c into cgocall.c.
* Add very large comment describing cgo call sequences.
* Simpilify, regularize cgo function implementations and names.
* Add test suite as misc/cgo/test.

Now the Go -> C path calls cgocall, which calls asmcgocall,
and the C -> Go path calls cgocallback, which calls cgocallbackg.

The shuffling, which affects mainly the callback case, moves
most of the callback implementation to cgocallback running
on the m->curg stack (not the m->g0 scheduler stack) and
only while accounted for with $GOMAXPROCS (between calls
to exitsyscall and entersyscall).

The previous callback code did not block in startcgocallback's
approximation to exitsyscall, so if, say, the garbage collector
were running, it would still barge in and start doing things
like call malloc.  Similarly endcgocallback's approximation of
entersyscall did not call matchmg to kick off new OS threads
when necessary, which caused the bug in issue 1560.

Fixes issue 1560.

R=iant
CC=golang-dev
http://codereview.appspot.com/4253054
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b