RFR(S): 8229422: Taskqueue: Outdated selection of weak memory model platforms
derekw at marvell.com
Thu Aug 15 15:49:37 UTC 2019
This is a good area to clean up! I have 2 issues to consider with this patch:
Re - the AArch64 side of things:
Arm retconned the ARMv8 spec  to decide that multi-copy atomicity was a good idea after all (after checking that no CPU implementations took advantage of this level of weakness).
However, setting CPU_MULTI_COPY_ATOMIC for AArch64 would result in changing behavior (removing fence in taskqueue) that should be looked at and tested by the aarch64 folks, so if Andrew Haley agrees, I suggest deferring changing this AArch64 behavior to a separate issue.
BTW, this could be a nice improvement for AArch64 - Thanks for bringing this up!
Re - the patch generally:
There changes around CPU_NOT_MULTIPLE_COPY_ATOMIC in src/hotspot/share/utilities/globalDefinitions.hpp are not simply a change to GC behavior for S390, but are changing interpreter and compiler behavior arm32 (and maybe Aarch64?).
I believe this change will remove required fences from arm32 interpreter and Jitted code relating to JMM volatile accesses.
In any case I think it should be reviewed beyond the context of hotspot-gc-dev.
Thanks again bringing this up!
From: hotspot-gc-dev <hotspot-gc-dev-bounces at openjdk.java.net> On Behalf Of Doerr, Martin
Sent: Tuesday, August 13, 2019 6:30 AM
To: David Holmes <david.holmes at oracle.com>; hotspot-gc-dev at openjdk.java.net; Kim Barrett <kim.barrett at oracle.com>
Subject: [EXT] RE: RFR(S): 8229422: Taskqueue: Outdated selection of weak memory model platforms
Hi Kim and David,
thank you for looking into this issue.
I've replied to your comment in the issue.
> I find the inversion of the ifdef slightly confusing. I also don't
> like a comment to say we don't have a given property. Wouldn't it be
> better to set CPU_MULTI_COPY_ATOMIC to 0 or 1 as appropriate?
Hmm. We could change that. I'm not sure what is better.
I think it should be designed such that correct usage is easy and wrong usage is difficult.
It has already happened that people used an #ifdef for a macro which is always defined (0 or 1) by mistake.
That's why I'm not a big fan of defining things to 0 or 1.
With the #define or not define approach, all platforms except those which explicitly specify the property are conservatively treated as non-multi-copy atomic.
But if your version is preferred by all reviewers, I can use it.
> Can't comment on ppc64 specifics.
I'll ask for additional reviews once the main issue was reviewed.
> It's not at all obvious to me that the need for the fence() in
> pop_global is directly related to CPU_MULTI_COPY_ATOMIC. I prefer to
> see that define connected only with the IRIW issue as it currently is.
This was explained in the email thread a few emails later:
More information about the hotspot-gc-dev