8253284: Zero OrderAccess barrier mappings are incorrect #224

shipilev · 2020-09-17T11:35:50Z

There are some jcstress failures with AArch64 Zero. It seems because to happen because orderAccess_linux_zero.hpp defaults to compiler-only barriers for most OrderAccess::* calls. We need to defer to the strongest barriers by default.

The code also needs some rearrangement to make the mappings clear.

jcstress seems to capture the bug, and seems to pass when bug is fixed. Since this is zero, we want only the interpreter (compiler configs would be excessive). Release builds capture more samples.

$ wget https://builds.shipilev.net/jcstress/jcstress-tests-all-20200917.jar
$ build/linux-x86_64-server-release/images/jdk/bin/java -jar jcstress-tests-all-20200917.jar --jvmArgs "-Xint"

Testing:

x86_64 Linux zero release jcstress run
x86_64 MacOS zero release jcstress run
AArch64 Linux zero release jcstress run
PPC Linux zero release jcstress run
SKIPPED: ARM32 Linux zero release jcstress run (it would seem ARM32 is broken already: JDK-8253464)

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8253284: Zero OrderAccess barrier mappings are incorrect

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Andrew Haley (@theRealAph - Reviewer)
Andrew John Hughes (@gnu-andrew - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/224/head:pull/224
$ git checkout pull/224

bridgekeeper · 2020-09-17T11:37:39Z

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-09-17T11:39:02Z

@shipilev The following label will be automatically applied to this pull request: hotspot-runtime.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

mlbridge · 2020-09-17T11:42:48Z

Webrevs

01: Full - Incremental (add14aa)
00: Full (012a59a)

dholmes-ora

Looks reasonable other than the ifdef ARM issue.

src/hotspot/os_cpu/bsd_zero/orderAccess_bsd_zero.hpp

gnu-andrew · 2020-09-18T01:51:19Z

Is the #if defined(ARM) referring to ARM in general here, or just 32-bit ARM? Because there appears to be no change to its definition, just rearrangement.
The patch is quite hard to follow, and made even more difficult by multiple versions being attached to this PR.

shipilev · 2020-09-18T05:00:52Z

Is the #if defined(ARM) referring to ARM in general here, or just 32-bit ARM? Because there appears to be no change to its definition, just rearrangement.

No, there is ARM (32) and then there is AARCH64 (64). This is why AArch64 fails: it effectively uses compiler barriers only through the confusing application of #else branches.

The patch is quite hard to follow, and made even more difficult by multiple versions being attached to this PR.

These are not multiple versions, those commits would be squashed into one on push -- that is how Skara workflow works. Look at the final version, which basically enables default (implicitly used by ALPHA and AARCH64) strong barriers -- that is the bug fix. It also keeps the light-weight barrier light for X86 by calling it out explicitly.

mlbridge · 2020-09-18T08:28:55Z

Mailing list message from Andrew Haley on hotspot-runtime-dev:

On 18/09/2020 06:03, Aleksey Shipilev wrote:

On Fri, 18 Sep 2020 01:51:19 GMT, Andrew John Hughes <andrew at openjdk.org> wrote:

Is the #if defined(ARM) referring to ARM in general here, or just
32-bit ARM? Because there appears to be no change to its
definition, just rearrangement.

No, there is `ARM` (32) and then there is `AARCH64` (64). This is
why AArch64 fails: it effectively uses compiler barriers only
through the confusing application of #else branches.

The patch is quite hard to follow, and made even more difficult by
multiple versions being attached to this PR.

These are not multiple versions, those commits would be squashed
into one on push -- that is how Skara workflow works. Look at the
final version, which basically enables default (implicitly used by
`ALPHA` and `AARCH64`) strong barriers -- that is the bug fix. It
also keeps the light-weight barrier light for `X86` by calling it
out explicitly.

It looks good enough to me.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

openjdk · 2020-09-18T12:53:37Z

@shipilev This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253284: Zero OrderAccess barrier mappings are incorrect

Reviewed-by: dholmes, aph, andrew

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 56 commits pushed to the master branch:

284bbf0: 8253079: DeterministicDump.java fails due to garbage in structure padding
a4affd5: 8253412: Unsupported GC options passed in JAVA_TOOL_OPTIONS are silently ignored
96f722c: 8252114: Windows-AArch64: Enable and test ZGC and ShenandoahGC
f7b1ce4: 8253447: Remove buggy code introduced by 8249451
282b9dc: 8081833: Clean up JVMFlag getter/setter code
0e98fc1: 8253237: [REDO] Improve large object handling during evacuation
d1f9b8a: 8234808: jdb quoted option parsing broken
d8921ed: 8247281: migrate ObjectMonitor::_object to OopStorage
f800af9: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
2e30ff6: 8253311: Cleanup relocInfo constructors
... and 46 more: https://git.openjdk.java.net/jdk/compare/a9993f9464a724637b3d38bd2f6caedf884d3e0b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 284bbf02ddba7c13e0f93ddb3967615ebaeeb3dc.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

shipilev · 2020-09-18T12:55:07Z

/reviewers 2

openjdk · 2020-09-18T12:56:20Z

@shipilev
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

gnu-andrew · 2020-09-18T15:09:19Z

Is the #if defined(ARM) referring to ARM in general here, or just 32-bit ARM? Because there appears to be no change to its definition, just rearrangement.

No, there is ARM (32) and then there is AARCH64 (64). This is why AArch64 fails: it effectively uses compiler barriers only through the confusing application of #else branches.

Ok, patch looks ok then. That seems to differ from the other architectures, where 'PPC' and 'X86' are referring to both 32- and 64-bit variants.

The patch is quite hard to follow, and made even more difficult by multiple versions being attached to this PR.

These are not multiple versions, those commits would be squashed into one on push -- that is how Skara workflow works. Look at the final version, which basically enables default (implicitly used by ALPHA and AARCH64) strong barriers -- that is the bug fix. It also keeps the light-weight barrier light for X86 by calling it out explicitly.

If there are not multiple versions, how is there a 'final version'? :-)

