RFR(S): 8229422: Taskqueue: Outdated selection of weak memory model platforms

[Doerr, Martin]

Hi Derek,

thanks for pointing me to the aarch64 issue and paper.
I have added a link to that issue.

> 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?).

My initial webrev is only a functional change for s390.

Current implementation:
- CPU_NOT_MULTIPLE_COPY_ATOMIC is only used to control the following:
  support_IRIW_for_not_multiple_copy_atomic_cpu: only true on PPC64
- TaskQueue uses fence on all platforms except SPARC and x86

My webrev:
- Still:
  support_IRIW_for_not_multiple_copy_atomic_cpu: only true on PPC64
- TaskQueue uses fence on all platforms except SPARC, x86 and s390


According to the paper, we could enable CPU_MULTI_COPY_ATOMIC on aarch64 (not arm32). But I think this should be worth a separate issue once we have decided how to proceed with this one.
I think it'd be also good to have support_IRIW_for_not_multiple_copy_atomic_cpu switchable for PPC64.

> In any case I think it should be reviewed beyond the context of hotspot-gc-dev.
Definitely, yes. But since the taskqueue belongs to GC, I'd like to get feedback from GC, first.

It is always surprising how many ideas and opinions show up when touching this code 😊

Best regards,
Martin


> -----Original Message-----
> From: Derek White <derekw at marvell.com>
> Sent: Donnerstag, 15. August 2019 17:50
> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes
> <david.holmes at oracle.com>; hotspot-gc-dev at openjdk.java.net; Kim
> Barrett <kim.barrett at oracle.com>; Andrew Haley <aph at redhat.com>
> Subject: RE: RFR(S): 8229422: Taskqueue: Outdated selection of weak
> memory model platforms
> 
> Hi Martin,
> 
> 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 [1][2] 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!
> 
> 	[1] https://www.cl.cam.ac.uk/~pes20/armv8-mca/
> 	[2] https://bugs.openjdk.java.net/browse/JDK-8165058
> 
> 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!
>  - Derek
> 
> -----Original Message-----
> 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
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Kim and David,
> 
> thank you for looking into this issue.
> 
> @Kim:
> I've replied to your comment in the issue.
> 
> @David:
> > 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:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2013-
> March/008853.html
> 
> 
> Best regards,
> Martin