Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(245)

Issue 124670043: code review 124670043: runtime: fix deadlock when gctrace (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 6 months ago by dvyukov
Modified:
10 years, 6 months ago
Reviewers:
khr
CC:
golang-codereviews, rsc, khr, rlh
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r 9985ecf9828f87d8887550109060357e2163ab8a https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 3 : diff -r 9985ecf9828f87d8887550109060357e2163ab8a https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 4 : diff -r 9985ecf9828f87d8887550109060357e2163ab8a https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 5 : diff -r 9985ecf9828f87d8887550109060357e2163ab8a https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 6 : diff -r 9985ecf9828f87d8887550109060357e2163ab8a https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 1

Patch Set 7 : diff -r 60fa2aa366ab63ad5773c465df92f5ded3bce245 https://dvyukov%40google.com@code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -5 lines) Patch
M src/pkg/runtime/heapdump.c View 1 2 chunks +1 line, -1 line 0 comments Download
M src/pkg/runtime/malloc.go View 1 2 3 4 3 chunks +6 lines, -0 lines 0 comments Download
M src/pkg/runtime/mheap.c View 1 2 3 4 2 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 6
dvyukov
Hello golang-codereviews@googlegroups.com (cc: khr@golang.org, rlh@golang.org, rsc@golang.org), I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
10 years, 6 months ago (2014-08-20 11:22:33 UTC) #1
dvyukov
I will appreciate review of this one. This currently causes hangs on perf builders. And ...
10 years, 6 months ago (2014-08-20 18:28:56 UTC) #2
rsc
R=khr
10 years, 6 months ago (2014-08-20 18:31:03 UTC) #3
khr
LGTM. https://codereview.appspot.com/124670043/diff/50005/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/124670043/diff/50005/src/pkg/runtime/proc.c#newcode506 src/pkg/runtime/proc.c:506: // we just don't do it today. I ...
10 years, 6 months ago (2014-08-20 22:02:38 UTC) #4
dvyukov
On 2014/08/20 22:02:38, khr wrote: > LGTM. > > https://codereview.appspot.com/124670043/diff/50005/src/pkg/runtime/proc.c > File src/pkg/runtime/proc.c (right): > ...
10 years, 6 months ago (2014-08-21 07:46:35 UTC) #5
dvyukov
10 years, 6 months ago (2014-08-21 07:46:59 UTC) #6
*** Submitted as https://code.google.com/p/go/source/detail?r=b839c52cb382 ***

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://codereview.appspot.com/124670043
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b