| Age | Commit message (Collapse) | Author |
|---|
|
The function is called in nosplit context already.
Fixes #68525
Change-Id: I8b9e324335b7c605d5a20cacdd58df09b7a7cf52
Reviewed-on: https://go-review.googlesource.com/c/go/+/599675
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
|
|
Profiling of runtime-internal locks checks gp.m.locks to see if it's
safe to add a new record to the profile, but direct use of
acquireLockRank can change the list of the M's active lock ranks without
updating gp.m.locks to match. The runtime's internal rwmutex
implementation makes a point of calling acquirem/releasem when
manipulating the lock rank list, but the other user of acquireLockRank
(the GC's Gscan bit) relied on the GC's invariants to avoid deadlocks.
Codify the rwmutex approach by renaming acquireLockRank to
acquireLockRankAndM and having it include a call to aquirem. Do the same
for release.
Fixes #64706
Fixes #66004
Change-Id: Ib76eaa0cc1c45b64861d03345e17e1e843c19713
GitHub-Last-Rev: 160577bdb2bb2a4e869c6fd7e53e3be8fb819182
GitHub-Pull-Request: golang/go#66276
Reviewed-on: https://go-review.googlesource.com/c/go/+/571056
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Rhys Hiltner <rhys.hiltner@gmail.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
|
|
Also preserve the PC/SP in reentersyscall when doing lock ranking.
The test is TestDestructorCallbackRace with the staticlockranking
experiment enabled.
For #59711
Change-Id: I87ac1d121ec0d399de369666834891ab9e7d11b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/487955
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
|
|
A future change to gofmt will rewrite
// Doc comment.
//go:foo
to
// Doc comment.
//
//go:foo
Apply that change preemptively to all comments (not necessarily just doc comments).
For #51082.
Change-Id: Iffe0285418d1e79d34526af3520b415a12203ca9
Reviewed-on: https://go-review.googlesource.com/c/go/+/384260
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
|
|
When these packages are released as part of Go 1.18,
Go 1.16 will no longer be supported, so we can remove
the +build tags in these files.
Ran go fix -fix=buildtag std cmd and then reverted the bootstrapDirs
as defined in src/cmd/dist/buildtool.go, which need to continue
to build with Go 1.4 for now.
Also reverted src/vendor and src/cmd/vendor, which will need
to be updated in their own repos first.
Manual changes in runtime/pprof/mprof_test.go to adjust line numbers.
For #41184.
Change-Id: Ic0f93f7091295b6abc76ed5cd6e6746e1280861e
Reviewed-on: https://go-review.googlesource.com/c/go/+/344955
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
|
|
Make all our package sources use Go 1.17 gofmt format
(adding //go:build lines).
Part of //go:build change (#41184).
See https://golang.org/design/draft-gobuild
Change-Id: Ia0534360e4957e58cd9a18429c39d0e32a6addb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/294430
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
|
|
Stopping the world is an implicit lock for many operations, so we should
assert the world is stopped in functions that require it.
This is enabled along with the rest of lock ranking, though it is a bit
orthogonal and likely cheap enough to enable all the time should we
choose.
Requiring a lock _or_ world stop is common, so that can be expressed as
well.
Updates #40677
Change-Id: If0a58544f4251d367f73c4120c9d39974c6cd091
Reviewed-on: https://go-review.googlesource.com/c/go/+/248577
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Trust: Michael Pratt <mpratt@google.com>
|
|
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>
|
|
When lock ranking is enabled, we can now assert that lock preconditions
are met by checking that the caller holds required locks on function
entry.
This change adds the infrastructure to add assertions. Actual assertions
will be added for various locks in subsequent changes.
Some functions are protected by locks that are not directly accessible
in the function. In that case, we can use assertRankHeld to check that
any lock with the rank is held. This is less precise, but it avoids
requiring passing the lock into the functions.
Updates #40677
Change-Id: I843c6874867f975e90a063f087b6e2ffc147877b
Reviewed-on: https://go-review.googlesource.com/c/go/+/245484
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
|
|
Process may crash becaues acquireLockRank and releaseLockRank may
be called in nosplit context. With optimizations and inlining
disabled, these functions won't get inlined or have their morestack
calls eliminated.
Nosplit is not strictly required for lockWithRank, unlockWithRank
and lockWithRankMayAcquire, just keep consistency with lockrank_on.go
here.
Fixes #40843
Change-Id: I5824119f98a1da66d767cdb9a60dffe768f13c81
GitHub-Last-Rev: 38fd3ccf6ea03b670c7561c060ccdbccc42fff40
GitHub-Pull-Request: golang/go#40844
Reviewed-on: https://go-review.googlesource.com/c/go/+/248878
Reviewed-by: Dan Scales <danscales@google.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
|
|
For #38029
Change-Id: I71de2b66c1de617d32c46d4f2c1866f9ff1756ec
Reviewed-on: https://go-review.googlesource.com/c/go/+/244631
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
|
|
I added routines that can acquire/release a particular rank without
acquiring/releasing an associated lock. I added lockRankGscan as a rank
for acquiring/releasing the Gscan bit.
castogscanstatus() and casGtoPreemptScan() are acquires of the Gscan
bit. casfrom_Gscanstatus() is a release of the Gscan bit. casgstatus()
is like an acquire and release of the Gscan bit, since it will wait if
Gscan bit is currently set.
We have a cycle between hchan and Gscan. The acquisition of Gscan and
then hchan only happens in syncadjustsudogs() when the G is suspended,
so the main normal ordering (get hchan, then get Gscan) can't be
happening. So, I added a new rank lockRankHchanLeaf that is used when
acquiring hchan locks in syncadjustsudogs. This ranking is set so no
other locks can be acquired except other hchan locks.
Fixes #38922
Change-Id: I58ce526a74ba856cb42078f7b9901f2832e1d45c
Reviewed-on: https://go-review.googlesource.com/c/go/+/228417
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
|
|
I took some of the infrastructure from Austin's lock logging CR
https://go-review.googlesource.com/c/go/+/192704 (with deadlock
detection from the logs), and developed a setup to give static lock
ranking for runtime locks.
Static lock ranking establishes a documented total ordering among locks,
and then reports an error if the total order is violated. This can
happen if a deadlock happens (by acquiring a sequence of locks in
different orders), or if just one side of a possible deadlock happens.
Lock ordering deadlocks cannot happen as long as the lock ordering is
followed.
Along the way, I found a deadlock involving the new timer code, which Ian fixed
via https://go-review.googlesource.com/c/go/+/207348, as well as two other
potential deadlocks.
See the constants at the top of runtime/lockrank.go to show the static
lock ranking that I ended up with, along with some comments. This is
great documentation of the current intended lock ordering when acquiring
multiple locks in the runtime.
I also added an array lockPartialOrder[] which shows and enforces the
current partial ordering among locks (which is embedded within the total
ordering). This is more specific about the dependencies among locks.
I don't try to check the ranking within a lock class with multiple locks
that can be acquired at the same time (i.e. check the ranking when
multiple hchan locks are acquired).
Currently, I am doing a lockInit() call to set the lock rank of most
locks. Any lock that is not otherwise initialized is assumed to be a
leaf lock (a very high rank lock), so that eliminates the need to do
anything for a bunch of locks (including all architecture-dependent
locks). For two locks, root.lock and notifyList.lock (only in the
runtime/sema.go file), it is not as easy to do lock initialization, so
instead, I am passing the lock rank with the lock calls.
For Windows compilation, I needed to increase the StackGuard size from
896 to 928 because of the new lock-rank checking functions.
Checking of the static lock ranking is enabled by setting
GOEXPERIMENT=staticlockranking before doing a run.
To make sure that the static lock ranking code has no overhead in memory
or CPU when not enabled by GOEXPERIMENT, I changed 'go build/install' so
that it defines a build tag (with the same name) whenever any experiment
has been baked into the toolchain (by checking Expstring()). This allows
me to avoid increasing the size of the 'mutex' type when static lock
ranking is not enabled.
Fixes #38029
Change-Id: I154217ff307c47051f8dae9c2a03b53081acd83a
Reviewed-on: https://go-review.googlesource.com/c/go/+/207619
Reviewed-by: Dan Scales <danscales@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
|
