RFR(M): 8202080: Introduce ordering semantics for Atomic::add and other RMW atomics
David Holmes
david.holmes at oracle.com
Mon May 7 07:06:35 UTC 2018
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