diff options
author | Austin Clements <austin@google.com> | 2019-10-23 11:25:38 -0400 |
---|---|---|
committer | Austin Clements <austin@google.com> | 2019-10-31 17:09:50 +0000 |
commit | 7de15e362b0bc4ba83c8ca4d7cadc319c99db65a (patch) | |
tree | ced17246a0d745f32f1dbee98338197b5f8c7f10 /src/runtime/mgcmark.go | |
parent | a9b37ae02604e03d2356b6143679d2a71bdd32a7 (diff) | |
download | go-7de15e362b0bc4ba83c8ca4d7cadc319c99db65a.tar.gz go-7de15e362b0bc4ba83c8ca4d7cadc319c99db65a.zip |
runtime: atomically set span state and use as publication barrier
When everything is working correctly, any pointer the garbage
collector encounters can only point into a fully initialized heap
span, since the span must have been initialized before that pointer
could escape the heap allocator and become visible to the GC.
However, in various cases, we try to be defensive against bad
pointers. In findObject, this is just a sanity check: we never expect
to find a bad pointer, but programming errors can lead to them. In
spanOfHeap, we don't necessarily trust the pointer and we're trying to
check if it really does point to the heap, though it should always
point to something. Conservative scanning takes this to a new level,
since it can only guess that a word may be a pointer and verify this.
In all of these cases, we have a problem that the span lookup and
check can race with span initialization, since the span becomes
visible to lookups before it's fully initialized.
Furthermore, we're about to start initializing the span without the
heap lock held, which is going to introduce races where accesses were
previously protected by the heap lock.
To address this, this CL makes accesses to mspan.state atomic, and
ensures that the span is fully initialized before setting the state to
mSpanInUse. All loads are now atomic, and in any case where we don't
trust the pointer, it first atomically loads the span state and checks
that it's mSpanInUse, after which it will have synchronized with span
initialization and can safely check the other span fields.
For #10958, #24543, but a good fix in general.
Change-Id: I518b7c63555b02064b98aa5f802c92b758fef853
Reviewed-on: https://go-review.googlesource.com/c/go/+/203286
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Diffstat (limited to 'src/runtime/mgcmark.go')
-rw-r--r-- | src/runtime/mgcmark.go | 16 |
1 files changed, 9 insertions, 7 deletions
diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go index 338983424c..2987d3572b 100644 --- a/src/runtime/mgcmark.go +++ b/src/runtime/mgcmark.go @@ -321,7 +321,9 @@ func markrootSpans(gcw *gcWork, shard int) { // entered the scan phase, so addfinalizer will have ensured // the above invariants for them. for _, s := range spans { - if s.state != mSpanInUse { + // This is racing with spans being initialized, so + // check the state carefully. + if s.state.get() != mSpanInUse { continue } // Check that this span was swept (it may be cached or uncached). @@ -1310,15 +1312,15 @@ func gcDumpObject(label string, obj, off uintptr) { return } print(" s.base()=", hex(s.base()), " s.limit=", hex(s.limit), " s.spanclass=", s.spanclass, " s.elemsize=", s.elemsize, " s.state=") - if 0 <= s.state && int(s.state) < len(mSpanStateNames) { - print(mSpanStateNames[s.state], "\n") + if state := s.state.get(); 0 <= state && int(state) < len(mSpanStateNames) { + print(mSpanStateNames[state], "\n") } else { - print("unknown(", s.state, ")\n") + print("unknown(", state, ")\n") } skipped := false size := s.elemsize - if s.state == mSpanManual && size == 0 { + if s.state.get() == mSpanManual && size == 0 { // We're printing something from a stack frame. We // don't know how big it is, so just show up to an // including off. @@ -1406,7 +1408,7 @@ var useCheckmark = false func initCheckmarks() { useCheckmark = true for _, s := range mheap_.allspans { - if s.state == mSpanInUse { + if s.state.get() == mSpanInUse { heapBitsForAddr(s.base()).initCheckmarkSpan(s.layout()) } } @@ -1415,7 +1417,7 @@ func initCheckmarks() { func clearCheckmarks() { useCheckmark = false for _, s := range mheap_.allspans { - if s.state == mSpanInUse { + if s.state.get() == mSpanInUse { heapBitsForAddr(s.base()).clearCheckmarkSpan(s.layout()) } } |