code review 6682050: runtime: fix spurious deadlock crashes

runtime: fix spurious deadlock crashes
Fixes issue 4243.

[dvyukov, 2012-10-15 11:20:59]

Hello golang-dev@googlegroups.com (cc: sebastien.paolacci@gmail.com),

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

[dvyukov, 2012-10-15 11:22:09]

I do not want to touch the deadlock detection logic, so I moved GC from the
scavenger.

[iant2, 2012-10-15 16:49:18]

LGTM

On Mon, Oct 15, 2012 at 4:20 AM,  <dvyukov@google.com> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: sebastien.paolacci@gmail.com),
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: fix spurious deadlock crashes
> Fixes issue 4243.
>
> Please review this at http://codereview.appspot.com/6682050/
>
> Affected files:
>   M src/pkg/runtime/mheap.c
>
>
> Index: src/pkg/runtime/mheap.c
> ===================================================================
> --- a/src/pkg/runtime/mheap.c
> +++ b/src/pkg/runtime/mheap.c
> @@ -343,6 +343,13 @@
>                 runtime·MSpanList_Insert(&h->large, s);
>  }
>
> +static void
> +forcegchelper(Note *note)
> +{
> +       runtime·gc(1);
> +       runtime·notewakeup(note);
> +}
> +
>  // Release (part of) unused memory to OS.
>  // Goroutine created at startup.
>  // Loop forever.
> @@ -356,7 +363,7 @@
>         uintptr released, sumreleased;
>         byte *env;
>         bool trace;
> -       Note note;
> +       Note note, *notep;
>
>         // If we go two minutes without a garbage collection, force one to
> run.
>         forcegc = 2*60*1e9;
> @@ -385,7 +392,15 @@
>                 now = runtime·nanotime();
>                 if(now - mstats.last_gc > forcegc) {
>                         runtime·unlock(h);
> -                       runtime·gc(1);
> +                       // The scavenger can not block other goroutines,
> +                       // otherwise deadlock detector can fire spuriously.
> +                       // GC blocks other goroutines via the
> runtime·worldsema.
> +                       runtime·noteclear(&note);
> +                       notep = &note;
> +                       runtime·newproc1((byte*)forcegchelper,
> (byte*)&notep, sizeof(notep), 0, runtime·MHeap_Scavenger);
> +                       runtime·entersyscall();
> +                       runtime·notesleep(&note);
> +                       runtime·exitsyscall();
>                         runtime·lock(h);
>                         now = runtime·nanotime();
>                         if (trace)
>
>

[Sebastien Paolacci, 2012-10-15 18:59:15]

LGTM. I can't think I've never thought about that one, nor observed
any crash (and I'm having long running processes..), but the deadlock
detection logic was definitely wrong by not expecting that the
scavenger could block (/unblock) other goroutines.

Thanks.

Sebastien

On Mon, Oct 15, 2012 at 1:22 PM,  <dvyukov@google.com> wrote:
> I do not want to touch the deadlock detection logic, so I moved GC from
> the scavenger.
>
> https://codereview.appspot.com/6682050/

[dvyukov, 2012-10-16 10:41:18]

On 2012/10/15 18:59:15, Sebastien Paolacci wrote:
> LGTM. I can't think I've never thought about that one, nor observed
> any crash (and I'm having long running processes..)

Probability of the crash is highly dependent on type of work you do. E.g. if you
have periodic GCs, then the scavenger won't trigger any GCs, I think that's your
case. If your program may not trigger GC for several minutes then the crash can
happen. If your program trigger GCs very episodically and you call ReadMemStats
frequently, then it will crash on a daily basis.



, but the deadlock
> detection logic was definitely wrong by not expecting that the
> scavenger could block (/unblock) other goroutines.
> 
> Thanks.
> 
> Sebastien
> 
> On Mon, Oct 15, 2012 at 1:22 PM,  <mailto:dvyukov@google.com> wrote:
> > I do not want to touch the deadlock detection logic, so I moved GC from
> > the scavenger.
> >
> > https://codereview.appspot.com/6682050/

[dvyukov, 2012-10-16 10:41:47]

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

runtime: fix spurious deadlock crashes
Fixes issue 4243.

R=golang-dev, iant
CC=golang-dev, sebastien.paolacci
http://codereview.appspot.com/6682050