RFR(M): 8202080: Introduce ordering semantics for Atomic::add and other RMW atomics

Fri May 4 15:32:48 UTC 2018

Hi David,

thanks for your time. I understand that there's a lot of work at the moment.

I have removed memory_order_consume again. We don't need it for our platforms and I guess nobody will miss it.

I like your comment proposals, so I've updated them.

New webrev:
http://cr.openjdk.java.net/~mdoerr/8202080_atomic_add/webrev.02/

> Though that raises the question why you haven't added memory_order_seq_cst?
I think that one could easily be used incorrectly within hotspot which would lead to problems on PPC.
"Atomic operations tagged memory_order_seq_cst ... establish a single total modification order of all atomic operations that are so tagged." [1]
It should work fine if all involved accesses are "so tagged". But hotspot doesn't use seq_cst loads, it uses things like OrderAccess::load_acquire. On PPC, GCC and xlC don't use a trailing sync for seq_cst stores and RMW-atomics so they only work well with leading sync loads which are not used in hotspot code.
I believe that there's a substantial amount of work to be done to get this right.

Thanks,
Martin

[1] https://en.cppreference.com/w/cpp/atomic/memory_order#Sequentially-consistent_ordering

-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com] 
Sent: Freitag, 4. Mai 2018 08:09
To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(M): 8202080: Introduce ordering semantics for Atomic::add and other RMW atomics

Hi Martin,

On 3/05/2018 11:03 PM, Doerr, Martin wrote:
> Hi Robbin,
> 
> thank you for reviewing.
> 
> Submission forest tests have passed in addition to SAP's nightly builds. I have 2 reviews, now, but I'll wait a little bit before pushing.

Thanks for waiting. I think it important there is a general buy-in to 
this effort, not just the official "get a review and push". Apologies 
that it's taken me a while to get to this.

memory_order_consume has been deprecated in C++17:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0371r1.html

I would suggest we not add it to the VM.

   40 enum atomic_memory_order {
   41   // We support most semantics like in C++11.

I would say something like:

// The modes that align with C++11 are intended to
// follow the same semantics.

47   // We need to be more conservative than seq_cst on PPC64.

Not sure the PPC64 comment is relevant here. memory_order_conservative 
represents the pre-existing strong fencing requirements. It's not 
intended to map to anything else, nor intended to be different across 
platforms.

Though that raises the question why you haven't added memory_order_seq_cst?

Looking at all the platform code that adds the unused "order" parameter 
... I would not be surprised if the forthcoming compiler upgrades result 
in some additional "unused" warnings. But I guess we'll just deal with 
them as they are discovered.

Otherwise all seems okay.

Thanks,
David

> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Robbin Ehn [mailto:robbin.ehn at oracle.com]
> Sent: Donnerstag, 3. Mai 2018 14:29
> To: Doerr, Martin <martin.doerr at sap.com>; Schmidt, Lutz <lutz.schmidt at sap.com>; David Holmes <david.holmes at oracle.com>; Erik Österlund <erik.osterlund at oracle.com>; hotspot-runtime-dev at openjdk.java.net; Aleksey Shipilev <shade at redhat.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Michihiro Horie (HORIE at jp.ibm.com) <HORIE at jp.ibm.com>; Andrew Haley (aph at redhat.com) <aph at redhat.com>; John Paul Adrian Glaubitz <glaubitz at physik.fu-berlin.de>
> Subject: Re: RFR(M): 8202080: Introduce ordering semantics for Atomic::add and other RMW atomics
> 
> On 2018-05-03 12:50, Doerr, Martin wrote:
>> Hi Lutz,
>>
>> thanks for reviewing.
>>
>> Can I get more reviews, please?
> 
> I think this looks good, thanks for fixing!
> 
> /Robbin
> 
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Schmidt, Lutz
>> Sent: Mittwoch, 2. Mai 2018 16:20
>> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes <david.holmes at oracle.com>; Erik Österlund <erik.osterlund at oracle.com>; hotspot-runtime-dev at openjdk.java.net; Aleksey Shipilev <shade at redhat.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Michihiro Horie (HORIE at jp.ibm.com) <HORIE at jp.ibm.com>; Andrew Haley (aph at redhat.com) <aph at redhat.com>; John Paul Adrian Glaubitz <glaubitz at physik.fu-berlin.de>
>> Subject: Re: RFR(M): 8202080: Introduce ordering semantics for Atomic::add and other RMW atomics
>>
>> Hi Martin,
>>
>> your changes look good.
>>
>> For the additional synchronization on s390 in particular, I do not expect a serious performance impact. The checkpoint synchronization part has always been the expensive, but rarely required, part. PPC may be another story. We knoow that a full-blown sync really hurts. We will see...
>>
>> Thanks,
>> Lutz
>>
>> On 26.04.18, 16:23, "hotspot-runtime-dev on behalf of Doerr, Martin" <hotspot-runtime-dev-bounces at openjdk.java.net on behalf of martin.doerr at sap.com> wrote:
>>
>>       Hi,
>>       
>>       I have renamed "cmpxchg_memory_order" to "atomic_memory_order" and added support to all Read-Modify-Write atomics with support for all C++11 semantics except seq_cst. The latter has issues on PPC64 so we're currently using our own memory_order_conservative instead, which is the default.
>>       
>>       Please review my new webrev:
>>       http://cr.openjdk.java.net/~mdoerr/8202080_atomic_add/webrev.01/
>>       
>>       We'll test Windows, MacOS, linux x86+ppc64+s390, AIX and Solaris SPARC.
>>       It'd be great if somebody could check arm/aarch64 and zero.
>>       
>>       This change may enable optimizations for arm/aarch64 (not yet included).
>>       
>>       Best regards,
>>       Martin
>>       
>>       
>>       -----Original Message-----
>>       From: Doerr, Martin
>>       Sent: Donnerstag, 26. April 2018 11:20
>>       To: 'David Holmes' <david.holmes at oracle.com>; Erik Österlund <erik.osterlund at oracle.com>; hotspot-runtime-dev at openjdk.java.net; Aleksey Shipilev <shade at redhat.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Michihiro Horie (HORIE at jp.ibm.com) <HORIE at jp.ibm.com>
>>       Subject: RE: RFR(M): 8202080: Introduce ordering semantics for Atomic::add
>>       
>>       Hi David,
>>       
>>       sounds like we are on the same page, now. I think we should have some kind of summary of what was analyzed. But that belongs to the other thread (JDK- 8154736).
>>       
>>       > The bugs in this kind of code are very subtle and rarely exposed through normal levels of testing.
>>       Right. We have experienced this often enough. So it is definitely in our interest to have correct memory barriers. Mistakes will hit us (PPC64). So I appreciate that you insist on careful analysis.
>>       
>>       I'll prepare a new webrev with conservative semantics for all read-modify-write atomics by default.
>>       
>>       Best regards,
>>       Martin
>>       
>>       
>>