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

David Holmes david.holmes at oracle.com
Mon May 7 12:16:25 UTC 2018 

On 7/05/2018 8:47 PM, Doerr, Martin wrote:
> Hi David,
> 
> thanks for the review. I've pushed it.
> 
> About the seq_cst topic:
> I could imagine supporting memory_order_seq_cst, but IMHO it will only be really useful if we add something equivalent to C++11's x.load(std::memory_order_seq_cst) and x.store(y, std::memory_order_seq_cst).
> Having it only for the RMW-atomics without being able to tag load/store the same way sounds very poor to me.
> In contrast, the release/acquire semantics already make sense to me because they respect all other accesses, not only the "so tagged" ones.

I'm not sure how to reconcile that with the existing status-quo where we 
have "conservative" Atomic-rmw combined with acquire/release accesses. 
Is seq-cst in some sense stronger than our "conservative" ordering ??

Cheers,
David

> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Montag, 7. Mai 2018 09:07
> 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 5/05/2018 1:32 AM, Doerr, Martin wrote:
>> 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/
> 
> Changes seem fine. Thanks.
> 
>>> 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.
> 
> I find this somewhat ironic/amusing. My main concern with adding in any
> access modes weaker than the existing "conservative" is precisely
> because it can be very hard to establish that they are in fact correct. :)
> 
> But I don't follow your concern. The "total modification order" is in
> addition to the acquire/release semantics that are inherent in seq-cst.
> So if the seq-cst atomic-rmw operation is used on a variable that is
> otherwise accessed via load_acquire/release_store, then I would expect
> everything to be okay - else it serves no point to define seq-cst rmw in
> terms of acquire and release IMHO. I would expect these access modes to
> interact in the "obvious" way.
> 
> Cheers,
> David
> 
>> 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
>>>>         
>>>>         
>>>>

More information about the hotspot-runtime-dev mailing list