code review 72490043: runtime: fix memory leak in runfinq

runtime: fix memory leak in runfinq

One reason the sync.Pool finalizer test can fail is that
this function's ef1 contains uninitialized data that just
happens to point at some of the old pool. I've seen this cause
retention of a single pool cache line (32 elements) on arm.

Really we need liveness information for C functions, but
for now we can be more careful about data in long-lived
C functions that block.

[rsc, 2014-03-07 15:53:14]

Hello golang-codereviews@googlegroups.com (cc: dvyukov, iant, khr),

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

[bradfitz, 2014-03-07 15:56:22]

LGTM

Fun. Or write more of the runtime in Go.

Why don't you need the USED flush macros in the second zeroing block?
 On Mar 7, 2014 7:53 AM, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: dvyukov, iant, khr),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: fix memory leak in runfinq
>
> One reason the sync.Pool finalizer test can fail is that
> this function's ef1 contains uninitialized data that just
> happens to point at some of the old pool. I've seen this cause
> retention of a single pool cache line (32 elements) on arm.
>
> Really we need liveness information for C functions, but
> for now we can be more careful about data in long-lived
> C functions that block.
>
> Please review this at https://codereview.appspot.com/72490043/
>
> Affected files (+31, -0 lines):
>   M src/pkg/runtime/mgc0.c
>
>
> Index: src/pkg/runtime/mgc0.c
> ===================================================================
> --- a/src/pkg/runtime/mgc0.c
> +++ b/src/pkg/runtime/mgc0.c
> @@ -2525,8 +2525,29 @@
>         uint32 framesz, framecap, i;
>         Eface *ef, ef1;
>
> +       // This function blocks for long periods of time, and because it
> is written in C
> +       // we have no liveness information. Zero everything so that
> uninitialized pointers
> +       // do not cause memory leaks.
> +       f = nil;
> +       fb = nil;
> +       next = nil;
>         frame = nil;
>         framecap = 0;
> +       framesz = 0;
> +       i = 0;
> +       ef = nil;
> +       ef1.type = nil;
> +       ef1.data = nil;
> +
> +       // force flush to memory
> +       USED(&f);
> +       USED(&fb);
> +       USED(&next);
> +       USED(&framesz);
> +       USED(&i);
> +       USED(&ef);
> +       USED(&ef1);
> +
>         for(;;) {
>                 runtime·lock(&gclock);
>                 fb = finq;
> @@ -2581,6 +2602,16 @@
>                         finc = fb;
>                         runtime·unlock(&gclock);
>                 }
> +
> +               // Zero everything that's dead, to avoid memory leaks.
> +               // See comment at top of function.
> +               f = nil;
> +               fb = nil;
> +               next = nil;
> +               i = 0;
> +               ef = nil;
> +               ef1.type = nil;
> +               ef1.data = nil;
>                 runtime·gc(1);  // trigger another gc to clean up the
> finalized objects, if possible
>         }
>  }
>
>
> --
> 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+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[rsc, 2014-03-07 16:03:43]

On Fri, Mar 7, 2014 at 10:56 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Why don't you need the USED flush macros in the second zeroing block?
>

USED(&x) means the compiler must assume that any function call might look
at x (the compiler has no idea where &x ended up and who can get to it). so
it applies for the whole function essentially.

[dvyukov, 2014-03-07 16:16:36]

LGTM

[rsc, 2014-03-07 16:27:07]

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

runtime: fix memory leak in runfinq

One reason the sync.Pool finalizer test can fail is that
this function's ef1 contains uninitialized data that just
happens to point at some of the old pool. I've seen this cause
retention of a single pool cache line (32 elements) on arm.

Really we need liveness information for C functions, but
for now we can be more careful about data in long-lived
C functions that block.

LGTM=bradfitz, dvyukov
R=golang-codereviews, bradfitz, dvyukov
CC=golang-codereviews, iant, khr
https://codereview.appspot.com/72490043