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

[Derek White]

Hi Martin,

You are right about the non-impact on arm32 or aarch64. I read over the existing implementation too quickly. Sorry!

And I agree that we should enable CPU_MULTI_COPY_ATOMIC as a separate issue.

So no more issues on this from me.

Thanks,
 - Derek

-----Original Message-----
From: Doerr, Martin <martin.doerr at sap.com> 
Sent: Thursday, August 15, 2019 12:29 PM
To: Derek White <derekw at marvell.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: [EXT] RE: RFR(S): 8229422: Taskqueue: Outdated selection of weak memory model platforms

----------------------------------------------------------------------
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