[go] runtime: add workaround for exynos CPU and enable LSE atomics in Android
(Daniel.L) Byoungchan Lee (Gerrit)
(Daniel.L) Byoungchan Lee has uploaded this change for review.
runtime: add workaround for exynos CPU and enable LSE atomics in Android
Big.LITTLE Heterogeneous architectures, as described by ARM [1],
require that the instruction set architecture of the big and little
cores be compatible. However, the Samsung Exynos 9810 is known to
have different ISAs in its core.
According to #28431, some cores are ARMv8.2 and others are ARMv8.0.
Since LSE is for ARMv8.1 and later, it should be disabled
for this broken CPU. Also, enable LSE atomics on Android.
[1] https://developer.arm.com/documentation/den0024/a/big-LITTLE-Technology
Change-Id: I9b22c00e39cea54f1bd7a4162b533214bbe6818f
---
A src/runtime/sys_android_arm64.s
M src/internal/cpu/cpu_arm64_hwcap.go
M src/internal/cpu/cpu_arm64_android.go
A src/runtime/sys_android_arm64.go
M src/runtime/sys_libc.go
5 files changed, 91 insertions(+), 6 deletions(-)
diff --git a/src/internal/cpu/cpu_arm64_android.go b/src/internal/cpu/cpu_arm64_android.go
index fbdf7ba..dc18099 100644
--- a/src/internal/cpu/cpu_arm64_android.go
+++ b/src/internal/cpu/cpu_arm64_android.go
@@ -6,6 +6,42 @@
package cpu
+import "unsafe"
+
+// define PROP_VALUE_MAX 92 from <sys/system_properties.h>
+const propValueMax = 92
+
func osInit() {
hwcapInit("android")
}
+
+//go:noescape
+func system_property_get(name, value uintptr) int32
+
+func isKnownForBrokenAtomics() bool {
+ var name [8]byte
+ copy(name[:], "ro.arch")
+ nameP := unsafe.Pointer(&name[0])
+
+ var arch [propValueMax]byte
+ archP := unsafe.Pointer(&arch[0])
+
+ if system_property_get(uintptr(nameP), uintptr(archP)) < 1 {
+ return false
+ }
+ // Some cores of Exynos 9810 are ARMv8.2 and others are ARMv8.0,
+ // so disable the lse atomics completely.
+ if arch[0] != 'e' ||
+ arch[1] != 'x' ||
+ arch[2] != 'y' ||
+ arch[3] != 'n' ||
+ arch[4] != 'o' ||
+ arch[5] != 's' ||
+ arch[6] != '9' ||
+ arch[7] != '8' ||
+ arch[8] != '1' ||
+ arch[9] != '0' {
+ return false
+ }
+ return true
+}
diff --git a/src/internal/cpu/cpu_arm64_hwcap.go b/src/internal/cpu/cpu_arm64_hwcap.go
index 0baa39f..ed77d61 100644
--- a/src/internal/cpu/cpu_arm64_hwcap.go
+++ b/src/internal/cpu/cpu_arm64_hwcap.go
@@ -31,11 +31,7 @@
ARM64.HasSHA2 = isSet(HWCap, hwcap_SHA2)
ARM64.HasCRC32 = isSet(HWCap, hwcap_CRC32)
ARM64.HasCPUID = isSet(HWCap, hwcap_CPUID)
-
- // The Samsung S9+ kernel reports support for atomics, but not all cores
- // actually support them, resulting in SIGILL. See issue #28431.
- // TODO(elias.naur): Only disable the optimization on bad chipsets on android.
- ARM64.HasATOMICS = isSet(HWCap, hwcap_ATOMICS) && os != "android"
+ ARM64.HasATOMICS = isSet(HWCap, hwcap_ATOMICS) && !isKnownForBrokenAtomics()
// Check to see if executing on a NeoverseN1 and in order to do that,
// check the AUXV for the CPUID bit. The getMIDR function executes an
diff --git a/src/runtime/sys_android_arm64.go b/src/runtime/sys_android_arm64.go
new file mode 100644
index 0000000..96bd37a
--- /dev/null
+++ b/src/runtime/sys_android_arm64.go
@@ -0,0 +1,22 @@
+// Copyright 2021 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 runtime
+
+import (
+ "internal/abi"
+ "unsafe"
+)
+
+//go:linkname internal_system_property_get internal/cpu.system_property_get
+//go:noescape
+func system_property_get_trampoline(name, value uintptr) int32
+
+//go:cgo_import_dynamic libc_system_property_get __system_property_get "libc.so"
+
+//go:nosplit
+//go:cgo_unsafe_args
+func internal_system_property_get(name, value uintptr) int32 {
+ return libcCall(unsafe.Pointer(abi.FuncPCABI0(system_property_get_trampoline)), unsafe.Pointer(&name))
+}
diff --git a/src/runtime/sys_android_arm64.s b/src/runtime/sys_android_arm64.s
new file mode 100644
index 0000000..f9f48ed
--- /dev/null
+++ b/src/runtime/sys_android_arm64.s
@@ -0,0 +1,11 @@
+// Copyright 2021 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.
+
+#include "textflag.h"
+
+TEXT runtime·system_property_get_trampoline(SB),NOSPLIT,$0
+ MOVD 8(R0), R1 // arg 2 value
+ MOVD 0(R0), R0 // arg 1 name
+ CALL libc_system_property_get(SB)
+ RET
diff --git a/src/runtime/sys_libc.go b/src/runtime/sys_libc.go
index 7012b41..0ee7d5f 100644
--- a/src/runtime/sys_libc.go
+++ b/src/runtime/sys_libc.go
@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
-//go:build darwin || (openbsd && !mips64)
+//go:build darwin || (openbsd && !mips64) || (android && arm64)
package runtime
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
(Daniel.L) Byoungchan Lee (Gerrit)
Attention is currently required from: Martin Möhrmann.
1 comment:
Patchset:
Note that a patch similar to this CL have been landed to the LLVM compiler-rt. https://reviews.llvm.org/D114523
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Patchset:
I would like to get a review for this CL.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Martin Möhrmann.
Daniel.L (Byoungchan) Lee uploaded patch set #4 to this change.
runtime: add workaround for exynos CPU and enable LSE atomics in Android
According to #28431, some cores in the Exynos 9810 CPU are ARMv8.2 and
others are ARMv8.0; only the former support LSE atomics.
However, the kernel in the initial Android 8.0 release of Galaxy S9/S9+
devices incorrectly reported the feature as being supported.
Enable LSE atomics on Android and add a workaround for problematic
devices.
Change-Id: I9b22c00e39cea54f1bd7a4162b533214bbe6818f
---
M src/internal/cpu/cpu_arm64_android.go
M src/internal/cpu/cpu_arm64_hwcap.go
A src/runtime/sys_android_arm64.go
A src/runtime/sys_android_arm64.s
M src/runtime/sys_libc.go
5 files changed, 88 insertions(+), 6 deletions(-)
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Martin Möhrmann, Than McIntosh.
1 comment:
Patchset:
Hi, Than. Please take a look. Thank you.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Martin Möhrmann, Daniel.L (Byoungchan) Lee.
1 comment:
Patchset:
Thanks.
I am probably not the right person to review this patch though-- would be better to get one of the runtime experts to look it over.
You wrote:
>>kernel in the initial Android 8.0 release of Galaxy S9/S9+ devices incorrectly reported the feature as being supported
Is there a bug number for this kernel issue?
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Martin Möhrmann, Than McIntosh.
1 comment:
Patchset:
Can you help me find someone who can review this Cl?
Is there a bug number for this kernel issue?
I don't know the kernel issue number for this, but this has been mentioned in the LKML.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Martin Möhrmann, Than McIntosh, Cherry Mui.
1 comment:
Patchset:
Hi, Cherry. Could you take a look? Thanks.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Cherry Mui, Daniel.L (Byoungchan) Lee, Martin Möhrmann, Than McIntosh.
6 comments:
File src/internal/cpu/cpu_arm64_android.go:
Patch Set #4, Line 19:
func system_property_get(name, value uintptr) int32Why uintptr? It looks like this should be (name, value unsafe.Pointer).
Patch Set #4, Line 23:
copy(name[:], "ro.arch")Just
name := []byte("ro.arch\000")Patch Set #4, Line 34:
if arch[0] != 'e' ||const check = "exynos9810"
return string(arch)[:len(check)] != check
File src/runtime/sys_android_arm64.go:
Patch Set #4, Line 1:
// Copyright 2021 The Go Authors. All rights reserved.s/2021/2022/
File src/runtime/sys_android_arm64.s:
Patch Set #4, Line 1:
// Copyright 2021 The Go Authors. All rights reserved.s/2021/2022/
File src/runtime/sys_libc.go:
Patch Set #4, Line 5:
//go:build darwin || (openbsd && !mips64) || (android && arm64)This is going to force us to do external linking for android/arm64. I think that we currently do force external linking, but will we always?
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Martin Möhrmann, Than McIntosh.
7 comments:
Patchset:
Thanks for the review.
File src/internal/cpu/cpu_arm64_android.go:
Patch Set #4, Line 19:
func system_property_get(name, value uintptr) int32Why uintptr? It looks like this should be (name, value unsafe.Pointer).
Done
Patch Set #4, Line 23:
copy(name[:], "ro.arch")
Just […]
Done
Patch Set #4, Line 34:
if arch[0] != 'e' ||
const check = "exynos9810" […]
Done
File src/runtime/sys_android_arm64.go:
Patch Set #4, Line 1:
// Copyright 2021 The Go Authors. All rights reserved.s/2021/2022/
Done
File src/runtime/sys_android_arm64.s:
Patch Set #4, Line 1:
// Copyright 2021 The Go Authors. All rights reserved.s/2021/2022/
Done
File src/runtime/sys_libc.go:
Patch Set #4, Line 5:
//go:build darwin || (openbsd && !mips64) || (android && arm64)This is going to force us to do external linking for android/arm64. […]
I tested building binaries with `GO_EXTLINK_ENABLED=0` and it works, so internal linking seems to work.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Cherry Mui, Daniel.L (Byoungchan) Lee, Ian Lance Taylor, Martin Möhrmann.
Patch set 5:Code-Review +1
Ian Lance Taylor (Gerrit)
Attention is currently required from: Cherry Mui, Daniel.L (Byoungchan) Lee, Hyang-Ah Hana Kim, Martin Möhrmann.
Patch set 5:Code-Review +1
1 comment:
File src/runtime/sys_libc.go:
Patch Set #4, Line 5:
//go:build darwin || (openbsd && !mips64) || (android && arm64)
I tested building binaries with `GO_EXTLINK_ENABLED=0` and it works, so internal linking seems to wo […]
Ack
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Daniel.L (Byoungchan) Lee, Hyang-Ah Hana Kim, Martin Möhrmann.
2 comments:
Commit Message:
However, the kernel in the initial Android 8.0 release of Galaxy S9/S9+
devices incorrectly reported the feature as being supported.Are the buggy device and/or kernel still popular? Can we just stop supporting it?
File src/runtime/sys_libc.go:
Patch Set #5, Line 5:
(android && arm64)I'm not sure this is safe. For a non-cgo binary only some platforms support libc call directly from Go (which needs some work). I don't think Android/ARM64 is one of them. Most of Android binaries are cgo binaries, but I think there are non-cgo programs.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Cherry Mui, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Commit Message:
However, the kernel in the initial Android 8.0 release of Galaxy S9/S9+
devices incorrectly reported the feature as being supported.Are the buggy device and/or kernel still popular? Can we just stop supporting it?
Problematic devices was sold in units of 10 million units. I'm sure that there's still a non-negligible amount of devices still in use. Relevant chromium bug: https://crbug.com/1272795
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Daniel.L (Byoungchan) Lee, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Commit Message:
However, the kernel in the initial Android 8.0 release of Galaxy S9/S9+
devices incorrectly reported the feature as being supported.
Problematic devices was sold in units of 10 million units. […]
Thanks. How important is it to enable LSE atomics? How much performance benefit does it have?
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Cherry Mui, Hyang-Ah Hana Kim, Martin Möhrmann.
2 comments:
Commit Message:
However, the kernel in the initial Android 8.0 release of Galaxy S9/S9+
devices incorrectly reported the feature as being supported.
Thanks. […]
I don't know about golang-related benchmarks, but according to MySQL[1], applying LSE Atomics gave a performance improvement of 2-4% in some workloads.
I ran the `runtime/internal/atomic` benchmark on the Pixel 3a and got the following results:```
name old time/op new time/op delta
AtomicLoad64-8 1.05ns ± 6% 1.07ns ± 5% ~ (p=0.489 n=10+10)
AtomicStore64-8 1.28ns ± 7% 1.25ns ± 6% ~ (p=0.344 n=9+10)
AtomicLoad-8 1.04ns ± 6% 1.04ns ± 7% ~ (p=0.965 n=10+10)
AtomicStore-8 1.29ns ± 5% 1.25ns ± 7% ~ (p=0.123 n=10+10)
And8-8 14.7ns ± 4% 69.7ns ± 6% +374.41% (p=0.000 n=10+10)
And-8 12.3ns ± 0% 70.8ns ± 2% +475.83% (p=0.000 n=9+9)
And8Parallel-8 55.8ns ± 0% 35.8ns ± 0% -35.89% (p=0.000 n=10+10)
AndParallel-8 47.1ns ± 1% 35.8ns ± 0% -23.94% (p=0.000 n=10+9)
Or8-8 14.8ns ± 3% 70.4ns ± 2% +376.49% (p=0.000 n=10+10)
Or-8 12.5ns ± 4% 69.8ns ± 2% +458.16% (p=0.000 n=10+8)
Or8Parallel-8 55.6ns ± 0% 35.8ns ± 0% -35.68% (p=0.000 n=10+10)
OrParallel-8 46.6ns ± 0% 35.8ns ± 0% -23.20% (p=0.000 n=10+10)
Xadd-8 47.3ns ± 0% 35.8ns ± 0% -24.21% (p=0.000 n=8+10)
Xadd64-8 47.0ns ± 0% 35.8ns ± 0% -23.91% (p=0.000 n=9+10)
Cas-8 75.3ns ± 0% 71.7ns ± 0% -4.87% (p=0.000 n=10+10)
Cas64-8 75.3ns ± 0% 71.6ns ± 0% -4.85% (p=0.000 n=10+8)
Xchg-8 49.3ns ± 1% 35.8ns ± 0% -27.43% (p=0.000 n=10+10)
Xchg64-8 49.5ns ± 0% 36.1ns ± 2% -26.97% (p=0.000 n=9+10)
```And{,8} and Or{,8} had a performance regression, but other tests saw slight performance improvements.
Patchset:
Using `./make.bash` to build golang gives linker errors when external linking is enabled.
`runtime.system_property_get_trampoline: unhandled relocation for libc_system_property_get (type 44 (SDYNIMPORT) rtype 9 (R_CALLARM64))`Also, using an internal link causes another error, which seems to be easy to fix.
`/path/to/src/go/pkg/tool/linux_amd64/link: internal linking requested via GO_EXTLINK_ENABLED but external linking required: android does not support internal cgo`
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Cherry Mui, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Commit Message:
However, the kernel in the initial Android 8.0 release of Galaxy S9/S9+
devices incorrectly reported the feature as being supported.
I don't know about golang-related benchmarks, but according to MySQL[1], applying LSE Atomics gave a […]
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Cherry Mui, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Patchset:
iant@, I saw your proposal in [1] and found out that Android port is not a first-class citizen. Could you please add the maintainer of the Android port as a reviewer to clarify the linking issue on Android? Thank you.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Changkun Ou, Daniel.L (Byoungchan) Lee, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Patchset:
Using `./make.bash` to build golang gives linker errors when external linking is enabled. […]
This failure looks related. It needs to be addressed.
As commented earlier, I don't think making a libc call from a non-cgo binary would work in general. One possibility is perhaps writing the check in C in the runtime/cgo package, and only call it and enable the new atomics if cgo is used, which is the majority on Android.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Changkun Ou (Gerrit)
Attention is currently required from: Daniel.L (Byoungchan) Lee, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Patchset:
After reading this CL, I think it is a well-motivated change but also not strong enough. As Cherry's comments pointed out, a few concerns have not yet been addressed. Furthermore, I would more favor deprecating old and buggy devices/kernels. Hence a relevant change might simply remove the TODO comments and os != "android" check.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Daniel.L (Byoungchan) Lee (Gerrit)
Attention is currently required from: Changkun Ou, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Patchset:
After reading this CL, I think it is a well-motivated change but also not strong enough. […]
Hello, Changkun. Thank you for your reply.
Since I didn't have an exact number on how many devices and kernels had bugs, I was careful not to break any exising applications with this CL .
If you think removing the TODO comment and checking os != "android" check are sufficient, I'll do that.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Changkun Ou (Gerrit)
Attention is currently required from: Daniel.L (Byoungchan) Lee, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Patchset:
Hello, Changkun. Thank you for your reply. […]
I think this is more of a decision that needs to be made by the Go (runtime) team. Perhaps, if we'd like to get this in, the best way is to resolve the comments above (from Cherry) and send an alternative CL (which removes the TODO) as a comparison and support the decision.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Changkun Ou, Daniel.L (Byoungchan) Lee, Hyang-Ah Hana Kim, Martin Möhrmann.
1 comment:
Patchset:
I think this is more of a decision that needs to be made by the Go (runtime) team. […]
Thanks, Changkun. I think the Android port maintainers can decide whether to drop support of old buggy devices/kernels, and (if keeping the support) whether the optimization is worth doing. You're right that more work needs to be done if we want to pursue this CL. Thanks.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.
Changkun Ou (Gerrit)
Attention is currently required from: Cherry Mui, Daniel.L (Byoungchan) Lee.
1 comment:
Patchset:
Thanks, Changkun. […]
Thanks, Cherry, for the clarification. @dani...@hpcnt.com, would you be willing to send two different CLs to support the decision better? With the CLs, we could engage in a discussion among many stakeholders of the Android platform.
To view, visit change 367076. To unsubscribe, or for help writing mail filters, visit settings.