Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: lock held checking #40677

Open
prattmic opened this issue Aug 10, 2020 · 9 comments
Open

runtime: lock held checking #40677

prattmic opened this issue Aug 10, 2020 · 9 comments
Assignees
Labels
Milestone

Comments

@prattmic
Copy link
Member

@prattmic prattmic commented Aug 10, 2020

As an extension of @danscales' work on lock ordering, we can extend the runtime to check that functions hold their prerequisite locks when staticlockranking is enabled.

@prattmic prattmic added this to the Go1.16 milestone Aug 10, 2020
@prattmic prattmic self-assigned this Aug 10, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 14, 2020

Change https://golang.org/cl/248577 mentions this issue: WIP: runtime: add world-stopped assertions

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 14, 2020

Change https://golang.org/cl/245484 mentions this issue: WORK IN PROGRESS: runtime: check held locks with staticlockranking

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 24, 2020

Change https://golang.org/cl/250261 mentions this issue: runtime: add sched.lock assertions

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 24, 2020

Change https://golang.org/cl/250262 mentions this issue: WIP: runtime: add heap lock assertions

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 24, 2020

Change https://golang.org/cl/250263 mentions this issue: WIP: runtime: add channel lock assertions

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 24, 2020

Change https://golang.org/cl/250264 mentions this issue: WIP: runtime: add itab lock assertions

gopherbot pushed a commit that referenced this issue Sep 22, 2020
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>
gopherbot pushed a commit that referenced this issue Sep 22, 2020
Functions that require holding sched.lock now have an assertion.

A few places with missing locks have been fixed in this CL:

Additionally, locking is added around the call to procresize in
schedinit. This doesn't technically need a lock since the program is
still starting (thus no concurrency) when this is called, but lock held
checking doesn't know that.

Updates #40677

Change-Id: I198d3cbaa727f7088e4d55ba8fa989cf1ee8f9cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/250261
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>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 30, 2020

Change https://golang.org/cl/266718 mentions this issue: runtime: drop systemstack from lock assertions

gopherbot pushed a commit that referenced this issue Oct 30, 2020
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>
gopherbot pushed a commit that referenced this issue Oct 30, 2020
Some functions that required holding the heap lock _or_ world stop have
been simplified to simply requiring the heap lock. This is conceptually
simpler and taking the heap lock during world stop is guaranteed to not
contend. This was only done on functions already called on the
systemstack to avoid too many extra systemstack calls in GC.

Updates #40677

Change-Id: I15aa1dadcdd1a81aac3d2a9ecad6e7d0377befdc
Reviewed-on: https://go-review.googlesource.com/c/go/+/250262
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>
gopherbot pushed a commit that referenced this issue Oct 30, 2020
We use systemstack on the locking path to avoid stack splits which could
cause locks to be recorded out of order (see comment on lockWithRank).

This concern is irrelevant on lock assertions, where we simply need to
see if a lock is held and don't care if another is taken in the
meantime. Thus we can simply drop these unless we actually need to
crash.

Updates #40677

Change-Id: I85d730913a59867753ee1ed0386f8c5efda5c432
Reviewed-on: https://go-review.googlesource.com/c/go/+/266718
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>
@aclements
Copy link
Member

@aclements aclements commented Dec 8, 2020

A bunch of this is landed in 1.16, but there are still a few WIP CLs to add more assertions. Bumping to 1.17 for those.

@aclements aclements removed this from the Go1.16 milestone Dec 8, 2020
@aclements aclements added this to the Go1.17 milestone Dec 8, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 30, 2021

Are we doing anything here for 1.17? Thanks.

@prattmic prattmic removed this from the Go1.17 milestone May 3, 2021
@prattmic prattmic added this to the Backlog milestone May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants