diff mbox

libgo patch committed: Make NumGoroutine wait for system goroutines to register

Message ID CAOyqgcUmp4B0bUy7pdDC4BJbL0W_q0nywyUJs_iusZRQO2Mndg@mail.gmail.com
State New
Headers show

Commit Message

Ian Lance Taylor June 22, 2017, 3:46 p.m. UTC
In libgo system goroutines register themselves after they start.  That
means that there is a small race between the goroutine being seen by
the scheduler and the scheduler knowing that the goroutine is a system
goroutine. That in turn means that runtime.NumGoroutines can
overestimate the number of goroutines at times.

This patch fixes the overestimate by counting the number of system
goroutines waiting to start, and pausing NumGoroutines until those
goroutines have all registered.

This is kind of a lot of mechanism for this not very important
problem, but I couldn't think of a better approach.

The test for this is TestNumGoroutine in runtime/proc_test.go.  The
test is not currently run, but it will be soon.

Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu.  Committed
to mainline.

Ian
diff mbox

Patch

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 249564)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@ 
-54d83c2d67c35ad4f622936d2fbf81c17354fff9
+681c8a7b0a9d52c0b81e7a4b1c55fe65ed889573
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/go/runtime/debug.go
===================================================================
--- libgo/go/runtime/debug.go	(revision 249205)
+++ libgo/go/runtime/debug.go	(working copy)
@@ -54,6 +54,7 @@  func NumCgoCall() int64 {
 
 // NumGoroutine returns the number of goroutines that currently exist.
 func NumGoroutine() int {
+	waitForSystemGoroutines()
 	return int(gcount())
 }
 
Index: libgo/go/runtime/mfinal.go
===================================================================
--- libgo/go/runtime/mfinal.go	(revision 249205)
+++ libgo/go/runtime/mfinal.go	(working copy)
@@ -106,6 +106,7 @@  var (
 func createfing() {
 	// start the finalizer goroutine exactly once
 	if fingCreate == 0 && atomic.Cas(&fingCreate, 0, 1) {
+		expectSystemGoroutine()
 		go runfinq()
 	}
 }
Index: libgo/go/runtime/mgc.go
===================================================================
--- libgo/go/runtime/mgc.go	(revision 249205)
+++ libgo/go/runtime/mgc.go	(working copy)
@@ -209,6 +209,7 @@  func readgogc() int32 {
 // It kicks off the background sweeper goroutine and enables GC.
 func gcenable() {
 	c := make(chan int, 1)
+	expectSystemGoroutine()
 	go bgsweep(c)
 	<-c
 	memstats.enablegc = true // now that runtime is initialized, GC is okay
@@ -1399,6 +1400,7 @@  func gcBgMarkStartWorkers() {
 			break
 		}
 		if p.gcBgMarkWorker == 0 {
+			expectSystemGoroutine()
 			go gcBgMarkWorker(p)
 			notetsleepg(&work.bgMarkReady, -1)
 			noteclear(&work.bgMarkReady)
Index: libgo/go/runtime/proc.go
===================================================================
--- libgo/go/runtime/proc.go	(revision 249561)
+++ libgo/go/runtime/proc.go	(working copy)
@@ -233,6 +233,7 @@  func os_beforeExit() {
 
 // start forcegc helper goroutine
 func init() {
+	expectSystemGoroutine()
 	go forcegchelper()
 }
 
@@ -2728,6 +2729,28 @@  func newproc(fn uintptr, arg unsafe.Poin
 	return newg
 }
 
+// expectedSystemGoroutines counts the number of goroutines expected
+// to mark themselves as system goroutines. After they mark themselves
+// by calling setSystemGoroutine, this is decremented. NumGoroutines
+// uses this to wait for all system goroutines to mark themselves
+// before it counts them.
+var expectedSystemGoroutines uint32
+
+// expectSystemGoroutine is called when starting a goroutine that will
+// call setSystemGoroutine. It increments expectedSystemGoroutines.
+func expectSystemGoroutine() {
+	atomic.Xadd(&expectedSystemGoroutines, +1)
+}
+
+// waitForSystemGoroutines waits for all currently expected system
+// goroutines to register themselves.
+func waitForSystemGoroutines() {
+	for atomic.Load(&expectedSystemGoroutines) > 0 {
+		Gosched()
+		osyield()
+	}
+}
+
 // setSystemGoroutine marks this goroutine as a "system goroutine".
 // In the gc toolchain this is done by comparing startpc to a list of
 // saved special PCs. In gccgo that approach does not work as startpc
@@ -2738,6 +2761,7 @@  func newproc(fn uintptr, arg unsafe.Poin
 func setSystemGoroutine() {
 	getg().isSystemGoroutine = true
 	atomic.Xadd(&sched.ngsys, +1)
+	atomic.Xadd(&expectedSystemGoroutines, -1)
 }
 
 // Put on gfree list.
Index: libgo/go/runtime/time.go
===================================================================
--- libgo/go/runtime/time.go	(revision 249205)
+++ libgo/go/runtime/time.go	(working copy)
@@ -113,6 +113,7 @@  func addtimerLocked(t *timer) {
 	}
 	if !timers.created {
 		timers.created = true
+		expectSystemGoroutine()
 		go timerproc()
 	}
 }