From 886caba73ca3d895ecb8f17ea6866b34f2f7e8c1 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Fri, 5 Jun 2020 22:01:25 -0400 Subject: [PATCH] runtime: always mark span when marking an object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page sweeper depends on spans being marked if any object in the span is marked, but currently only greyobject does this. gcmarknewobject and wbBufFlush1 also mark objects, but neither set span marks. As a result, if there are live objects on a span, but they're all marked via allocation or write barriers, then the span itself won't be marked and the page reclaimer will free the span, ultimately leading to memory corruption when the memory for those live allocations gets reused. Fix this by making gcmarknewobject and wbBufFlush1 also mark pages. No test because I have no idea how to reliably (or even unreliably) trigger this. Fixes #39432. Performance is a wash or very slightly worse. I benchmarked the gcmarknewobject and wbBufFlush1 changes independently and both showed a slight performance improvement, so I'm going to call this noise. name old time/op new time/op delta BiogoIgor 15.9s ± 2% 15.9s ± 2% ~ (p=0.758 n=25+25) BiogoKrishna 15.7s ± 3% 15.7s ± 3% ~ (p=0.382 n=21+21) BleveIndexBatch100 4.94s ± 3% 5.07s ± 4% +2.63% (p=0.000 n=25+25) CompileTemplate 204ms ± 1% 205ms ± 1% +0.43% (p=0.000 n=21+23) CompileUnicode 77.8ms ± 1% 78.1ms ± 1% ~ (p=0.130 n=23+23) CompileGoTypes 731ms ± 1% 733ms ± 1% +0.30% (p=0.006 n=22+22) CompileCompiler 3.64s ± 2% 3.65s ± 3% ~ (p=0.179 n=24+25) CompileSSA 8.44s ± 1% 8.46s ± 1% +0.30% (p=0.003 n=22+23) CompileFlate 132ms ± 1% 133ms ± 1% ~ (p=0.098 n=22+22) CompileGoParser 164ms ± 1% 164ms ± 1% +0.37% (p=0.000 n=21+23) CompileReflect 455ms ± 1% 457ms ± 2% +0.50% (p=0.002 n=20+22) CompileTar 182ms ± 2% 182ms ± 1% ~ (p=0.382 n=22+22) CompileXML 245ms ± 3% 245ms ± 1% ~ (p=0.070 n=21+23) CompileStdCmd 16.5s ± 2% 16.5s ± 3% ~ (p=0.486 n=23+23) FoglemanFauxGLRenderRotateBoat 12.9s ± 1% 13.0s ± 1% +0.97% (p=0.000 n=21+24) FoglemanPathTraceRenderGopherIter1 18.6s ± 1% 18.7s ± 0% ~ (p=0.083 n=23+24) GopherLuaKNucleotide 28.4s ± 1% 29.3s ± 1% +2.84% (p=0.000 n=25+25) MarkdownRenderXHTML 252ms ± 0% 251ms ± 1% -0.50% (p=0.000 n=23+24) Tile38WithinCircle100kmRequest 516µs ± 2% 516µs ± 2% ~ (p=0.763 n=24+25) Tile38IntersectsCircle100kmRequest 689µs ± 2% 689µs ± 2% ~ (p=0.617 n=24+24) Tile38KNearestLimit100Request 608µs ± 1% 606µs ± 2% -0.35% (p=0.030 n=19+22) [Geo mean] 522ms 524ms +0.41% https://perf.golang.org/search?q=upload:20200606.4 Change-Id: I8b331f310dbfaba0468035f207467c8403005bf5 Reviewed-on: https://go-review.googlesource.com/c/go/+/236817 Run-TryBot: Austin Clements TryBot-Result: Gobot Gobot Reviewed-by: Michael Knyszek --- src/runtime/malloc.go | 20 ++++++++++---------- src/runtime/mgcmark.go | 14 ++++++++++++-- src/runtime/mwbbuf.go | 7 +++++++ 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/runtime/malloc.go b/src/runtime/malloc.go index 77a5a387687ef..eaf8db72208cd 100644 --- a/src/runtime/malloc.go +++ b/src/runtime/malloc.go @@ -976,6 +976,7 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { throw("malloc called with no P") } } + var span *mspan var x unsafe.Pointer noscan := typ == nil || typ.ptrdata == 0 if size <= maxSmallSize { @@ -1028,10 +1029,10 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { return x } // Allocate a new maxTinySize block. - span := c.alloc[tinySpanClass] + span = c.alloc[tinySpanClass] v := nextFreeFast(span) if v == 0 { - v, _, shouldhelpgc = c.nextFree(tinySpanClass) + v, span, shouldhelpgc = c.nextFree(tinySpanClass) } x = unsafe.Pointer(v) (*[2]uint64)(x)[0] = 0 @@ -1052,7 +1053,7 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { } size = uintptr(class_to_size[sizeclass]) spc := makeSpanClass(sizeclass, noscan) - span := c.alloc[spc] + span = c.alloc[spc] v := nextFreeFast(span) if v == 0 { v, span, shouldhelpgc = c.nextFree(spc) @@ -1063,15 +1064,14 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { } } } else { - var s *mspan shouldhelpgc = true systemstack(func() { - s = largeAlloc(size, needzero, noscan) + span = largeAlloc(size, needzero, noscan) }) - s.freeindex = 1 - s.allocCount = 1 - x = unsafe.Pointer(s.base()) - size = s.elemsize + span.freeindex = 1 + span.allocCount = 1 + x = unsafe.Pointer(span.base()) + size = span.elemsize } var scanSize uintptr @@ -1112,7 +1112,7 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { // This may be racing with GC so do it atomically if there can be // a race marking the bit. if gcphase != _GCoff { - gcmarknewobject(uintptr(x), size, scanSize) + gcmarknewobject(span, uintptr(x), size, scanSize) } if raceenabled { diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go index dafb4634b4204..fe988c46d9c46 100644 --- a/src/runtime/mgcmark.go +++ b/src/runtime/mgcmark.go @@ -1627,11 +1627,21 @@ func gcDumpObject(label string, obj, off uintptr) { // //go:nowritebarrier //go:nosplit -func gcmarknewobject(obj, size, scanSize uintptr) { +func gcmarknewobject(span *mspan, obj, size, scanSize uintptr) { if useCheckmark { // The world should be stopped so this should not happen. throw("gcmarknewobject called while doing checkmark") } - markBitsForAddr(obj).setMarked() + + // Mark object. + objIndex := span.objIndex(obj) + span.markBitsForIndex(objIndex).setMarked() + + // Mark span. + arena, pageIdx, pageMask := pageIndexOf(span.base()) + if arena.pageMarks[pageIdx]&pageMask == 0 { + atomic.Or8(&arena.pageMarks[pageIdx], pageMask) + } + gcw := &getg().m.p.ptr().gcw gcw.bytesMarked += uint64(size) gcw.scanWork += int64(scanSize) diff --git a/src/runtime/mwbbuf.go b/src/runtime/mwbbuf.go index f444452bab502..632769c11450d 100644 --- a/src/runtime/mwbbuf.go +++ b/src/runtime/mwbbuf.go @@ -296,6 +296,13 @@ func wbBufFlush1(_p_ *p) { continue } mbits.setMarked() + + // Mark span. + arena, pageIdx, pageMask := pageIndexOf(span.base()) + if arena.pageMarks[pageIdx]&pageMask == 0 { + atomic.Or8(&arena.pageMarks[pageIdx], pageMask) + } + if span.spanclass.noscan() { gcw.bytesMarked += uint64(span.elemsize) continue