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 c04b61e..40edf88 100644
--- a/src/runtime/lockrank_off.go
+++ b/src/runtime/lockrank_off.go
@@ -18,30 +18,24 @@
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 850f7cd..db7ff23 100644
--- a/src/runtime/lockrank_on.go
+++ b/src/runtime/lockrank_on.go
@@ -40,15 +40,19 @@
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 @@
}
// 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 @@
}
}
-//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 @@
}
// 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 @@
})
}
-//go:nosplit
+// See comment on lockWithRank regarding stack splitting.
func lockWithRankMayAcquire(l *mutex, rank lockRank) {
gp := getg()
if gp.m.locksHeldLen == 0 {