I eventually found the webrev with the full and final patch. One would normally rebase the patch with amendments, not pile commit on commit and then leave it to a bot to rebase. I don't like the idea that the final commit will look different to what is being reviewed.

/reviewer credit andrew

openjdk · 2020-09-18T15:10:04Z

@gnu-andrew Only the author (@shipilev) is allowed to issue the reviewer command.

gnu-andrew · 2020-09-18T18:34:10Z

Why is there this /reviewer command that doesn't work?

shipilev · 2020-09-19T14:06:52Z

Why is there this /reviewer command that doesn't work?

Reviewer command is to tell the bot who are the off-GH reviewers are. In retrospect, I could have just mentioned you myself here...

dholmes-ora · 2020-09-20T13:14:19Z

The final version of this is the "sum" of the applied commits. When viewing the "changed files" you can select which commit(s) to look at. There is no need to rebase (nor is it even desirable) as you will lose the context of review comments.

Final update looks fine to me.

shipilev · 2020-09-22T08:32:05Z

/integrate

openjdk · 2020-09-22T08:33:52Z

@shipilev Since your change was applied there have been 56 commits pushed to the master branch:

284bbf0: 8253079: DeterministicDump.java fails due to garbage in structure padding
a4affd5: 8253412: Unsupported GC options passed in JAVA_TOOL_OPTIONS are silently ignored
96f722c: 8252114: Windows-AArch64: Enable and test ZGC and ShenandoahGC
f7b1ce4: 8253447: Remove buggy code introduced by 8249451
282b9dc: 8081833: Clean up JVMFlag getter/setter code
0e98fc1: 8253237: [REDO] Improve large object handling during evacuation
d1f9b8a: 8234808: jdb quoted option parsing broken
d8921ed: 8247281: migrate ObjectMonitor::_object to OopStorage
f800af9: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
2e30ff6: 8253311: Cleanup relocInfo constructors
... and 46 more: https://git.openjdk.java.net/jdk/compare/a9993f9464a724637b3d38bd2f6caedf884d3e0b...master

Your commit was automatically rebased without conflicts.

Pushed as commit b9729cb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

