FAQ
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)

Search Discussions

  • Dvyukov at Oct 15, 2012 at 11:28 am
    I do not want to touch the deadlock detection logic, so I moved GC from
    the scavenger.

    https://codereview.appspot.com/6682050/
  • Sébastien Paolacci at Oct 15, 2012 at 6:59 pm
    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, wrote:
    I do not want to touch the deadlock detection logic, so I moved GC from
    the scavenger.

    https://codereview.appspot.com/6682050/
  • Ian Lance Taylor at Oct 15, 2012 at 5:51 pm
    LGTM
    On Mon, Oct 15, 2012 at 4:20 AM, 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)
  • Dvyukov at Oct 16, 2012 at 10:47 am

    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, wrote:
    I do not want to touch the deadlock detection logic, so I moved GC
    from


    https://codereview.appspot.com/6682050/
  • Dvyukov at Oct 16, 2012 at 10:48 am
    *** 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


    http://codereview.appspot.com/6682050/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 15, '12 at 11:27a
activeOct 16, '12 at 10:48a
posts6
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase