From 53c984d976f49b5671b11ff17f5d622572d4cf58 Mon Sep 17 00:00:00 2001 From: Richard Miller Date: Sat, 5 Dec 2020 19:53:08 +0000 Subject: runtime: skip wakep call in wakeNetPoller on Plan 9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was part of a performance improvement made by CL 232298 to reduce timer latency. On multiprocessor Plan 9 machines, it triggers memory faults often enough that the builder test suite never completes successfully. See issue #42303 for discussion. As shown by the benchmark result below, worst case latency on plan9_arm is very bad even with the wakep call in place - in the tickers-per-P=1 case, a 3ms timer is 270ms late. Skipping the wakep call and running the benchmark again shows some cases worse, some better. The performance cost doesn't seem excessive for this temporary workaround which makes the plan9_arm builders usable again. With wakep call: cpu% go test -bench Latency time goos: plan9 goarch: arm pkg: time BenchmarkParallelTimerLatency-4 100 10985859 avg-late-ns 18630963 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=1-4 195 270294688 avg-late-ns 542057670 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=2-4 234 182452000 avg-late-ns 423933688 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=3-4 280 193003004 avg-late-ns 408034405 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=4-4 282 132819086 avg-late-ns 313624570 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=5-4 339 71152187 avg-late-ns 189014519 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=6-4 315 26860484 avg-late-ns 101759844 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=7-4 357 19106739 avg-late-ns 59435620 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=8-4 376 7246933 avg-late-ns 38888461 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=9-4 267 40476892 avg-late-ns 205851926 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=10-4 294 87836303 avg-late-ns 252059695 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=2ms/tickers-per-P=1-4 379 4127144 avg-late-ns 10494927 max-late-ns Without wakep call: BenchmarkParallelTimerLatency-4 61 10775151 avg-late-ns 18668517 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=1-4 199 299587535 avg-late-ns 597182307 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=2-4 272 184561831 avg-late-ns 449739837 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=3-4 235 154983257 avg-late-ns 370940553 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=4-4 290 150034689 avg-late-ns 332399843 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=5-4 298 47540764 avg-late-ns 133709031 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=6-4 350 20379394 avg-late-ns 81742809 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=7-4 363 14403223 avg-late-ns 98901212 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=8-4 375 12293090 avg-late-ns 50266552 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=9-4 336 40628820 avg-late-ns 150946099 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=300µs/tickers-per-P=10-4 289 88265539 avg-late-ns 280770418 max-late-ns BenchmarkStaggeredTickerLatency/work-dur=2ms/tickers-per-P=1-4 375 8364937 avg-late-ns 22598421 max-late-ns Fixes #42303 Change-Id: I70c63cb2a2bad46950a7cd9dfc7bb32943710d32 Reviewed-on: https://go-review.googlesource.com/c/go/+/275672 Reviewed-by: David du Colombier <0intro@gmail.com> Trust: Michael Pratt --- src/runtime/proc.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src/runtime') diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 64e102fb0a..418e06932e 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -2882,7 +2882,9 @@ func wakeNetPoller(when int64) { } else { // There are no threads in the network poller, try to get // one there so it can handle new timers. - wakep() + if GOOS != "plan9" { // Temporary workaround - see issue #42303. + wakep() + } } } -- cgit v1.3 From cb95819cf6e969dc7dcc64ec7820d3995379c9f4 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Fri, 11 Dec 2020 14:14:30 -0500 Subject: runtime: detect netbsd netpoll overrun in sysmon The netbsd kernel has a bug [1] that occassionally prevents netpoll from waking with netpollBreak, which could result in missing timers for an unbounded amount of time, as netpoll can't restart with a shorter delay when an earlier timer is added. Prior to CL 232298, sysmon could detect these overrun timers and manually start an M to run them. With this fallback gone, the bug actually prevents timer execution indefinitely. As a workaround, we add back sysmon detection only for netbsd. [1] https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=50094 Updates #42515 Change-Id: I8391f5b9dabef03dd1d94c50b3b4b3bd4f889e66 Reviewed-on: https://go-review.googlesource.com/c/go/+/277332 Run-TryBot: Michael Pratt TryBot-Result: Go Bot Reviewed-by: Michael Knyszek Reviewed-by: Austin Clements Trust: Michael Pratt --- src/runtime/proc.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'src/runtime') diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 418e06932e..5adcbf07dc 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -5130,6 +5130,26 @@ func sysmon() { } } mDoFixup() + if GOOS == "netbsd" { + // netpoll is responsible for waiting for timer + // expiration, so we typically don't have to worry + // about starting an M to service timers. (Note that + // sleep for timeSleepUntil above simply ensures sysmon + // starts running again when that timer expiration may + // cause Go code to run again). + // + // However, netbsd has a kernel bug that sometimes + // misses netpollBreak wake-ups, which can lead to + // unbounded delays servicing timers. If we detect this + // overrun, then startm to get something to handle the + // timer. + // + // See issue 42515 and + // https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=50094. + if next, _ := timeSleepUntil(); next < now { + startm(nil, false) + } + } if atomic.Load(&scavenge.sysmonWake) != 0 { // Kick the scavenger awake if someone requested it. wakeScavenger() -- cgit v1.3 From 8438a5779b76620237d608282a99d17467b91f4c Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Wed, 11 Nov 2020 21:27:56 -0500 Subject: runtime: use _exit on darwin On darwin, where we use libc for syscalls, when the runtime exits, it calls libc exit function, which may call back into user code, e.g. invoking functions registered with atexit. In particular, it may call back into Go. But at this point, the Go runtime is already exiting, so this wouldn't work. On non-libc platforms we use exit syscall directly, which doesn't invoke any callbacks. Use _exit on darwin to achieve the same behavior. No test for now, as it doesn't pass on all platforms (see trybot run of PS2). May fix #42465. May fix #43294. Change-Id: Ia1ada22b5da8cb64fdd598d0541eb90e195367eb Reviewed-on: https://go-review.googlesource.com/c/go/+/269378 Trust: Cherry Zhang Run-TryBot: Cherry Zhang TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor --- src/runtime/sys_darwin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/runtime') diff --git a/src/runtime/sys_darwin.go b/src/runtime/sys_darwin.go index c89ce78012..55845bf2e5 100644 --- a/src/runtime/sys_darwin.go +++ b/src/runtime/sys_darwin.go @@ -467,7 +467,7 @@ func setNonblock(fd int32) { //go:cgo_import_dynamic libc_pthread_create pthread_create "/usr/lib/libSystem.B.dylib" //go:cgo_import_dynamic libc_pthread_self pthread_self "/usr/lib/libSystem.B.dylib" //go:cgo_import_dynamic libc_pthread_kill pthread_kill "/usr/lib/libSystem.B.dylib" -//go:cgo_import_dynamic libc_exit exit "/usr/lib/libSystem.B.dylib" +//go:cgo_import_dynamic libc_exit _exit "/usr/lib/libSystem.B.dylib" //go:cgo_import_dynamic libc_raise raise "/usr/lib/libSystem.B.dylib" //go:cgo_import_dynamic libc_open open "/usr/lib/libSystem.B.dylib" -- cgit v1.3 From 6cff874c47bdb4567f5c84bc59d93311493caefe Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Mon, 7 Dec 2020 15:11:46 +0000 Subject: runtime/metrics: add Read examples This change adds two examples of using the Read function: one that reads one metric and one that reads all metrics. Change-Id: I4940a44c9b1d65f3f7a1554e3145ff07e6492fc1 Reviewed-on: https://go-review.googlesource.com/c/go/+/275855 Run-TryBot: Michael Knyszek TryBot-Result: Go Bot Reviewed-by: Michael Pratt Trust: Michael Knyszek --- src/runtime/metrics/example_test.go | 96 +++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 src/runtime/metrics/example_test.go (limited to 'src/runtime') diff --git a/src/runtime/metrics/example_test.go b/src/runtime/metrics/example_test.go new file mode 100644 index 0000000000..cade0c38bf --- /dev/null +++ b/src/runtime/metrics/example_test.go @@ -0,0 +1,96 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package metrics_test + +import ( + "fmt" + "runtime/metrics" +) + +func ExampleRead_readingOneMetric() { + // Name of the metric we want to read. + const myMetric = "/memory/classes/heap/free:bytes" + + // Create a sample for the metric. + sample := make([]metrics.Sample, 1) + sample[0].Name = myMetric + + // Sample the metric. + metrics.Read(sample) + + // Check if the metric is actually supported. + // If it's not, the resulting value will always have + // kind KindBad. + if sample[0].Value.Kind() == metrics.KindBad { + panic(fmt.Sprintf("metric %q no longer supported", myMetric)) + } + + // Handle the result. + // + // It's OK to assume a particular Kind for a metric; + // they're guaranteed not to change. + freeBytes := sample[0].Value.Uint64() + + fmt.Printf("free but not released memory: %d\n", freeBytes) +} + +func ExampleRead_readingAllMetrics() { + // Get descriptions for all supported metrics. + descs := metrics.All() + + // Create a sample for each metric. + samples := make([]metrics.Sample, len(descs)) + for i := range samples { + samples[i].Name = descs[i].Name + } + + // Sample the metrics. Re-use the samples slice if you can! + metrics.Read(samples) + + // Iterate over all results. + for _, sample := range samples { + // Pull out the name and value. + name, value := sample.Name, sample.Value + + // Handle each sample. + switch value.Kind() { + case metrics.KindUint64: + fmt.Printf("%s: %d\n", name, value.Uint64()) + case metrics.KindFloat64: + fmt.Printf("%s: %f\n", name, value.Float64()) + case metrics.KindFloat64Histogram: + // The histogram may be quite large, so let's just pull out + // a crude estimate for the median for the sake of this example. + fmt.Printf("%s: %f\n", name, medianBucket(value.Float64Histogram())) + case metrics.KindBad: + // This should never happen because all metrics are supported + // by construction. + panic("bug in runtime/metrics package!") + default: + // This may happen as new metrics get added. + // + // The safest thing to do here is to simply log it somewhere + // as something to look into, but ignore it for now. + // In the worst case, you might temporarily miss out on a new metric. + fmt.Printf("%s: unexpected metric Kind: %v\n", name, value.Kind()) + } + } +} + +func medianBucket(h *metrics.Float64Histogram) float64 { + total := uint64(0) + for _, count := range h.Counts { + total += count + } + thresh := total / 2 + total = 0 + for i, count := range h.Counts { + total += count + if total > thresh { + return h.Buckets[i] + } + } + panic("should not happen") +} -- cgit v1.3 From 4d27c4c223ccb7de3876abbac79b58ad9579be1a Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 9 Dec 2020 15:14:59 -0500 Subject: runtime: correct error handling in several FreeBSD syscall wrappers The FreeBSD syscall convention uses the carry flag to indicate whether an error has occured. The sys_umtx_op, thr_new, and pipe2 syscall wrappers were failing to account for this convention and silently suppressing errors as a result. This commit corrects these wrappers by copying the pattern used by the other fallible syscall wrappers. Note that futexsleep1 must now explicitly ignore the ETIMEDOUT error from sys_umtx_op. Previously ETIMEDOUT was implicitly ignored because sys_umtx_op never returned an error. Fixes #43106. Change-Id: I9c422b87cf4c6d308003bf42c3b419f785578b5d Reviewed-on: https://go-review.googlesource.com/c/go/+/276892 Run-TryBot: Ian Lance Taylor Reviewed-by: Austin Clements Trust: Than McIntosh --- src/runtime/defs_freebsd_386.go | 9 +++++---- src/runtime/defs_freebsd_amd64.go | 9 +++++---- src/runtime/defs_freebsd_arm.go | 9 +++++---- src/runtime/defs_freebsd_arm64.go | 9 +++++---- src/runtime/os_freebsd.go | 3 +-- src/runtime/sys_freebsd_386.s | 6 ++++++ src/runtime/sys_freebsd_amd64.s | 6 ++++++ src/runtime/sys_freebsd_arm.s | 3 +++ src/runtime/sys_freebsd_arm64.s | 6 ++++++ 9 files changed, 42 insertions(+), 18 deletions(-) (limited to 'src/runtime') diff --git a/src/runtime/defs_freebsd_386.go b/src/runtime/defs_freebsd_386.go index 767755425c..f822934d58 100644 --- a/src/runtime/defs_freebsd_386.go +++ b/src/runtime/defs_freebsd_386.go @@ -13,10 +13,11 @@ const ( ) const ( - _EINTR = 0x4 - _EFAULT = 0xe - _EAGAIN = 0x23 - _ENOSYS = 0x4e + _EINTR = 0x4 + _EFAULT = 0xe + _EAGAIN = 0x23 + _ENOSYS = 0x4e + _ETIMEDOUT = 0x3c _O_NONBLOCK = 0x4 _O_CLOEXEC = 0x100000 diff --git a/src/runtime/defs_freebsd_amd64.go b/src/runtime/defs_freebsd_amd64.go index 5a833426fd..0b696cf227 100644 --- a/src/runtime/defs_freebsd_amd64.go +++ b/src/runtime/defs_freebsd_amd64.go @@ -13,10 +13,11 @@ const ( ) const ( - _EINTR = 0x4 - _EFAULT = 0xe - _EAGAIN = 0x23 - _ENOSYS = 0x4e + _EINTR = 0x4 + _EFAULT = 0xe + _EAGAIN = 0x23 + _ENOSYS = 0x4e + _ETIMEDOUT = 0x3c _O_NONBLOCK = 0x4 _O_CLOEXEC = 0x100000 diff --git a/src/runtime/defs_freebsd_arm.go b/src/runtime/defs_freebsd_arm.go index b55dfd88cf..b6f3e790cf 100644 --- a/src/runtime/defs_freebsd_arm.go +++ b/src/runtime/defs_freebsd_arm.go @@ -13,10 +13,11 @@ const ( ) const ( - _EINTR = 0x4 - _EFAULT = 0xe - _EAGAIN = 0x23 - _ENOSYS = 0x4e + _EINTR = 0x4 + _EFAULT = 0xe + _EAGAIN = 0x23 + _ENOSYS = 0x4e + _ETIMEDOUT = 0x3c _O_NONBLOCK = 0x4 _O_CLOEXEC = 0x100000 diff --git a/src/runtime/defs_freebsd_arm64.go b/src/runtime/defs_freebsd_arm64.go index 5b9d504ba6..0759a1238f 100644 --- a/src/runtime/defs_freebsd_arm64.go +++ b/src/runtime/defs_freebsd_arm64.go @@ -13,10 +13,11 @@ const ( ) const ( - _EINTR = 0x4 - _EFAULT = 0xe - _EAGAIN = 0x23 - _ENOSYS = 0x4e + _EINTR = 0x4 + _EFAULT = 0xe + _EAGAIN = 0x23 + _ENOSYS = 0x4e + _ETIMEDOUT = 0x3c _O_NONBLOCK = 0x4 _O_CLOEXEC = 0x100000 diff --git a/src/runtime/os_freebsd.go b/src/runtime/os_freebsd.go index 730973a202..1c60ee2a57 100644 --- a/src/runtime/os_freebsd.go +++ b/src/runtime/os_freebsd.go @@ -166,7 +166,7 @@ func futexsleep1(addr *uint32, val uint32, ns int64) { utp = &ut } ret := sys_umtx_op(addr, _UMTX_OP_WAIT_UINT_PRIVATE, val, unsafe.Sizeof(*utp), utp) - if ret >= 0 || ret == -_EINTR { + if ret >= 0 || ret == -_EINTR || ret == -_ETIMEDOUT { return } print("umtx_wait addr=", addr, " val=", val, " ret=", ret, "\n") @@ -208,7 +208,6 @@ func newosproc(mp *m) { var oset sigset sigprocmask(_SIG_SETMASK, &sigset_all, &oset) - // TODO: Check for error. ret := thr_new(¶m, int32(unsafe.Sizeof(param))) sigprocmask(_SIG_SETMASK, &oset, nil) if ret < 0 { diff --git a/src/runtime/sys_freebsd_386.s b/src/runtime/sys_freebsd_386.s index c346e719e1..97e6d9ab36 100644 --- a/src/runtime/sys_freebsd_386.s +++ b/src/runtime/sys_freebsd_386.s @@ -13,12 +13,16 @@ TEXT runtime·sys_umtx_op(SB),NOSPLIT,$-4 MOVL $454, AX INT $0x80 + JAE 2(PC) + NEGL AX MOVL AX, ret+20(FP) RET TEXT runtime·thr_new(SB),NOSPLIT,$-4 MOVL $455, AX INT $0x80 + JAE 2(PC) + NEGL AX MOVL AX, ret+8(FP) RET @@ -120,6 +124,8 @@ TEXT runtime·pipe2(SB),NOSPLIT,$12-16 MOVL flags+0(FP), BX MOVL BX, 8(SP) INT $0x80 + JAE 2(PC) + NEGL AX MOVL AX, errno+12(FP) RET diff --git a/src/runtime/sys_freebsd_amd64.s b/src/runtime/sys_freebsd_amd64.s index 010b2ec4d4..07734b0d7d 100644 --- a/src/runtime/sys_freebsd_amd64.s +++ b/src/runtime/sys_freebsd_amd64.s @@ -18,6 +18,8 @@ TEXT runtime·sys_umtx_op(SB),NOSPLIT,$0 MOVQ ut+24(FP), R8 MOVL $454, AX SYSCALL + JCC 2(PC) + NEGQ AX MOVL AX, ret+32(FP) RET @@ -26,6 +28,8 @@ TEXT runtime·thr_new(SB),NOSPLIT,$0 MOVL size+8(FP), SI MOVL $455, AX SYSCALL + JCC 2(PC) + NEGQ AX MOVL AX, ret+16(FP) RET @@ -118,6 +122,8 @@ TEXT runtime·pipe2(SB),NOSPLIT,$0-20 MOVL flags+0(FP), SI MOVL $542, AX SYSCALL + JCC 2(PC) + NEGQ AX MOVL AX, errno+16(FP) RET diff --git a/src/runtime/sys_freebsd_arm.s b/src/runtime/sys_freebsd_arm.s index 1e12f9cfcb..b12e47c576 100644 --- a/src/runtime/sys_freebsd_arm.s +++ b/src/runtime/sys_freebsd_arm.s @@ -51,6 +51,7 @@ TEXT runtime·sys_umtx_op(SB),NOSPLIT,$0 ADD $20, R13 // arg 5 is passed on stack MOVW $SYS__umtx_op, R7 SWI $0 + RSB.CS $0, R0 SUB $20, R13 // BCS error MOVW R0, ret+20(FP) @@ -61,6 +62,7 @@ TEXT runtime·thr_new(SB),NOSPLIT,$0 MOVW size+4(FP), R1 MOVW $SYS_thr_new, R7 SWI $0 + RSB.CS $0, R0 MOVW R0, ret+8(FP) RET @@ -144,6 +146,7 @@ TEXT runtime·pipe2(SB),NOSPLIT,$0-16 MOVW flags+0(FP), R1 MOVW $SYS_pipe2, R7 SWI $0 + RSB.CS $0, R0 MOVW R0, errno+12(FP) RET diff --git a/src/runtime/sys_freebsd_arm64.s b/src/runtime/sys_freebsd_arm64.s index 8a4f9b7fa1..1aa09e87ca 100644 --- a/src/runtime/sys_freebsd_arm64.s +++ b/src/runtime/sys_freebsd_arm64.s @@ -60,6 +60,9 @@ TEXT runtime·sys_umtx_op(SB),NOSPLIT,$0 MOVD ut+24(FP), R4 MOVD $SYS__umtx_op, R8 SVC + BCC ok + NEG R0, R0 +ok: MOVW R0, ret+32(FP) RET @@ -69,6 +72,9 @@ TEXT runtime·thr_new(SB),NOSPLIT,$0 MOVW size+8(FP), R1 MOVD $SYS_thr_new, R8 SVC + BCC ok + NEG R0, R0 +ok: MOVW R0, ret+16(FP) RET -- cgit v1.3