runtime: always mark span when marking an object

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 <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

diff --git a/src/runtime/malloc.go b/src/runtime/malloc.go
index 77a5a387687efa6f3671c08ec34379694514f4a9..eaf8db72208cdc21d7077ccdbfe8302e7eb8a835 100644 (file)
--- 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 dafb4634b4204b1e2b4374b029a5ca13404aed47..fe988c46d9c46ad302127a0a8358bdd4337d913d 100644 (file)
--- 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 f444452bab502abe00a0a9d1adaa8dfd441161a7..632769c11450da7324e45ca3fd084b7fc6752a53 100644 (file)
--- 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