From f8c350873c94baaf53b9c1c2b6ddfb463172c3de Mon Sep 17 00:00:00 2001 From: Dmitriy Vyukov Date: Wed, 26 Mar 2014 15:11:36 +0400 Subject: [PATCH] runtime: fix yet another race in bgsweep Currently it's possible that bgsweep finishes before all spans have been swept (we only know that sweeping of all spans has *started*). In such case bgsweep may fail wake up runfinq goroutine when it needs to. finq may still be nil at this point, but some finalizers may be queued later. Make bgsweep to wait for sweeping to *complete*, then it can decide whether it needs to wake up runfinq for sure. Update #7533 LGTM=rsc R=rsc CC=golang-codereviews https://golang.org/cl/75960043 --- src/pkg/runtime/malloc.goc | 1 + src/pkg/runtime/malloc.h | 4 ++ src/pkg/runtime/mgc0.c | 75 ++++++++++++++++++++------------------ src/pkg/runtime/proc.c | 2 + 4 files changed, 47 insertions(+), 35 deletions(-) diff --git a/src/pkg/runtime/malloc.goc b/src/pkg/runtime/malloc.goc index 03062adbbd..104b0f18c7 100644 --- a/src/pkg/runtime/malloc.goc +++ b/src/pkg/runtime/malloc.goc @@ -893,6 +893,7 @@ func SetFinalizer(obj Eface, finalizer Eface) { } } if(finalizer.type != nil) { + runtime·createfing(); if(finalizer.type->kind != KindFunc) goto badfunc; ft = (FuncType*)finalizer.type; diff --git a/src/pkg/runtime/malloc.h b/src/pkg/runtime/malloc.h index f4c1bef770..30eccf26f7 100644 --- a/src/pkg/runtime/malloc.h +++ b/src/pkg/runtime/malloc.h @@ -571,6 +571,10 @@ void runtime·MProf_TraceGC(void); int32 runtime·gcprocs(void); void runtime·helpgc(int32 nproc); void runtime·gchelper(void); +void runtime·createfing(void); +G* runtime·wakefing(void); +extern bool runtime·fingwait; +extern bool runtime·fingwake; void runtime·setprofilebucket(void *p, Bucket *b); diff --git a/src/pkg/runtime/mgc0.c b/src/pkg/runtime/mgc0.c index ec6712cbf9..c2519d32c3 100644 --- a/src/pkg/runtime/mgc0.c +++ b/src/pkg/runtime/mgc0.c @@ -202,15 +202,17 @@ extern byte ebss[]; extern byte gcdata[]; extern byte gcbss[]; -static G *fing; -static FinBlock *finq; // list of finalizers that are to be executed -static FinBlock *finc; // cache of free blocks -static FinBlock *allfin; // list of all blocks -static int32 fingwait; +static Lock finlock; // protects the following variables +static FinBlock *finq; // list of finalizers that are to be executed +static FinBlock *finc; // cache of free blocks +static FinBlock *allfin; // list of all blocks +bool runtime·fingwait; +bool runtime·fingwake; + static Lock gclock; +static G* fing; static void runfinq(void); -static void wakefing(void); static void bgsweep(void); static Workbuf* getempty(Workbuf*); static Workbuf* getfull(Workbuf*); @@ -1652,7 +1654,7 @@ runtime·queuefinalizer(byte *p, FuncVal *fn, uintptr nret, Type *fint, PtrType FinBlock *block; Finalizer *f; - runtime·lock(&gclock); + runtime·lock(&finlock); if(finq == nil || finq->cnt == finq->cap) { if(finc == nil) { finc = runtime·persistentalloc(FinBlockSize, 0, &mstats.gc_sys); @@ -1672,7 +1674,8 @@ runtime·queuefinalizer(byte *p, FuncVal *fn, uintptr nret, Type *fint, PtrType f->fint = fint; f->ot = ot; f->arg = p; - runtime·unlock(&gclock); + runtime·fingwake = true; + runtime·unlock(&finlock); } void @@ -1769,7 +1772,7 @@ runtime·MSpan_Sweep(MSpan *s) shift = off % wordsPerBitmapWord; *bitp |= bitMarked<specials; special = *specialp; @@ -1899,7 +1902,6 @@ static struct { G* g; bool parked; - uint32 lastsweepgen; MSpan** spans; uint32 nspan; @@ -1914,17 +1916,8 @@ bgsweep(void) for(;;) { while(runtime·sweepone() != -1) { gcstats.nbgsweep++; - if(sweep.lastsweepgen != runtime·mheap.sweepgen) { - // If bgsweep does not catch up for any reason - // (does not finish before next GC), - // we still need to kick off runfinq at least once per GC. - sweep.lastsweepgen = runtime·mheap.sweepgen; - wakefing(); - } runtime·gosched(); } - // kick off goroutine to run queued finalizers - wakefing(); runtime·lock(&gclock); if(!runtime·mheap.sweepdone) { // It's possible if GC has happened between sweepone has @@ -2277,8 +2270,6 @@ runtime·gc(int32 force) // now that gc is done, kick off finalizer thread if needed if(!ConcurrentSweep) { - // kick off goroutine to run queued finalizers - wakefing(); // give the queued finalizers, if any, a chance to run runtime·gosched(); } @@ -2565,15 +2556,15 @@ runfinq(void) USED(&ef1); for(;;) { - runtime·lock(&gclock); + runtime·lock(&finlock); fb = finq; finq = nil; if(fb == nil) { - fingwait = 1; - runtime·parkunlock(&gclock, "finalizer wait"); + runtime·fingwait = true; + runtime·parkunlock(&finlock, "finalizer wait"); continue; } - runtime·unlock(&gclock); + runtime·unlock(&finlock); if(raceenabled) runtime·racefingo(); for(; fb; fb=next) { @@ -2613,10 +2604,10 @@ runfinq(void) f->ot = nil; } fb->cnt = 0; - runtime·lock(&gclock); + runtime·lock(&finlock); fb->next = finc; finc = fb; - runtime·unlock(&gclock); + runtime·unlock(&finlock); } // Zero everything that's dead, to avoid memory leaks. @@ -2632,22 +2623,36 @@ runfinq(void) } } -static void -wakefing(void) +void +runtime·createfing(void) { - if(finq == nil) + if(fing != nil) return; + // Here we use gclock instead of finlock, + // because newproc1 can allocate, which can cause on-demand span sweep, + // which can queue finalizers, which would deadlock. runtime·lock(&gclock); - // kick off or wake up goroutine to run queued finalizers if(fing == nil) fing = runtime·newproc1(&runfinqv, nil, 0, 0, runtime·gc); - else if(fingwait) { - fingwait = 0; - runtime·ready(fing); - } runtime·unlock(&gclock); } +G* +runtime·wakefing(void) +{ + G *res; + + res = nil; + runtime·lock(&finlock); + if(runtime·fingwait && runtime·fingwake) { + runtime·fingwait = false; + runtime·fingwake = false; + res = fing; + } + runtime·unlock(&finlock); + return res; +} + void runtime·marknogc(void *v) { diff --git a/src/pkg/runtime/proc.c b/src/pkg/runtime/proc.c index 375dced240..2ab54be70c 100644 --- a/src/pkg/runtime/proc.c +++ b/src/pkg/runtime/proc.c @@ -1144,6 +1144,8 @@ top: gcstopm(); goto top; } + if(runtime·fingwait && runtime·fingwake && (gp = runtime·wakefing()) != nil) + runtime·ready(gp); // local runq gp = runqget(m->p); if(gp) -- 2.48.1