runtime: check summary before scavenging in fast path

In scavengeOne's fast path, we currently don't check the summary for the
chunk that scavAddr points to, which means that we might accidentally
scavenge unused address space if the previous scavenge moves the
scavAddr into that space. The result of this today is a crash.

This change makes it so that scavengeOne's fast path only happens after
the check, following the comment in mpagealloc.go. It also adds a test
for this case.

Fixes #35465.
Updates #35112.

Change-Id: I861d44ee75e42a0e1f5aaec243bc449228273903
Reviewed-on: https://go-review.googlesource.com/c/go/+/206978
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

diff --git a/src/runtime/mgcscavenge.go b/src/runtime/mgcscavenge.go
index db7c8121dca7f1ed1e3f004afe7e4a5fb51dd616..b3f9cca10d06b9cd3e3d8e9519fcd6b55ae137f8 100644 (file)
--- a/src/runtime/mgcscavenge.go
+++ b/src/runtime/mgcscavenge.go
@@ -408,13 +408,20 @@ func (s *pageAlloc) scavengeOne(max uintptr, locked bool) uintptr {
        // Check the chunk containing the scav addr, starting at the addr
        // and see if there are any free and unscavenged pages.
        ci := chunkIndex(s.scavAddr)
-       base, npages := s.chunks[ci].findScavengeCandidate(chunkPageIndex(s.scavAddr), minPages, maxPages)
-
-       // If we found something, scavenge it and return!
-       if npages != 0 {
-               s.scavengeRangeLocked(ci, base, npages)
-               unlockHeap()
-               return uintptr(npages) * pageSize
+       if s.summary[len(s.summary)-1][ci].max() >= uint(minPages) {
+               // We only bother looking for a candidate if there at least
+               // minPages free pages at all. It's important that we only
+               // continue if the summary says we can because that's how
+               // we can tell if parts of the address space are unused.
+               // See the comment on s.chunks in mpagealloc.go.
+               base, npages := s.chunks[ci].findScavengeCandidate(chunkPageIndex(s.scavAddr), minPages, maxPages)
+
+               // If we found something, scavenge it and return!
+               if npages != 0 {
+                       s.scavengeRangeLocked(ci, base, npages)
+                       unlockHeap()
+                       return uintptr(npages) * pageSize
+               }
        }
        unlockHeap()
 

diff --git a/src/runtime/mgcscavenge_test.go b/src/runtime/mgcscavenge_test.go
index b29a4d796ab88db2117b7fbbd2809cbc85db3ca2..518d5ab27ae5f0748ef12ec06504ceee47efed6f 100644 (file)
--- a/src/runtime/mgcscavenge_test.go
+++ b/src/runtime/mgcscavenge_test.go
@@ -373,6 +373,25 @@ func TestPageAllocScavenge(t *testing.T) {
                                BaseChunkIdx + 1: {{0, PallocChunkPages}},
                        },
                },
+               "ScavDiscontiguous": {
+                       beforeAlloc: map[ChunkIdx][]BitRange{
+                               BaseChunkIdx:       {},
+                               BaseChunkIdx + 0xe: {},
+                       },
+                       beforeScav: map[ChunkIdx][]BitRange{
+                               BaseChunkIdx:       {{uint(minPages), PallocChunkPages - uint(2*minPages)}},
+                               BaseChunkIdx + 0xe: {{uint(2 * minPages), PallocChunkPages - uint(2*minPages)}},
+                       },
+                       expect: []test{
+                               {2 * minPages * PageSize, 2 * minPages * PageSize},
+                               {^uintptr(0), 2 * minPages * PageSize},
+                               {^uintptr(0), 0},
+                       },
+                       afterScav: map[ChunkIdx][]BitRange{
+                               BaseChunkIdx:       {{0, PallocChunkPages}},
+                               BaseChunkIdx + 0xe: {{0, PallocChunkPages}},
+                       },
+               },
        }
        for name, v := range tests {
                v := v