From fc07039e2339a80f53e7db5e214b5be504bc1df6 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Tue, 28 Jun 2022 15:17:12 -0400 Subject: [release-branch.go1.17] runtime: add race annotations to metricsSema metricsSema protects the metrics map. The map implementation is race instrumented regardless of which package is it called from. semacquire/semrelease are not automatically race instrumented, so we can trigger race false positives without manually annotating our lock acquire and release. See similar instrumentation on trace.shutdownSema and reflectOffs.lock. Fixes #53589. For #53542. Change-Id: Ia3fd239ac860e037d09c7cb9c4ad267391e70705 Reviewed-on: https://go-review.googlesource.com/c/go/+/414517 Run-TryBot: Michael Pratt TryBot-Result: Gopher Robot Reviewed-by: Cherry Mui (cherry picked from commit d6481d5b9662b29453004204746945a93a6b4eb2) Reviewed-on: https://go-review.googlesource.com/c/go/+/415196 --- src/runtime/export_test.go | 4 ++-- src/runtime/metrics.go | 33 +++++++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go index 03ccfe10a8..3cee4dec57 100644 --- a/src/runtime/export_test.go +++ b/src/runtime/export_test.go @@ -321,9 +321,9 @@ func ReadMetricsSlow(memStats *MemStats, samplesp unsafe.Pointer, len, cap int) // Initialize the metrics beforehand because this could // allocate and skew the stats. - semacquire(&metricsSema) + metricsLock() initMetrics() - semrelease(&metricsSema) + metricsUnlock() systemstack(func() { // Read memstats first. It's going to flush diff --git a/src/runtime/metrics.go b/src/runtime/metrics.go index 4af2b0aef9..922dd2f814 100644 --- a/src/runtime/metrics.go +++ b/src/runtime/metrics.go @@ -12,9 +12,12 @@ import ( ) var ( - // metrics is a map of runtime/metrics keys to - // data used by the runtime to sample each metric's - // value. + // metrics is a map of runtime/metrics keys to data used by the runtime + // to sample each metric's value. metricsInit indicates it has been + // initialized. + // + // These fields are protected by metricsSema which should be + // locked/unlocked with metricsLock() / metricsUnlock(). metricsSema uint32 = 1 metricsInit bool metrics map[string]metricData @@ -34,6 +37,23 @@ type metricData struct { compute func(in *statAggregate, out *metricValue) } +func metricsLock() { + // Acquire the metricsSema but with handoff. Operations are typically + // expensive enough that queueing up goroutines and handing off between + // them will be noticeably better-behaved. + semacquire1(&metricsSema, true, 0, 0) + if raceenabled { + raceacquire(unsafe.Pointer(&metricsSema)) + } +} + +func metricsUnlock() { + if raceenabled { + racerelease(unsafe.Pointer(&metricsSema)) + } + semrelease(&metricsSema) +} + // initMetrics initializes the metrics map if it hasn't been yet. // // metricsSema must be held. @@ -546,10 +566,7 @@ func readMetrics(samplesp unsafe.Pointer, len int, cap int) { sl := slice{samplesp, len, cap} samples := *(*[]metricSample)(unsafe.Pointer(&sl)) - // Acquire the metricsSema but with handoff. This operation - // is expensive enough that queueing up goroutines and handing - // off between them will be noticeably better-behaved. - semacquire1(&metricsSema, true, 0, 0) + metricsLock() // Ensure the map is initialized. initMetrics() @@ -573,5 +590,5 @@ func readMetrics(samplesp unsafe.Pointer, len int, cap int) { data.compute(&agg, &sample.value) } - semrelease(&metricsSema) + metricsUnlock() } -- cgit v1.2.3-54-g00ecf