runtime: drop nosplit from primary lockrank functions

acquireLockRank and releaseLockRank are called from nosplit context, and
thus must be nosplit.

lockWithRank, unlockWithRank, and lockWithRankMayAcquire are called from
spittable context, and thus don't strictly need to be nosplit.

The stated reasoning for making these functions nosplit is to avoid
re-entrant calls due to a stack split on function entry taking a lock.
There are two potential issues at play here:

1. A stack split on function entry adds a new lock ordering edge before
   we (a) take lock l, or (b) release lock l.

2. A stack split in a child call (such as to lock2) introduces a new
   lock ordering edge _in the wrong order_ because e.g., in the case of
   lockWithRank, we've noted that l is taken, but the stack split in
   lock2 actually takes stack split locks _before_ l is actually locked.

(1) is indeed avoided by marking these functions nosplit, but this is
really just a bit of duct tape that generally has no effect overall. Any
earlier call can have a stack split and introduce the same new edge.
This includes lock/unlock which are not nosplit!

I began this CL as a change to extend nosplit to lock and unlock to try
to make this mitigation more effective, but I've realized that as long
as there is a _single_ nosplit call between a lock and unlock, we can
end up with the edge. There seems to be few enough cases without any
calls that is does not seem worth the extra cognitive load to extend
nosplit throughout all of the locking functions.

(2) is a real issue which would cause incorrect ordering, but it is
already handled by switching to the system stack before recording the
lock ordering. Adding / removing nosplit has no effect on this issue.

Change-Id: I94fbd21b2bf928dbf1bf71aabb6788fc0a012829
Reviewed-on: https://go-review.googlesource.com/c/go/+/254367
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Trust: Michael Pratt <mpratt@google.com>

diff --git a/src/runtime/lockrank_off.go b/src/runtime/lockrank_off.go
index c04b61edc7e47716162929316f7d932e0b8a5b06..40edf882eeb788e935f6ddd12a89967385ebaa55 100644 (file)
--- a/src/runtime/lockrank_off.go
+++ b/src/runtime/lockrank_off.go
@@ -18,30 +18,24 @@ func getLockRank(l *mutex) lockRank {
        return 0
 }
 
-// The following functions may be called in nosplit context.
-// Nosplit is not strictly required for lockWithRank, unlockWithRank
-// and lockWithRankMayAcquire, but these nosplit annotations must
-// be kept consistent with the equivalent functions in lockrank_on.go.
-
-//go:nosplit
 func lockWithRank(l *mutex, rank lockRank) {
        lock2(l)
 }
 
+// This function may be called in nosplit context and thus must be nosplit.
 //go:nosplit
 func acquireLockRank(rank lockRank) {
 }
 
-//go:nosplit
 func unlockWithRank(l *mutex) {
        unlock2(l)
 }
 
+// This function may be called in nosplit context and thus must be nosplit.
 //go:nosplit
 func releaseLockRank(rank lockRank) {
 }
 
-//go:nosplit
 func lockWithRankMayAcquire(l *mutex, rank lockRank) {
 }
 

diff --git a/src/runtime/lockrank_on.go b/src/runtime/lockrank_on.go
index 850f7cdd38394eaed033899debe4373358b78b50..db7ff23a58dc52b3c95bbfd625561587f2e6b355 100644 (file)
--- a/src/runtime/lockrank_on.go
+++ b/src/runtime/lockrank_on.go
@@ -40,15 +40,19 @@ func getLockRank(l *mutex) lockRank {
        return l.rank
 }
 
-// The following functions are the entry-points to record lock
-// operations.
-// All of these are nosplit and switch to the system stack immediately
-// to avoid stack growths. Since a stack growth could itself have lock
-// operations, this prevents re-entrant calls.
-
 // lockWithRank is like lock(l), but allows the caller to specify a lock rank
 // when acquiring a non-static lock.
-//go:nosplit
+//
+// Note that we need to be careful about stack splits:
+//
+// This function is not nosplit, thus it may split at function entry. This may
+// introduce a new edge in the lock order, but it is no different from any
+// other (nosplit) call before this call (including the call to lock() itself).
+//
+// However, we switch to the systemstack to record the lock held to ensure that
+// we record an accurate lock ordering. e.g., without systemstack, a stack
+// split on entry to lock2() would record stack split locks as taken after l,
+// even though l is not actually locked yet.
 func lockWithRank(l *mutex, rank lockRank) {
        if l == &debuglock || l == &paniclk {
                // debuglock is only used for println/printlock(). Don't do lock
@@ -99,6 +103,8 @@ func printHeldLocks(gp *g) {
 }
 
 // acquireLockRank acquires a rank which is not associated with a mutex lock
+//
+// This function may be called in nosplit context and thus must be nosplit.
 //go:nosplit
 func acquireLockRank(rank lockRank) {
        gp := getg()
@@ -154,7 +160,7 @@ func checkRanks(gp *g, prevRank, rank lockRank) {
        }
 }
 
-//go:nosplit
+// See comment on lockWithRank regarding stack splitting.
 func unlockWithRank(l *mutex) {
        if l == &debuglock || l == &paniclk {
                // See comment at beginning of lockWithRank.
@@ -181,6 +187,8 @@ func unlockWithRank(l *mutex) {
 }
 
 // releaseLockRank releases a rank which is not associated with a mutex lock
+//
+// This function may be called in nosplit context and thus must be nosplit.
 //go:nosplit
 func releaseLockRank(rank lockRank) {
        gp := getg()
@@ -201,7 +209,7 @@ func releaseLockRank(rank lockRank) {
        })
 }
 
-//go:nosplit
+// See comment on lockWithRank regarding stack splitting.
 func lockWithRankMayAcquire(l *mutex, rank lockRank) {
        gp := getg()
        if gp.m.locksHeldLen == 0 {