FAQ
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

Search Discussions

  • Dave Cheney at Nov 9, 2012 at 7:48 am
    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
  • Minux at Nov 9, 2012 at 9:51 am

    On Fri, Nov 9, 2012 at 3:48 PM, Dave Cheney 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.
  • Dvyukov at Nov 9, 2012 at 9:44 am
    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#newcode226
    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.

    https://codereview.appspot.com/6819119/
  • Iant at Nov 9, 2012 at 2:54 pm
    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#newcode226
    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.

    https://codereview.appspot.com/6819119/
  • Dvyukov at Nov 9, 2012 at 3:06 pm
    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#newcode226
    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

    https://codereview.appspot.com/6819119/
  • Iant at Nov 10, 2012 at 7:19 pm
    *** 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


    http://codereview.appspot.com/6819119/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 9, '12 at 7:41a
activeNov 10, '12 at 7:19p
posts7
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase