From d183253572461eb0cfa46b847b4abd966deb39df Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Wed, 13 Nov 2019 16:56:50 +0000 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Cherry Zhang --- src/runtime/mgcscavenge.go | 21 ++++++++++++++------- src/runtime/mgcscavenge_test.go | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/runtime/mgcscavenge.go b/src/runtime/mgcscavenge.go index db7c8121dc..b3f9cca10d 100644 --- 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 b29a4d796a..518d5ab27a 100644 --- 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 -- 2.50.0