code review 6819119: runtime, runtime/cgo: track memory allocated by non-Go code

runtime, runtime/cgo: track memory allocated by non-Go code

Otherwise a poorly timed GC can collect the memory before it
is returned to the Go program.

[iant, 2012-11-09 07:41:04]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2012-11-09 07:48:57]

Very interesting. I wonder if this was a cause of the segfaults we see
occasionally on arm. 

On 09/11/2012, at 18:41, iant@golang.org wrote:

> Reviewers: golang-dev_googlegroups.com,
> 
> Message:
> Hello golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/
> 
> 
> Description:
> runtime, runtime/cgo: track memory allocated by non-Go code
> 
> Otherwise a poorly timed GC can collect the memory before it
> is returned to the Go program.
> 
> Please review this at http://codereview.appspot.com/6819119/
> 
> Affected files:
>  M src/pkg/runtime/cgo/callbacks.c
>  M src/pkg/runtime/cgocall.c
>  M src/pkg/runtime/runtime.h
> 
> 
> Index: src/pkg/runtime/cgo/callbacks.c
> ===================================================================
> --- a/src/pkg/runtime/cgo/callbacks.c
> +++ b/src/pkg/runtime/cgo/callbacks.c
> @@ -33,7 +33,13 @@
> static void
> _cgo_allocate_internal(uintptr len, byte *ret)
> {
> +    CgoMal *c;
> +
>    ret = runtime·mal(len);
> +    c = runtime·mal(sizeof(*c));
> +    c->next = g->cgomal;
> +    c->alloc = ret;
> +    g->cgomal = c;
>    FLUSH(&ret);
> }
> 
> Index: src/pkg/runtime/cgocall.c
> ===================================================================
> --- a/src/pkg/runtime/cgocall.c
> +++ b/src/pkg/runtime/cgocall.c
> @@ -135,6 +135,8 @@
>        g->defer = &d;
>    }
> 
> +    g->ncgo++;
> +
>    /*
>     * Announce we are entering a system call
>     * so that the scheduler knows to create another
> @@ -150,6 +152,14 @@
>    runtime·asmcgocall(fn, arg);
>    runtime·exitsyscall();
> 
> +    g->ncgo--;
> +    if(g->ncgo == 0) {
> +        // We are going back to Go and are not in a recursive
> +        // call.  Let the GC collect any memory allocated via
> +        // _cgo_allocate that is no longer referenced.
> +        g->cgomal = nil;
> +    }
> +
>    if(d.nofree) {
>        if(g->defer != &d || d.fn != (byte*)unlockm)
>            runtime·throw("runtime: bad defer entry in cgocallback");
> Index: src/pkg/runtime/runtime.h
> ===================================================================
> --- a/src/pkg/runtime/runtime.h
> +++ b/src/pkg/runtime/runtime.h
> @@ -81,6 +81,7 @@
> typedef struct    LFNode        LFNode;
> typedef struct    ParFor        ParFor;
> typedef struct    ParForThread    ParForThread;
> +typedef struct    CgoMal        CgoMal;
> 
> /*
>  * Per-CPU declaration.
> @@ -221,6 +222,8 @@
>    uintptr    sigcode1;
>    uintptr    sigpc;
>    uintptr    gopc;    // pc of go statement that created this goroutine
> +    int32    ncgo;        // number of cgo calls in progress
> +    CgoMal*    cgomal;
>    uintptr    end[];
> };
> struct    M
> @@ -414,6 +417,14 @@
>    uint64 nsleep;
> };
> 
> +// Track memory allocated by code not written in Go during a cgo call,
> +// so that the garbage collector can see them.
> +struct CgoMal
> +{
> +    CgoMal    *next;
> +    byte    *alloc;
> +};
> +
> /*
>  * defined macros
>  *    you need super-gopher-guru privilege
> 
> 

[dvyukov, 2012-11-09 09:44:09]

https://codereview.appspot.com/6819119/diff/1/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/6819119/diff/1/src/pkg/runtime/runtime.h#newco...
src/pkg/runtime/runtime.h:226: CgoMal*	cgomal;
I would prefer moving these 2 fields to M, there are much more G's than M's. The
M is locked to G during cgo calls.

[minux1, 2012-11-09 09:51:32]

On Fri, Nov 9, 2012 at 3:48 PM, Dave Cheney <dave@cheney.net> wrote:

> Very interesting. I wonder if this was a cause of the segfaults we see
> occasionally on arm.
>
no. i think cgo_allocate is only used by swig generated code.

[iant, 2012-11-09 14:54:04]

https://codereview.appspot.com/6819119/diff/1/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/6819119/diff/1/src/pkg/runtime/runtime.h#newco...
src/pkg/runtime/runtime.h:226: CgoMal*	cgomal;
On 2012/11/09 09:44:09, dvyukov wrote:
> I would prefer moving these 2 fields to M, there are much more G's than M's.
The
> M is locked to G during cgo calls.

Done.  Thanks.

[dvyukov, 2012-11-09 15:06:35]

On 2012/11/09 14:54:04, iant wrote:
> https://codereview.appspot.com/6819119/diff/1/src/pkg/runtime/runtime.h
> File src/pkg/runtime/runtime.h (right):
> 
>
https://codereview.appspot.com/6819119/diff/1/src/pkg/runtime/runtime.h#newco...
> src/pkg/runtime/runtime.h:226: CgoMal*	cgomal;
> On 2012/11/09 09:44:09, dvyukov wrote:
> > I would prefer moving these 2 fields to M, there are much more G's than M's.
> The
> > M is locked to G during cgo calls.
> 
> Done.  Thanks.

LGTM

[iant, 2012-11-10 19:19:11]

*** Submitted as http://code.google.com/p/go/source/detail?r=97b8514db41b ***

runtime, runtime/cgo: track memory allocated by non-Go code

Otherwise a poorly timed GC can collect the memory before it
is returned to the Go program.

R=golang-dev, dave, dvyukov, minux.ma
CC=golang-dev
http://codereview.appspot.com/6819119