From 4f3f5d84125db0c511b2e1e44b0fe3c6e7ca6f45 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 16 Oct 2023 10:47:39 -0700 Subject: [PATCH] Fix sync.SeqCount.ReadOk() on non-x86 architectures. sync.SeqCount relies on the following memory orderings: - All stores following BeginWrite() in program order happen after the atomic read-modify-write (RMW) of SeqCount.epoch. In the Go 1.19 memory model, this is implied by atomic loads being acquire-seqcst. - All stores preceding EndWrite() in program order happen before the RMW of SeqCount.epoch. In the Go 1.19 memory model, this is implied by atomic stores being release-seqcst. - All loads following BeginRead() in program order happen after the load of SeqCount.epoch. In the Go 1.19 memory model, this is implied by atomic loads being acquire-seqcst. - All loads preceding ReadOk() in program order happen before the load of SeqCount.epoch. The Go 1.19 memory model does not imply this property. The x86 memory model *does* imply this final property, and in practice the current Go compiler does not reorder memory accesses around the load of SeqCount.epoch, so sync.SeqCount behaves correctly on x86. However, on ARM64, the instruction that is actually emitted for the atomic load from SeqCount.epoch is LDAR: ``` gvisor/pkg/sentry/kernel/kernel.SeqAtomicTryLoadTaskGoroutineSchedInfo(): gvisor/pkg/sentry/kernel/seqatomic_taskgoroutineschedinfo_unsafe.go:34 56371c: f9400025 ldr x5, [x1] 563720: f9400426 ldr x6, [x1, #8] 563724: f9400822 ldr x2, [x1, #16] 563728: f9400c23 ldr x3, [x1, #24] gvisor/pkg/sentry/kernel/seqatomic_taskgoroutineschedinfo_unsafe.go:36 56372c: d503201f nop gvisor/pkg/sync/sync.(*SeqCount).ReadOk(): gvisor/pkg/sync/seqcount.go:107 563730: 88dffc07 ldar w7, [x0] 563734: 6b0400ff cmp w7, w4 ``` LDAR is explicitly documented as not implying the required memory ordering: https://developer.arm.com/documentation/den0024/latest/Memory-Ordering/Barriers/One-way-barriers Consequently, SeqCount.ReadOk() is incorrectly memory-ordered on weakly-ordered architectures. To fix this, we need to introduce an explicit memory fence. On ARM64, there is no way to implement the memory fence in question without resorting to assembly, so the implementation is straightforward. On x86, we introduce a compiler fence, since future compilers might otherwise reorder memory accesses to after atomic loads; the only apparent way to do so is also by using assembly, which unfortunately introduces overhead: - After the call to sync.MemoryFenceReads(), callers zero XMM15 and reload the runtime.g pointer from %fs:-8, reflecting the switch from ABI0 to ABIInternal. This is a relatively small cost. - Before the call to sync.MemoryFenceReads(), callers spill all registers to the stack, since ABI0 function calls clobber all registers. The cost of this depends on the state of the caller before the call, and is not reflected in BenchmarkSeqCountReadUncontended (which does not read any protected state between the calls to BeginRead() and ReadOk()). Both of these problems are caused by Go assembly functions being restricted to ABI0. Go provides a way to mark assembly functions as using ABIInternal instead, but restricts its use to functions in package runtime (https://github.com/golang/go/issues/44065). runtime.publicationBarrier(), which is semantically "sync.MemoryFenceWrites()", is implemented as a compiler fence on x86; defining sync.MemoryFenceReads() as an alias for that function (using go:linkname) would mitigate the former problem, but not the latter. Thus, for simplicity, we define sync.MemoryFenceReads() in (ABI0) assembly, and have no choice but to eat the overhead. ("Fence" and "barrier" are often used interchangeably in this context; Linux uses "barrier" (e.g. `smp_rmb()`), while C++ uses "fence" (e.g. `std::atomic_memory_fence()`). We choose "fence" to reduce ambiguity with "write barriers", since Go is a GC'd language.) PiperOrigin-RevId: 573861378 --- pkg/sync/BUILD | 3 +++ pkg/sync/fence.go | 19 +++++++++++++++++++ pkg/sync/fence_amd64.s | 26 ++++++++++++++++++++++++++ pkg/sync/fence_arm64.s | 23 +++++++++++++++++++++++ pkg/sync/seqcount.go | 18 +----------------- 5 files changed, 72 insertions(+), 17 deletions(-) create mode 100644 pkg/sync/fence.go create mode 100644 pkg/sync/fence_amd64.s create mode 100644 pkg/sync/fence_arm64.s diff --git a/pkg/sync/BUILD b/pkg/sync/BUILD index ec6e4c56b..e38df4cd1 100644 --- a/pkg/sync/BUILD +++ b/pkg/sync/BUILD @@ -12,6 +12,9 @@ go_library( "aliases.go", "checklocks_off_unsafe.go", "checklocks_on_unsafe.go", + "fence.go", + "fence_amd64.s", + "fence_arm64.s", "gate_unsafe.go", "goyield_go113_unsafe.go", "goyield_unsafe.go", diff --git a/pkg/sync/fence.go b/pkg/sync/fence.go new file mode 100644 index 000000000..6706676a5 --- /dev/null +++ b/pkg/sync/fence.go @@ -0,0 +1,19 @@ +// Copyright 2023 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sync + +// MemoryFenceReads ensures that all preceding memory loads happen before +// following memory loads. +func MemoryFenceReads() diff --git a/pkg/sync/fence_amd64.s b/pkg/sync/fence_amd64.s new file mode 100644 index 000000000..87766f1d3 --- /dev/null +++ b/pkg/sync/fence_amd64.s @@ -0,0 +1,26 @@ +// Copyright 2023 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build amd64 +// +build amd64 + +#include "textflag.h" + +// func MemoryFenceReads() +TEXT ·MemoryFenceReads(SB),NOSPLIT|NOFRAME,$0-0 + // No memory fence is required on x86. However, a compiler fence is + // required to prevent the compiler from reordering memory accesses. The Go + // compiler will not reorder memory accesses around a call to an assembly + // function; compare runtime.publicationBarrier. + RET diff --git a/pkg/sync/fence_arm64.s b/pkg/sync/fence_arm64.s new file mode 100644 index 000000000..f4f9ce9de --- /dev/null +++ b/pkg/sync/fence_arm64.s @@ -0,0 +1,23 @@ +// Copyright 2023 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build arm64 +// +build arm64 + +#include "textflag.h" + +// func MemoryFenceReads() +TEXT ·MemoryFenceReads(SB),NOSPLIT|NOFRAME,$0-0 + DMB $0x9 // ISHLD + RET diff --git a/pkg/sync/seqcount.go b/pkg/sync/seqcount.go index 9adc95322..c90d2d9fa 100644 --- a/pkg/sync/seqcount.go +++ b/pkg/sync/seqcount.go @@ -39,23 +39,6 @@ type SeqCount struct { // SeqCountEpoch tracks writer critical sections in a SeqCount. type SeqCountEpoch uint32 -// We assume that: -// -// - All functions in sync/atomic that perform a memory read are at least a -// read fence: memory reads before calls to such functions cannot be reordered -// after the call, and memory reads after calls to such functions cannot be -// reordered before the call, even if those reads do not use sync/atomic. -// -// - All functions in sync/atomic that perform a memory write are at least a -// write fence: memory writes before calls to such functions cannot be -// reordered after the call, and memory writes after calls to such functions -// cannot be reordered before the call, even if those writes do not use -// sync/atomic. -// -// As of this writing, the Go memory model completely fails to describe -// sync/atomic, but these properties are implied by -// https://groups.google.com/forum/#!topic/golang-nuts/7EnEhM3U7B8. - // BeginRead indicates the beginning of a reader critical section. Reader // critical sections DO NOT BLOCK writer critical sections, so operations in a // reader critical section MAY RACE with writer critical sections. Races are @@ -104,6 +87,7 @@ func (s *SeqCount) beginReadSlow() SeqCountEpoch { // Reader critical sections do not need to be explicitly terminated; the last // call to ReadOk is implicitly the end of the reader critical section. func (s *SeqCount) ReadOk(epoch SeqCountEpoch) bool { + MemoryFenceReads() return atomic.LoadUint32(&s.epoch) == uint32(epoch) }