Commit a0dbbeae authored by Dmitriy Vyukov's avatar Dmitriy Vyukov

runtime: fix deadlock when gctrace

Calling ReadMemStats which does stoptheworld on m0 holding locks
was not a good idea.
Stoptheworld holding locks is a recipe for deadlocks (added check for this).
Stoptheworld on g0 may or may not work (added check for this as well).
As far as I understand scavenger will print incorrect numbers now,
as stack usage is not subtracted from heap. But it's better than deadlocking.

LGTM=khr
R=golang-codereviews, rsc, khr
CC=golang-codereviews, rlh
https://golang.org/cl/124670043
parent e249b0ff
...@@ -748,7 +748,6 @@ runtime∕debug·WriteHeapDump(uintptr fd) ...@@ -748,7 +748,6 @@ runtime∕debug·WriteHeapDump(uintptr fd)
// Stop the world. // Stop the world.
runtime·semacquire(&runtime·worldsema, false); runtime·semacquire(&runtime·worldsema, false);
g->m->gcing = 1; g->m->gcing = 1;
g->m->locks++;
runtime·stoptheworld(); runtime·stoptheworld();
// Update stats so we can dump them. // Update stats so we can dump them.
...@@ -774,6 +773,7 @@ runtime∕debug·WriteHeapDump(uintptr fd) ...@@ -774,6 +773,7 @@ runtime∕debug·WriteHeapDump(uintptr fd)
// Start up the world again. // Start up the world again.
g->m->gcing = 0; g->m->gcing = 0;
g->m->locks++;
runtime·semrelease(&runtime·worldsema); runtime·semrelease(&runtime·worldsema);
runtime·starttheworld(); runtime·starttheworld();
g->m->locks--; g->m->locks--;
......
...@@ -413,6 +413,7 @@ func gogc(force int32) { ...@@ -413,6 +413,7 @@ func gogc(force int32) {
return return
} }
releasem(mp) releasem(mp)
mp = nil
if panicking != 0 { if panicking != 0 {
return return
...@@ -441,7 +442,11 @@ func gogc(force int32) { ...@@ -441,7 +442,11 @@ func gogc(force int32) {
startTime := gonanotime() startTime := gonanotime()
mp = acquirem() mp = acquirem()
mp.gcing = 1 mp.gcing = 1
releasem(mp)
stoptheworld() stoptheworld()
if mp != acquirem() {
gothrow("gogc: rescheduled")
}
clearpools() clearpools()
...@@ -474,6 +479,7 @@ func gogc(force int32) { ...@@ -474,6 +479,7 @@ func gogc(force int32) {
semrelease(&worldsema) semrelease(&worldsema)
starttheworld() starttheworld()
releasem(mp) releasem(mp)
mp = nil
// now that gc is done, kick off finalizer thread if needed // now that gc is done, kick off finalizer thread if needed
if !concurrentSweep { if !concurrentSweep {
......
...@@ -608,7 +608,6 @@ scavenge(int32 k, uint64 now, uint64 limit) ...@@ -608,7 +608,6 @@ scavenge(int32 k, uint64 now, uint64 limit)
{ {
uint32 i; uint32 i;
uintptr sumreleased; uintptr sumreleased;
MStats stats;
MHeap *h; MHeap *h;
h = &runtime·mheap; h = &runtime·mheap;
...@@ -618,12 +617,13 @@ scavenge(int32 k, uint64 now, uint64 limit) ...@@ -618,12 +617,13 @@ scavenge(int32 k, uint64 now, uint64 limit)
sumreleased += scavengelist(&h->freelarge, now, limit); sumreleased += scavengelist(&h->freelarge, now, limit);
if(runtime·debug.gctrace > 0) { if(runtime·debug.gctrace > 0) {
runtime·ReadMemStats(&stats);
if(sumreleased > 0) if(sumreleased > 0)
runtime·printf("scvg%d: %D MB released\n", k, (uint64)sumreleased>>20); runtime·printf("scvg%d: %D MB released\n", k, (uint64)sumreleased>>20);
// TODO(dvyukov): these stats are incorrect as we don't subtract stack usage from heap.
// But we can't call ReadMemStats on g0 holding locks.
runtime·printf("scvg%d: inuse: %D, idle: %D, sys: %D, released: %D, consumed: %D (MB)\n", runtime·printf("scvg%d: inuse: %D, idle: %D, sys: %D, released: %D, consumed: %D (MB)\n",
k, stats.heap_inuse>>20, stats.heap_idle>>20, stats.heap_sys>>20, k, mstats.heap_inuse>>20, mstats.heap_idle>>20, mstats.heap_sys>>20,
stats.heap_released>>20, (stats.heap_sys - stats.heap_released)>>20); mstats.heap_released>>20, (mstats.heap_sys - mstats.heap_released)>>20);
} }
} }
......
...@@ -498,6 +498,14 @@ runtime·stoptheworld(void) ...@@ -498,6 +498,14 @@ runtime·stoptheworld(void)
P *p; P *p;
bool wait; bool wait;
// If we hold a lock, then we won't be able to stop another M
// that is blocked trying to acquire the lock.
if(g->m->locks > 0)
runtime·throw("stoptheworld: holding locks");
// There is no evidence that stoptheworld on g0 does not work,
// we just don't do it today.
if(g == g->m->g0)
runtime·throw("stoptheworld: on g0");
runtime·lock(&runtime·sched.lock); runtime·lock(&runtime·sched.lock);
runtime·sched.stopwait = runtime·gomaxprocs; runtime·sched.stopwait = runtime·gomaxprocs;
runtime·atomicstore((uint32*)&runtime·sched.gcwaiting, 1); runtime·atomicstore((uint32*)&runtime·sched.gcwaiting, 1);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment