runtime: hold worldsema while starting the world

Currently, startTheWorld releases worldsema before starting the
world. Since startTheWorld can change gomaxprocs after allowing Ps to
run, this means that gomaxprocs can change while another P holds
worldsema.

Unfortunately, the garbage collector and forEachP assume that holding
worldsema protects against changes in gomaxprocs (which it *almost*
does). In particular, this is causing somewhat frequent "P did not run
fn" crashes in forEachP in the runtime tests because gomaxprocs is
changing between the several loops that forEachP does over all the Ps.

Fix this by only releasing worldsema after the world is started.

This relates to issue #10618. forEachP still fails under stress
testing, but much less frequently.

Change-Id: I085d627b70cca9ebe9af28fe73b9872f1bb224ff
Reviewed-on: https://go-review.googlesource.com/10156
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go
index 68636740a6c5b68871c600e67576f6cc55967d0a..a16d7603a67fbb92b72c4213c3697d2dbcb9e42a 100644 (file)
--- a/src/runtime/mgc.go
+++ b/src/runtime/mgc.go
@@ -952,13 +952,12 @@ func gc(mode int) {
        // all done
        mp.preemptoff = ""
 
-       semrelease(&worldsema)
-
        if gcphase != _GCoff {
                throw("gc done but gcphase != _GCoff")
        }
 
        systemstack(startTheWorldWithSema)
+       semrelease(&worldsema)
 
        releasem(mp)
        mp = nil

diff --git a/src/runtime/proc1.go b/src/runtime/proc1.go
index ab0566b470505a109797150f1646067a4fd51983..31247db02a0b0b85ef87c621d292759c8bd8d08a 100644 (file)
--- a/src/runtime/proc1.go
+++ b/src/runtime/proc1.go
@@ -550,12 +550,15 @@ func stopTheWorld(reason string) {
 
 // startTheWorld undoes the effects of stopTheWorld.
 func startTheWorld() {
-       semrelease(&worldsema)
        systemstack(startTheWorldWithSema)
+       // worldsema must be held over startTheWorldWithSema to ensure
+       // gomaxprocs cannot change while worldsema is held.
+       semrelease(&worldsema)
        getg().m.preemptoff = ""
 }
 
-// Holding worldsema grants an M the right to try to stop the world.
+// Holding worldsema grants an M the right to try to stop the world
+// and prevents gomaxprocs from changing concurrently.
 var worldsema uint32 = 1
 
 // stopTheWorldWithSema is the core implementation of stopTheWorld.
@@ -571,8 +574,8 @@ var worldsema uint32 = 1
 // these three operations separately:
 //
 //     m.preemptoff = ""
-//     semrelease(&worldsema)
 //     systemstack(startTheWorldWithSema)
+//     semrelease(&worldsema)
 //
 // It is allowed to acquire worldsema once and then execute multiple
 // startTheWorldWithSema/stopTheWorldWithSema pairs.