backport: runtime: put ReadMemStats debug assertions behind a double-check mode
This commit is contained in:
EulerOSWander
parent
d399424f55
commit
8dddbed90e
@ -0,0 +1,213 @@
|
||||
From 398023279e53f4e847d598afa41c2d3d163baa94 Mon Sep 17 00:00:00 2001
|
||||
From: Michael Anthony Knyszek <mknyszek@google.com>
|
||||
Date: Mon, 27 Nov 2023 22:27:32 +0000
|
||||
Subject: [PATCH 09/20] [release-branch.go1.21] runtime: put ReadMemStats debug
|
||||
assertions behind a double-check mode
|
||||
MIME-Version: 1.0
|
||||
Content-Type: text/plain; charset=UTF-8
|
||||
Content-Transfer-Encoding: 8bit
|
||||
|
||||
Conflict:NA
|
||||
Reference:https://go-review.googlesource.com/c/go/+/545277
|
||||
ReadMemStats has a few assertions it makes about the consistency of the
|
||||
stats it's about to produce. Specifically, how those stats line up with
|
||||
runtime-internal stats. These checks are generally useful, but crashing
|
||||
just because some stats are wrong is a heavy price to pay.
|
||||
|
||||
For a long time this wasn't a problem, but very recently it became a
|
||||
real problem. It turns out that there's real benign skew that can happen
|
||||
wherein sysmon (which doesn't synchronize with a STW) generates a trace
|
||||
event when tracing is enabled, and may mutate some stats while
|
||||
ReadMemStats is running its checks.
|
||||
|
||||
Fix this by synchronizing with both sysmon and the tracer. This is a bit
|
||||
heavy-handed, but better that than false positives.
|
||||
|
||||
Also, put the checks behind a debug mode. We want to reduce the risk of
|
||||
backporting this change, and again, it's not great to crash just because
|
||||
user-facing stats are off. Still, enable this debug mode during the
|
||||
runtime tests so we don't lose quite as much coverage from disabling
|
||||
these checks by default.
|
||||
|
||||
For #64401.
|
||||
Fixes #64410.
|
||||
|
||||
Change-Id: I9adb3e5c7161d207648d07373a11da8a5f0fda9a
|
||||
Reviewed-on: https://go-review.googlesource.com/c/go/+/545277
|
||||
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
||||
Reviewed-by: Michael Pratt <mpratt@google.com>
|
||||
Reviewed-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
|
||||
(cherry picked from commit b2efd1de97402ec4b8fb4e9e0ec29c8e49e8e200)
|
||||
Reviewed-on: https://go-review.googlesource.com/c/go/+/545557
|
||||
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
|
||||
TryBot-Bypass: Matthew Dempsky <mdempsky@google.com>
|
||||
---
|
||||
src/runtime/export_test.go | 2 +
|
||||
src/runtime/gc_test.go | 5 ++
|
||||
src/runtime/mstats.go | 114 +++++++++++++++++++++----------------
|
||||
3 files changed, 71 insertions(+), 50 deletions(-)
|
||||
|
||||
diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go
|
||||
index f7ce5033f587..a740993c5878 100644
|
||||
--- a/src/runtime/export_test.go
|
||||
+++ b/src/runtime/export_test.go
|
||||
@@ -436,6 +436,8 @@ func ReadMetricsSlow(memStats *MemStats, samplesp unsafe.Pointer, len, cap int)
|
||||
startTheWorld()
|
||||
}
|
||||
|
||||
+var DoubleCheckReadMemStats = &doubleCheckReadMemStats
|
||||
+
|
||||
// ReadMemStatsSlow returns both the runtime-computed MemStats and
|
||||
// MemStats accumulated by scanning the heap.
|
||||
func ReadMemStatsSlow() (base, slow MemStats) {
|
||||
diff --git a/src/runtime/gc_test.go b/src/runtime/gc_test.go
|
||||
index bd01e3610300..9302ea32c3f1 100644
|
||||
--- a/src/runtime/gc_test.go
|
||||
+++ b/src/runtime/gc_test.go
|
||||
@@ -570,6 +570,11 @@ func TestPageAccounting(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
+func init() {
|
||||
+ // Enable ReadMemStats' double-check mode.
|
||||
+ *runtime.DoubleCheckReadMemStats = true
|
||||
+}
|
||||
+
|
||||
func TestReadMemStats(t *testing.T) {
|
||||
base, slow := runtime.ReadMemStatsSlow()
|
||||
if base != slow {
|
||||
diff --git a/src/runtime/mstats.go b/src/runtime/mstats.go
|
||||
index 9cdc56513719..308bed67d7b3 100644
|
||||
--- a/src/runtime/mstats.go
|
||||
+++ b/src/runtime/mstats.go
|
||||
@@ -367,6 +367,11 @@ func ReadMemStats(m *MemStats) {
|
||||
startTheWorld()
|
||||
}
|
||||
|
||||
+// doubleCheckReadMemStats controls a double-check mode for ReadMemStats that
|
||||
+// ensures consistency between the values that ReadMemStats is using and the
|
||||
+// runtime-internal stats.
|
||||
+var doubleCheckReadMemStats = false
|
||||
+
|
||||
// readmemstats_m populates stats for internal runtime values.
|
||||
//
|
||||
// The world must be stopped.
|
||||
@@ -441,56 +446,65 @@ func readmemstats_m(stats *MemStats) {
|
||||
|
||||
heapGoal := gcController.heapGoal()
|
||||
|
||||
- // The world is stopped, so the consistent stats (after aggregation)
|
||||
- // should be identical to some combination of memstats. In particular:
|
||||
- //
|
||||
- // * memstats.heapInUse == inHeap
|
||||
- // * memstats.heapReleased == released
|
||||
- // * memstats.heapInUse + memstats.heapFree == committed - inStacks - inWorkBufs - inPtrScalarBits
|
||||
- // * memstats.totalAlloc == totalAlloc
|
||||
- // * memstats.totalFree == totalFree
|
||||
- //
|
||||
- // Check if that's actually true.
|
||||
- //
|
||||
- // TODO(mknyszek): Maybe don't throw here. It would be bad if a
|
||||
- // bug in otherwise benign accounting caused the whole application
|
||||
- // to crash.
|
||||
- if gcController.heapInUse.load() != uint64(consStats.inHeap) {
|
||||
- print("runtime: heapInUse=", gcController.heapInUse.load(), "\n")
|
||||
- print("runtime: consistent value=", consStats.inHeap, "\n")
|
||||
- throw("heapInUse and consistent stats are not equal")
|
||||
- }
|
||||
- if gcController.heapReleased.load() != uint64(consStats.released) {
|
||||
- print("runtime: heapReleased=", gcController.heapReleased.load(), "\n")
|
||||
- print("runtime: consistent value=", consStats.released, "\n")
|
||||
- throw("heapReleased and consistent stats are not equal")
|
||||
- }
|
||||
- heapRetained := gcController.heapInUse.load() + gcController.heapFree.load()
|
||||
- consRetained := uint64(consStats.committed - consStats.inStacks - consStats.inWorkBufs - consStats.inPtrScalarBits)
|
||||
- if heapRetained != consRetained {
|
||||
- print("runtime: global value=", heapRetained, "\n")
|
||||
- print("runtime: consistent value=", consRetained, "\n")
|
||||
- throw("measures of the retained heap are not equal")
|
||||
- }
|
||||
- if gcController.totalAlloc.Load() != totalAlloc {
|
||||
- print("runtime: totalAlloc=", gcController.totalAlloc.Load(), "\n")
|
||||
- print("runtime: consistent value=", totalAlloc, "\n")
|
||||
- throw("totalAlloc and consistent stats are not equal")
|
||||
- }
|
||||
- if gcController.totalFree.Load() != totalFree {
|
||||
- print("runtime: totalFree=", gcController.totalFree.Load(), "\n")
|
||||
- print("runtime: consistent value=", totalFree, "\n")
|
||||
- throw("totalFree and consistent stats are not equal")
|
||||
- }
|
||||
- // Also check that mappedReady lines up with totalMapped - released.
|
||||
- // This isn't really the same type of "make sure consistent stats line up" situation,
|
||||
- // but this is an opportune time to check.
|
||||
- if gcController.mappedReady.Load() != totalMapped-uint64(consStats.released) {
|
||||
- print("runtime: mappedReady=", gcController.mappedReady.Load(), "\n")
|
||||
- print("runtime: totalMapped=", totalMapped, "\n")
|
||||
- print("runtime: released=", uint64(consStats.released), "\n")
|
||||
- print("runtime: totalMapped-released=", totalMapped-uint64(consStats.released), "\n")
|
||||
- throw("mappedReady and other memstats are not equal")
|
||||
+ if doubleCheckReadMemStats {
|
||||
+ // Only check this if we're debugging. It would be bad to crash an application
|
||||
+ // just because the debugging stats are wrong. We mostly rely on tests to catch
|
||||
+ // these issues, and we enable the double check mode for tests.
|
||||
+ //
|
||||
+ // The world is stopped, so the consistent stats (after aggregation)
|
||||
+ // should be identical to some combination of memstats. In particular:
|
||||
+ //
|
||||
+ // * memstats.heapInUse == inHeap
|
||||
+ // * memstats.heapReleased == released
|
||||
+ // * memstats.heapInUse + memstats.heapFree == committed - inStacks - inWorkBufs - inPtrScalarBits
|
||||
+ // * memstats.totalAlloc == totalAlloc
|
||||
+ // * memstats.totalFree == totalFree
|
||||
+ //
|
||||
+ // Check if that's actually true.
|
||||
+ //
|
||||
+ // Prevent sysmon and the tracer from skewing the stats since they can
|
||||
+ // act without synchronizing with a STW. See #64401.
|
||||
+ lock(&sched.sysmonlock)
|
||||
+ lock(&trace.lock)
|
||||
+ if gcController.heapInUse.load() != uint64(consStats.inHeap) {
|
||||
+ print("runtime: heapInUse=", gcController.heapInUse.load(), "\n")
|
||||
+ print("runtime: consistent value=", consStats.inHeap, "\n")
|
||||
+ throw("heapInUse and consistent stats are not equal")
|
||||
+ }
|
||||
+ if gcController.heapReleased.load() != uint64(consStats.released) {
|
||||
+ print("runtime: heapReleased=", gcController.heapReleased.load(), "\n")
|
||||
+ print("runtime: consistent value=", consStats.released, "\n")
|
||||
+ throw("heapReleased and consistent stats are not equal")
|
||||
+ }
|
||||
+ heapRetained := gcController.heapInUse.load() + gcController.heapFree.load()
|
||||
+ consRetained := uint64(consStats.committed - consStats.inStacks - consStats.inWorkBufs - consStats.inPtrScalarBits)
|
||||
+ if heapRetained != consRetained {
|
||||
+ print("runtime: global value=", heapRetained, "\n")
|
||||
+ print("runtime: consistent value=", consRetained, "\n")
|
||||
+ throw("measures of the retained heap are not equal")
|
||||
+ }
|
||||
+ if gcController.totalAlloc.Load() != totalAlloc {
|
||||
+ print("runtime: totalAlloc=", gcController.totalAlloc.Load(), "\n")
|
||||
+ print("runtime: consistent value=", totalAlloc, "\n")
|
||||
+ throw("totalAlloc and consistent stats are not equal")
|
||||
+ }
|
||||
+ if gcController.totalFree.Load() != totalFree {
|
||||
+ print("runtime: totalFree=", gcController.totalFree.Load(), "\n")
|
||||
+ print("runtime: consistent value=", totalFree, "\n")
|
||||
+ throw("totalFree and consistent stats are not equal")
|
||||
+ }
|
||||
+ // Also check that mappedReady lines up with totalMapped - released.
|
||||
+ // This isn't really the same type of "make sure consistent stats line up" situation,
|
||||
+ // but this is an opportune time to check.
|
||||
+ if gcController.mappedReady.Load() != totalMapped-uint64(consStats.released) {
|
||||
+ print("runtime: mappedReady=", gcController.mappedReady.Load(), "\n")
|
||||
+ print("runtime: totalMapped=", totalMapped, "\n")
|
||||
+ print("runtime: released=", uint64(consStats.released), "\n")
|
||||
+ print("runtime: totalMapped-released=", totalMapped-uint64(consStats.released), "\n")
|
||||
+ throw("mappedReady and other memstats are not equal")
|
||||
+ }
|
||||
+ unlock(&trace.lock)
|
||||
+ unlock(&sched.sysmonlock)
|
||||
}
|
||||
|
||||
// We've calculated all the values we need. Now, populate stats.
|
||||
--
|
||||
2.33.0
|
||||
|
||||
@ -66,7 +66,7 @@
|
||||
|
||||
Name: golang
|
||||
Version: 1.21.4
|
||||
Release: 26
|
||||
Release: 27
|
||||
Summary: The Go Programming Language
|
||||
License: BSD and Public Domain
|
||||
URL: https://golang.org/
|
||||
@ -145,6 +145,7 @@ Patch6021: backport-0021-CVE-2024-34155-track-depth-in-nested-element-lists.patc
|
||||
Patch6022: backport-0022-release-branch.go1.21-fix-CVE-2024-34156.patch
|
||||
Patch6023: backport-0023-go-build-constraint-add-parsing-limits.patch
|
||||
Patch6024: backport-0024-release-branch.go1.21-runtime-add-the-disablethp-GOD.patch
|
||||
Patch6025: backport-0025-release-branch.go1.21-runtime-put-ReadMemStats-debug.patch
|
||||
|
||||
ExclusiveArch: %{golang_arches}
|
||||
|
||||
@ -383,6 +384,12 @@ fi
|
||||
%files devel -f go-tests.list -f go-misc.list -f go-src.list
|
||||
|
||||
%changelog
|
||||
* Thu Dec 05 2024 hewenliang <314264452@qq.com> - 1.21.4-27
|
||||
- Type:bugfix
|
||||
- CVE:NA
|
||||
- SUG:NA
|
||||
- DESC:put ReadMemStats debug assertions behind a double-check mode
|
||||
|
||||
* Mon Nov 18 2024 Vanient <xiadanni1@huawei.com> - 1.21.4-26
|
||||
- runtime: add the disablethp GODEBUG setting
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user