Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Thu, 23 Nov 2023 08:09:44 GMT, Viktor Klang wrote: >>> Wow! This PR is much larger than I expected. >>> >>> Thumbs up! >> >> Thanks for the Review Dan! Yes lots of code deletion engineering in this one >> - and even better I got to delete template code with meta-programming stuff! >> :D > > @dholmes-ora Just passing by -- impressed by the thorough update! Thanks for taking a look @viktorklang-ora ! - PR Comment: https://git.openjdk.org/jdk/pull/16625#issuecomment-1823957279
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Thu, 23 Nov 2023 03:11:15 GMT, David Holmes wrote: >> Wow! This PR is much larger than I expected. >> >> Thumbs up! > >> Wow! This PR is much larger than I expected. >> >> Thumbs up! > > Thanks for the Review Dan! Yes lots of code deletion engineering in this one > - and even better I got to delete template code with meta-programming stuff! > :D @dholmes-ora Just passing by -- impressed by the thorough update! - PR Comment: https://git.openjdk.org/jdk/pull/16625#issuecomment-1823953312
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 18:35:33 GMT, Daniel D. Daugherty
wrote:
>> src/hotspot/share/runtime/vm_version.cpp line 33:
>>
>>> 31: void VM_Version_init() {
>>> 32: VM_Version::initialize();
>>> 33: guarantee(VM_Version::supports_cx8(), "Support for 64-bit atomic
>>> operations in required in this release");
>>
>> Typo: "in required in". Also, no need to mention "this release" at all?
>> Suggestion for message: "JVM requires platform support for 64-bit atomic
>> operations"
>
> Or the simpler change:
> s/in required/is required/
Message tweaked: Support for 64-bit atomic operations is required
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402877940
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 18:35:59 GMT, Daniel D. Daugherty wrote: > Wow! This PR is much larger than I expected. > > Thumbs up! Thanks for the Review Dan! Yes lots of code deletion engineering in this one - and even better I got to delete template code with meta-programming stuff! :D - PR Comment: https://git.openjdk.org/jdk/pull/16625#issuecomment-1823771180
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 15:50:17 GMT, Doug Lea wrote: >> David Holmes has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - Merge with master and update Zero code accordingly >> - Merge branch 'master' into 8318776-supports_cx8 >> - Remove unnecessary includes of vm_version.hpp. >>Fix copyright years. >> - Remove cx8 comment as no longer relevant (the spinlock is used regardless >> of cx8) >> - Remove suports_cx8() checks from gtest >> - Remove test for VMSupportsCX8 >> - 8318776: Require supports_cx8 to always be true > > The deletion of backup code and the check for it in > java.util.concurrent.AtomicLongFieldUpdater are clearly OK. We always thought > the need for it was transient. Thanks for looking at this @DougLea ! - PR Comment: https://git.openjdk.org/jdk/pull/16625#issuecomment-1823770525
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 21:41:50 GMT, Aleksey Shipilev wrote: >> @shipilev - Do you have a particular legacy x86 in mind? > > My point is that it is such an easy thing to do: leave the "cx8" flag sensing > code in, and keep setting up `_supports_cx8` based on it for `!_LP64` paths. > This both provides more safety by failing cleanly on non-CX8 platform, and > gives other platforms some guidance: if you can check something is supported, > check it. I think we are generally trying to fail cleanly on unsupported > configs, if that is easy to achieve. > > But now that you nerd-sniped me into this... I think non-CX8 platforms would > probably predate Pentium. The oldest real machine my lab has is Z530, which > already has CX8. But it was easy to also go to my QEMU-driven build-test > server, ask for `i486` as platform there, and et voila, no `cx8` in CPU flags: > > > buildworker-debian12-32:~$ lscpu > Architecture:i486 > CPU op-mode(s):32-bit > Address sizes: 36 bits physical, 32 bits virtual > Byte Order:Little Endian > CPU(s): 4 > On-line CPU(s) list: 0-3 > Vendor ID: GenuineIntel > Model name:486 DX/4 > CPU family: 4 > Model: 8 > Thread(s) per core: 4 > Core(s) per socket: 1 > Socket(s): 1 > Stepping:0 > BogoMIPS:5699.99 > Flags: fpu vme pse apic ht cpuid tsc_known_freq x2apic > hypervisor cpuid_fault > > > And mainline JDK even starts there! (with interpreter, there are some asserts > firing in compiler code, having to do with odd instruction selection on some > paths): > > > $ jdk/bin/java -Xint -version > openjdk version "22-testing" 2024-03-19 > OpenJDK Runtime Environment (fastdebug build > 22-testing-builds.shipilev.net-openjdk-jdk-b627-20231121) > OpenJDK Server VM (fastdebug build > 22-testing-builds.shipilev.net-openjdk-jdk-b627-20231121, interpreted mode, > sharing) Nice spelunking... I was wondering if it was something that old. I wasn't trying to nerd-snipe... I was in the dev lab at Intel when Xenix on the i386 first came up and sent its "Hello World!" email... I left Intel for Sun in 1987 while i486 was still in development, but I still had periodic lunches with folks that worked on those teams. Life was simpler back then... - PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402748121
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 18:26:12 GMT, Daniel D. Daugherty wrote: >> src/hotspot/cpu/x86/vm_version_x86.cpp line 819: >> >>> 817: } >>> 818: >>> 819: _supports_cx8 = supports_cmpxchg8(); >> >> I think we should leave the runtime check here (under `ifndef`, like in >> ARM?). This covers the remaining case of running on legacy x86 without CX8 >> implemented: the init guarantee would then fire and prevent any other >> surprises at runtime. Sure, it would be hard to come up with such a platform >> today, but it would be safer to refuse to run there right away on the >> off-chance someone actually has it :) > > @shipilev - Do you have a particular legacy x86 in mind? My point is that it is such an easy thing to do: leave the "cx8" flag sensing code in, and keep setting up `_supports_cx8` based on it. This both provides more safety by failing cleanly on non-CX8 platform, and gives other platforms some guidance: if you can check something is supported, check it. But now that you nerd-sniped me into this... I think non-CX8 platforms would probably predate Pentium. The oldest real machine my lab has is Z530, which already has CX8. But it was easy to also go to my QEMU-driven build-test server, ask for `i486` as platform there, and et voila, no `cx8` in CPU flags: buildworker-debian12-32:~$ lscpu Architecture:i486 CPU op-mode(s):32-bit Address sizes: 36 bits physical, 32 bits virtual Byte Order:Little Endian CPU(s): 4 On-line CPU(s) list: 0-3 Vendor ID: GenuineIntel Model name:486 DX/4 CPU family: 4 Model: 8 Thread(s) per core: 4 Core(s) per socket: 1 Socket(s): 1 Stepping:0 BogoMIPS:5699.99 Flags: fpu vme pse apic ht cpuid tsc_known_freq x2apic hypervisor cpuid_fault And mainline JDK even starts there! (with interpreter, there are some asserts firing in compiler code, having to do with odd instruction selection on some paths): $ jdk/bin/java -Xint -version openjdk version "22-testing" 2024-03-19 OpenJDK Runtime Environment (fastdebug build 22-testing-builds.shipilev.net-openjdk-jdk-b627-20231121) OpenJDK Server VM (fastdebug build 22-testing-builds.shipilev.net-openjdk-jdk-b627-20231121, interpreted mode, sharing) - PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402738580
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 08:48:09 GMT, Aleksey Shipilev wrote:
>> David Holmes has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge with master and update Zero code accordingly
>> - Merge branch 'master' into 8318776-supports_cx8
>> - Remove unnecessary includes of vm_version.hpp.
>>Fix copyright years.
>> - Remove cx8 comment as no longer relevant (the spinlock is used regardless
>> of cx8)
>> - Remove suports_cx8() checks from gtest
>> - Remove test for VMSupportsCX8
>> - 8318776: Require supports_cx8 to always be true
>
> src/hotspot/cpu/x86/vm_version_x86.cpp line 819:
>
>> 817: }
>> 818:
>> 819: _supports_cx8 = supports_cmpxchg8();
>
> I think we should leave the runtime check here (under `ifndef`, like in
> ARM?). This covers the remaining case of running on legacy x86 without CX8
> implemented: the init guarantee would then fire and prevent any other
> surprises at runtime. Sure, it would be hard to come up with such a platform
> today, but it would be safer to refuse to run there right away on the
> off-chance someone actually has it :)
@shipilev - Do you have a particular legacy x86 in mind?
> src/hotspot/share/runtime/vm_version.cpp line 33:
>
>> 31: void VM_Version_init() {
>> 32: VM_Version::initialize();
>> 33: guarantee(VM_Version::supports_cx8(), "Support for 64-bit atomic
>> operations in required in this release");
>
> Typo: "in required in". Also, no need to mention "this release" at all?
> Suggestion for message: "JVM requires platform support for 64-bit atomic
> operations"
Or the simpler change:
s/in required/is required/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402515036
PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402528045
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 02:09:38 GMT, David Holmes wrote: >> As discussed in JBS all platforms (some tweaks to Zero are in progress) >> actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip >> out the locked-based alternatives to using it and just add a guarantee that >> it is true at runtime. And all platforms except some ARM variants set >> `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes: >> - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not >> defined >> - Assertions for `supports_cx8()` are removed >> - Compiler predicates requiring `supports_cx8()` are removed >> - Access backend is greatly simplified without the need for lock-based >> alternative >> - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the >> need for a lock-based alternative >> >> I did consider moving all the ARM `kuser_helper` related code to be only >> defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a >> theoretical risk this could change the behaviour if ARMv7 binaries were run >> on other ARM CPU's. I added a note to that effect in >> vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up >> further if desired. >> >> Testing: >> - All Oracle tiers 1-5 builds (which includes an ARMv7 build) >> - GHA builds/tests >> - Oracle tiers 1-3 sanity testing >> >> Zero changes coming in via >> [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged >> when they arrive. >> >> Thanks. > > David Holmes has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge with master and update Zero code accordingly > - Merge branch 'master' into 8318776-supports_cx8 > - Remove unnecessary includes of vm_version.hpp. >Fix copyright years. > - Remove cx8 comment as no longer relevant (the spinlock is used regardless > of cx8) > - Remove suports_cx8() checks from gtest > - Remove test for VMSupportsCX8 > - 8318776: Require supports_cx8 to always be true Wow! This PR is much larger than I expected. Thumbs up! - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16625#pullrequestreview-1745089631
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 02:09:38 GMT, David Holmes wrote: >> As discussed in JBS all platforms (some tweaks to Zero are in progress) >> actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip >> out the locked-based alternatives to using it and just add a guarantee that >> it is true at runtime. And all platforms except some ARM variants set >> `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes: >> - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not >> defined >> - Assertions for `supports_cx8()` are removed >> - Compiler predicates requiring `supports_cx8()` are removed >> - Access backend is greatly simplified without the need for lock-based >> alternative >> - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the >> need for a lock-based alternative >> >> I did consider moving all the ARM `kuser_helper` related code to be only >> defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a >> theoretical risk this could change the behaviour if ARMv7 binaries were run >> on other ARM CPU's. I added a note to that effect in >> vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up >> further if desired. >> >> Testing: >> - All Oracle tiers 1-5 builds (which includes an ARMv7 build) >> - GHA builds/tests >> - Oracle tiers 1-3 sanity testing >> >> Zero changes coming in via >> [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged >> when they arrive. >> >> Thanks. > > David Holmes has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge with master and update Zero code accordingly > - Merge branch 'master' into 8318776-supports_cx8 > - Remove unnecessary includes of vm_version.hpp. >Fix copyright years. > - Remove cx8 comment as no longer relevant (the spinlock is used regardless > of cx8) > - Remove suports_cx8() checks from gtest > - Remove test for VMSupportsCX8 > - 8318776: Require supports_cx8 to always be true The deletion of backup code and the check for it in java.util.concurrent.AtomicLongFieldUpdater are clearly OK. We always thought the need for it was transient. - PR Comment: https://git.openjdk.org/jdk/pull/16625#issuecomment-1823026095
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 08:57:11 GMT, Aleksey Shipilev wrote: > Zero tests are running. Caught the `guarantee` on linux-arm-zero-fastdebug! But that is actually the fault in my previous patch: #16779. - PR Comment: https://git.openjdk.org/jdk/pull/16625#issuecomment-1822510325
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 02:09:38 GMT, David Holmes wrote:
>> As discussed in JBS all platforms (some tweaks to Zero are in progress)
>> actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip
>> out the locked-based alternatives to using it and just add a guarantee that
>> it is true at runtime. And all platforms except some ARM variants set
>> `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes:
>> - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not
>> defined
>> - Assertions for `supports_cx8()` are removed
>> - Compiler predicates requiring `supports_cx8()` are removed
>> - Access backend is greatly simplified without the need for lock-based
>> alternative
>> - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the
>> need for a lock-based alternative
>>
>> I did consider moving all the ARM `kuser_helper` related code to be only
>> defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a
>> theoretical risk this could change the behaviour if ARMv7 binaries were run
>> on other ARM CPU's. I added a note to that effect in
>> vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up
>> further if desired.
>>
>> Testing:
>> - All Oracle tiers 1-5 builds (which includes an ARMv7 build)
>> - GHA builds/tests
>> - Oracle tiers 1-3 sanity testing
>>
>> Zero changes coming in via
>> [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged
>> when they arrive.
>>
>> Thanks.
>
> David Holmes has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - Merge with master and update Zero code accordingly
> - Merge branch 'master' into 8318776-supports_cx8
> - Remove unnecessary includes of vm_version.hpp.
>Fix copyright years.
> - Remove cx8 comment as no longer relevant (the spinlock is used regardless
> of cx8)
> - Remove suports_cx8() checks from gtest
> - Remove test for VMSupportsCX8
> - 8318776: Require supports_cx8 to always be true
src/hotspot/share/runtime/vm_version.cpp line 33:
> 31: void VM_Version_init() {
> 32: VM_Version::initialize();
> 33: guarantee(VM_Version::supports_cx8(), "Support for 64-bit atomic
> operations in required in this release");
Typo: "in required in". Also, no need to mention "this release" at all?
Suggestion for message: "JVM requires platform support for 64-bit atomic
operations"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1401743607
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 02:09:38 GMT, David Holmes wrote: >> As discussed in JBS all platforms (some tweaks to Zero are in progress) >> actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip >> out the locked-based alternatives to using it and just add a guarantee that >> it is true at runtime. And all platforms except some ARM variants set >> `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes: >> - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not >> defined >> - Assertions for `supports_cx8()` are removed >> - Compiler predicates requiring `supports_cx8()` are removed >> - Access backend is greatly simplified without the need for lock-based >> alternative >> - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the >> need for a lock-based alternative >> >> I did consider moving all the ARM `kuser_helper` related code to be only >> defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a >> theoretical risk this could change the behaviour if ARMv7 binaries were run >> on other ARM CPU's. I added a note to that effect in >> vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up >> further if desired. >> >> Testing: >> - All Oracle tiers 1-5 builds (which includes an ARMv7 build) >> - GHA builds/tests >> - Oracle tiers 1-3 sanity testing >> >> Zero changes coming in via >> [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged >> when they arrive. >> >> Thanks. > > David Holmes has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge with master and update Zero code accordingly > - Merge branch 'master' into 8318776-supports_cx8 > - Remove unnecessary includes of vm_version.hpp. >Fix copyright years. > - Remove cx8 comment as no longer relevant (the spinlock is used regardless > of cx8) > - Remove suports_cx8() checks from gtest > - Remove test for VMSupportsCX8 > - 8318776: Require supports_cx8 to always be true Thanks! Zero tests are running. The PR looks great, except extra safety suggestion in x86 part: src/hotspot/cpu/x86/vm_version_x86.cpp line 819: > 817: } > 818: > 819: _supports_cx8 = supports_cmpxchg8(); I think we should leave the runtime check here (under `ifndef`, like in ARM?). This covers the remaining case of running on legacy x86 without CX8 implemented: the init guarantee would then fire and prevent any other surprises at runtime. Sure, it would be hard to come up with such a platform today, but it would be safer to refuse to run there right away on the off-chance someone actually has it :) - PR Review: https://git.openjdk.org/jdk/pull/16625#pullrequestreview-1743847107 PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1401696816
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
> As discussed in JBS all platforms (some tweaks to Zero are in progress) > actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip > out the locked-based alternatives to using it and just add a guarantee that > it is true at runtime. And all platforms except some ARM variants set > `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes: > - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not > defined > - Assertions for `supports_cx8()` are removed > - Compiler predicates requiring `supports_cx8()` are removed > - Access backend is greatly simplified without the need for lock-based > alternative > - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the > need for a lock-based alternative > > I did consider moving all the ARM `kuser_helper` related code to be only > defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a > theoretical risk this could change the behaviour if ARMv7 binaries were run > on other ARM CPU's. I added a note to that effect in > vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up > further if desired. > > Testing: > - All Oracle tiers 1-5 builds (which includes an ARMv7 build) > - GHA builds/tests > - Oracle tiers 1-3 sanity testing > > Zero changes coming in via > [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged > when they arrive. > > Thanks. David Holmes has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge with master and update Zero code accordingly - Merge branch 'master' into 8318776-supports_cx8 - Remove unnecessary includes of vm_version.hpp. Fix copyright years. - Remove cx8 comment as no longer relevant (the spinlock is used regardless of cx8) - Remove suports_cx8() checks from gtest - Remove test for VMSupportsCX8 - 8318776: Require supports_cx8 to always be true - Changes: - all: https://git.openjdk.org/jdk/pull/16625/files - new: https://git.openjdk.org/jdk/pull/16625/files/597cef53..aad0a4c4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16625&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16625&range=03-04 Stats: 621905 lines in 1279 files changed: 89413 ins; 471113 del; 61379 mod Patch: https://git.openjdk.org/jdk/pull/16625.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16625/head:pull/16625 PR: https://git.openjdk.org/jdk/pull/16625
