Commit 835dfef9 authored by Vladimir Stefanovic's avatar Vladimir Stefanovic Committed by Cherry Zhang

runtime/pprof: prevent a deadlock that SIGPROF might create on mips{,le}

64bit atomics on mips/mipsle are implemented using spinlocks. If SIGPROF
is received while the program is in the critical section, it will try to
write the sample using the same spinlock, creating a deadloop.
Prevent it by creating a counter of SIGPROFs during atomic64 and
postpone writing the sample(s) until called from elsewhere, with
pc set to _LostSIGPROFDuringAtomic64.

Added a test case, per Cherry's suggestion. Works around #20146.

Change-Id: Icff504180bae4ee83d78b19c0d9d6a80097087f9
Reviewed-on: https://go-review.googlesource.com/42652
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: default avatarCherry Zhang <cherryyz@google.com>
parent df91b804
...@@ -163,6 +163,15 @@ func (p *cpuProfile) addExtra() { ...@@ -163,6 +163,15 @@ func (p *cpuProfile) addExtra() {
} }
} }
func (p *cpuProfile) addLostAtomic64(count uint64) {
hdr := [1]uint64{count}
lostStk := [2]uintptr{
funcPC(_LostSIGPROFDuringAtomic64) + sys.PCQuantum,
funcPC(_System) + sys.PCQuantum,
}
cpuprof.log.write(nil, 0, hdr[:], lostStk[:])
}
// CPUProfile panics. // CPUProfile panics.
// It formerly provided raw access to chunks of // It formerly provided raw access to chunks of
// a pprof-format profile generated by the runtime. // a pprof-format profile generated by the runtime.
......
...@@ -12,6 +12,7 @@ import ( ...@@ -12,6 +12,7 @@ import (
"fmt" "fmt"
"internal/testenv" "internal/testenv"
"io" "io"
"io/ioutil"
"math/big" "math/big"
"os" "os"
"os/exec" "os/exec"
...@@ -20,6 +21,7 @@ import ( ...@@ -20,6 +21,7 @@ import (
"runtime/pprof/internal/profile" "runtime/pprof/internal/profile"
"strings" "strings"
"sync" "sync"
"sync/atomic"
"testing" "testing"
"time" "time"
) )
...@@ -713,3 +715,32 @@ func TestCPUProfileLabel(t *testing.T) { ...@@ -713,3 +715,32 @@ func TestCPUProfileLabel(t *testing.T) {
}) })
}) })
} }
// Check that there is no deadlock when the program receives SIGPROF while in
// 64bit atomics' critical section. Used to happen on mips{,le}. See #20146.
func TestAtomicLoadStore64(t *testing.T) {
f, err := ioutil.TempFile("", "profatomic")
if err != nil {
t.Fatalf("TempFile: %v", err)
}
defer os.Remove(f.Name())
defer f.Close()
if err := StartCPUProfile(f); err != nil {
t.Fatal(err)
}
defer StopCPUProfile()
var flag uint64
done := make(chan bool, 1)
go func() {
for atomic.LoadUint64(&flag) == 0 {
runtime.Gosched()
}
done <- true
}()
time.Sleep(50 * time.Millisecond)
atomic.StoreUint64(&flag, 1)
<-done
}
...@@ -3232,10 +3232,14 @@ var prof struct { ...@@ -3232,10 +3232,14 @@ var prof struct {
hz int32 hz int32
} }
func _System() { _System() } func _System() { _System() }
func _ExternalCode() { _ExternalCode() } func _ExternalCode() { _ExternalCode() }
func _LostExternalCode() { _LostExternalCode() } func _LostExternalCode() { _LostExternalCode() }
func _GC() { _GC() } func _GC() { _GC() }
func _LostSIGPROFDuringAtomic64() { _LostSIGPROFDuringAtomic64() }
// Counts SIGPROFs received while in atomic64 critical section, on mips{,le}
var lostAtomic64Count uint64
// Called if we receive a SIGPROF signal. // Called if we receive a SIGPROF signal.
// Called by the signal handler, may run during STW. // Called by the signal handler, may run during STW.
...@@ -3245,6 +3249,21 @@ func sigprof(pc, sp, lr uintptr, gp *g, mp *m) { ...@@ -3245,6 +3249,21 @@ func sigprof(pc, sp, lr uintptr, gp *g, mp *m) {
return return
} }
// On mips{,le}, 64bit atomics are emulated with spinlocks, in
// runtime/internal/atomic. If SIGPROF arrives while the program is inside
// the critical section, it creates a deadlock (when writing the sample).
// As a workaround, create a counter of SIGPROFs while in critical section
// to store the count, and pass it to sigprof.add() later when SIGPROF is
// received from somewhere else (with _LostSIGPROFDuringAtomic64 as pc).
if GOARCH == "mips" || GOARCH == "mipsle" {
if f := findfunc(pc); f.valid() {
if hasprefix(funcname(f), "runtime/internal/atomic") {
lostAtomic64Count++
return
}
}
}
// Profiling runs concurrently with GC, so it must not allocate. // Profiling runs concurrently with GC, so it must not allocate.
// Set a trap in case the code does allocate. // Set a trap in case the code does allocate.
// Note that on windows, one thread takes profiles of all the // Note that on windows, one thread takes profiles of all the
...@@ -3371,6 +3390,10 @@ func sigprof(pc, sp, lr uintptr, gp *g, mp *m) { ...@@ -3371,6 +3390,10 @@ func sigprof(pc, sp, lr uintptr, gp *g, mp *m) {
} }
if prof.hz != 0 { if prof.hz != 0 {
if (GOARCH == "mips" || GOARCH == "mipsle") && lostAtomic64Count > 0 {
cpuprof.addLostAtomic64(lostAtomic64Count)
lostAtomic64Count = 0
}
cpuprof.add(gp, stk[:n]) cpuprof.add(gp, stk[:n])
} }
getg().m.mallocing-- getg().m.mallocing--
......
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