r18 should not be used as it is reserved as platform register. Linux is fine with userspace using it, but Windows and also recently macOS ( openjdk/jdk11u-dev#301 (comment) ) are actually using it on the kernel side. The macro assembler uses the bit pattern `0x7fffffff` (== `r0-r30`) to specify which registers to spill; fortunately this helper is only used here: https://github.com/openjdk/jdk/blob/c05dc268acaf87236f30cf700ea3ac778e3b20e5/src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp#L1400-L1404 I haven't seen causing this particular instance any issues in practice _yet_, presumably because it looks hard to align the stars in order to trigger a problem (between stp and ldp of r18 a transition to kernel space must happen *and* the kernel needs to do something with r18). But jdk11u-dev has more usages of the `::pusha`/`::popa` macro and that causes troubles as explained in the link above. Output of `-XX:+PrintInterpreter` before this change: ``` ---------------------------------------------------------------------- method entry point (kind = native) [0x0000000138809b00, 0x000000013880a280] 1920 bytes -------------------------------------------------------------------------------- 0x0000000138809b00: ldr x2, [x12, #16] 0x0000000138809b04: ldrh w2, [x2, #44] 0x0000000138809b08: add x24, x20, x2, uxtx #3 0x0000000138809b0c: sub x24, x24, #0x8 [...] 0x0000000138809fa4: stp x16, x17, [sp, #128] 0x0000000138809fa8: stp x18, x19, [sp, #144] 0x0000000138809fac: stp x20, x21, [sp, #160] [...] 0x0000000138809fc0: stp x30, xzr, [sp, #240] 0x0000000138809fc4: mov x0, x28 ;; 0x10864ACCC 0x0000000138809fc8: mov x9, #0xaccc // #44236 0x0000000138809fcc: movk x9, #0x864, lsl #16 0x0000000138809fd0: movk x9, #0x1, lsl #32 0x0000000138809fd4: blr x9 0x0000000138809fd8: ldp x2, x3, [sp, #16] [...] 0x0000000138809ff4: ldp x16, x17, [sp, #128] 0x0000000138809ff8: ldp x18, x19, [sp, #144] 0x0000000138809ffc: ldp x20, x21, [sp, #160] ``` After: ``` ---------------------------------------------------------------------- method entry point (kind = native) [0x0000000108e4db00, 0x0000000108e4e280] 1920 bytes -------------------------------------------------------------------------------- 0x0000000108e4db00: ldr x2, [x12, #16] 0x0000000108e4db04: ldrh w2, [x2, #44] 0x0000000108e4db08: add x24, x20, x2, uxtx #3 0x0000000108e4db0c: sub x24, x24, #0x8 [...] 0x0000000108e4dfa4: stp x16, x17, [sp, #128] 0x0000000108e4dfa8: stp x19, x20, [sp, #144] 0x0000000108e4dfac: stp x21, x22, [sp, #160] [...] 0x0000000108e4dfbc: stp x29, x30, [sp, #224] 0x0000000108e4dfc0: mov x0, x28 ;; 0x107E4A06C 0x0000000108e4dfc4: mov x9, #0xa06c // #41068 0x0000000108e4dfc8: movk x9, #0x7e4, lsl #16 0x0000000108e4dfcc: movk x9, #0x1, lsl #32 0x0000000108e4dfd0: blr x9 0x0000000108e4dfd4: ldp x2, x3, [sp, #16] [...] 0x0000000108e4dff0: ldp x16, x17, [sp, #128] 0x0000000108e4dff4: ldp x19, x20, [sp, #144] 0x0000000108e4dff8: ldp x21, x22, [sp, #160] [...] ```

shipilev added 2 commits September 17, 2020 13:12

8253284: Zero OrderAccess barrier mappings are incorrect

9963407

Mirror the same in bsd_zero.hpp

012a59a

openjdk bot added the rfr Pull request is ready for review label Sep 17, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 17, 2020

dholmes-ora suggested changes Sep 17, 2020

View reviewed changes

src/hotspot/os_cpu/bsd_zero/orderAccess_bsd_zero.hpp Show resolved Hide resolved

Fix copy-paste omission in bsd_zero

add14aa

shipilev requested a review from dholmes-ora September 18, 2020 09:07

theRealAph approved these changes Sep 18, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 18, 2020

openjdk bot removed the ready Pull request is ready to be integrated label Sep 18, 2020

gnu-andrew approved these changes Sep 18, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 18, 2020

dholmes-ora approved these changes Sep 20, 2020

View reviewed changes

openjdk bot closed this Sep 22, 2020

openjdk bot added the integrated Pull request has been integrated label Sep 22, 2020

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 22, 2020

shipilev deleted the JDK-8253284-zero-barriers branch September 22, 2020 12:44

8253284: Zero OrderAccess barrier mappings are incorrect #224

8253284: Zero OrderAccess barrier mappings are incorrect #224

Uh oh!

Conversation

shipilev commented Sep 17, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Sep 17, 2020

Uh oh!

openjdk bot commented Sep 17, 2020

Uh oh!

mlbridge bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnu-andrew commented Sep 18, 2020

Uh oh!

shipilev commented Sep 18, 2020

Uh oh!

mlbridge bot commented Sep 18, 2020

Uh oh!

openjdk bot commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shipilev commented Sep 18, 2020

Uh oh!

openjdk bot commented Sep 18, 2020

Uh oh!

gnu-andrew commented Sep 18, 2020

Uh oh!

openjdk bot commented Sep 18, 2020

Uh oh!

gnu-andrew commented Sep 18, 2020

Uh oh!

shipilev commented Sep 19, 2020

Uh oh!

dholmes-ora commented Sep 20, 2020

Uh oh!

shipilev commented Sep 22, 2020

Uh oh!

openjdk bot commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

shipilev commented Sep 17, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 17, 2020 •

edited

Loading

openjdk bot commented Sep 18, 2020 •

edited

Loading