RFR: 8255799: AArch64: CPU_A53MAC feature may be set incorrectly

[Anton Kozlov]

On Thu, 5 Nov 2020 11:29:16 GMT, Nick Gasson <ngasson at openjdk.org> wrote:

>>> > It's not a feature of single-core CPUs: AFAIK it's a work around for very old arm64 kernels that only reported a single CPU in `/proc/cpuinfo` on multi-core systems where you may have a mix of different CPU types (i.e. mixed A53/A57 where the A57 is reported in cpuinfo).
>>> > I wonder if we should just remove this workaround altogether? The patch to list all CPUs in `/proc/cpuinfo` was backported to at least the 3.10 series. I really doubt there's anyone running latest OpenJDK on a A53 with such an old kernel.
>>> 
>>> Yes, please.
>> 
>>> > @theRealAph BTW, I think Nick is right that this patch is not needed. Are you ok to reject it?
>>> 
>>> We should delete lines 184-187 in the current file: this isn't working as intended since the switch to `os::processor_count()` and as discussed the ancient kernel versions where this was necessary should no longer be in use.
>> 
>> In general, I support to abandon this and remove the CPU_A53MAC instead. But the use of the flag is not clear for me https://github.com/openjdk/jdk/blob/f279ddfa06392f8ea14224e478a00bad33b84e7a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp#L447 This code does not check if there is a mix of CPU types, but does for exact type/number-of-cores combination that stands behind the flag. I don't feel comfortable to remove this `nop` now. It will be great if If someone can clarify this. Meanwhile, I'll check for `madd`/`msub`/... and will followup. 
>> 
>>> > At present reading /proc/cpuinfo that is the only reliable way I know of to identify whether dcpop is a supported feature (used to force persistence of data to memory). That is needed to support use of NVRAM-backed MappedByteBuffers.
>>> 
>>> I think that comes from `getauxval() & HWCAP_DCPOP`?
>> 
>> Yes, since JDK-8253015 dcpop feature is deteced by hwcap with guarantee about /proc/cpuinfo https://github.com/openjdk/jdk/commit/ec9bee68#diff-7e6fa90a7bcdbe41687eb8d39c6c6232e6518b019937a87aab75284166ef67bdR157-R159. It's only `hwcap => cpuinfo` implication checked, catching silent suboptimal performance. Failing `<=` should be evident from crashing on using dcpop.
>> 
>>> And while we're talking about Linux, can we really not get the info we need without parsing /proc/cpuinfo? And do we need to parse the entire file?
>> 
>> Another reason to read /proc/cpuinfo is to get vendor, model, etc of the CPUs. I would like to avoid addressing this here.
>> 
>>> Tested slowdebug build on Windows+Arm64 with this patch, and smoked tested it with `jtreg:tier1_compiler_1` successfully.
>>> 
>>> Change itself looks go to me too (but I'm not a reviewer).
>> 
>> Thanks!
>
>> > 
>> > We should delete lines 184-187 in the current file: this isn't working as intended since the switch to `os::processor_count()` and as discussed the ancient kernel versions where this was necessary should no longer be in use.
>> 
>> In general, I support to abandon this and remove the CPU_A53MAC instead. But the use of the flag is not clear for me
>> 
>> https://github.com/openjdk/jdk/blob/f279ddfa06392f8ea14224e478a00bad33b84e7a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp#L447
>> This code does not check if there is a mix of CPU types, but does for exact type/number-of-cores combination that stands behind the flag. I don't feel comfortable to remove this `nop` now. It will be great if If someone can clarify this. Meanwhile, I'll check for `madd`/`msub`/... and will followup.
>> 
> 
> I wasn't suggesting removing CPU_A53MAC entirely - that will always be required to work around an A53 hardware errata and we should set it whenever we detect an A53 in cpuinfo. What I meant was remove the logic on lines 184-187 that sets this flag if there is only one CPU listed in /proc/cpuinfo and that CPU is an A57. This exists to handle old Linux kernels that only reported CPU features for a single core in cpuinfo: it's possible on a mixed A53/A57 system that only the A57 features are reported but there's also an A53 lurking, in which case we still need to apply the MAC workaround in the JIT. The kernel was patched long ago to print the features of every CPU in /proc/cpuinfo so this check is no longer required.

> I wasn't suggesting removing CPU_A53MAC entirely [...] What I meant was remove the logic on lines 184-187

Oh, my bad, I spend too much time thinking about reported single-core case, that the real flag meaning slipped from me. Thanks for the hint. I agree with suggestion.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1039