RFR(M): 8202080: Introduce ordering semantics for Atomic::add and other RMW atomics
Doerr, Martin
martin.doerr at sap.com
Mon May 7 15:01:55 UTC 2018
Hi David,
"conservative" is not broken because the two-way sync makes it compatible with any loads and stores.
"seq_cst" doesn't generally specify if leading or trailing sync is used so it only has clear store-load ordering semantics with respect to other "seq_cst" accesses.
I think Erik's comparison with the JMM is nice.
Seq_cst accesses behave like Java volatile accesses with respect to other accesses. (Atomic RMW operations are both, read and write.) Java volatiles can be implemented as leading or trailing sync, too. At least until "JEP 188: Java Memory Model Update" clarifies this.
Best regards,
Martin
-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com]
Sent: Montag, 7. Mai 2018 14:50
To: Erik Österlund <erik.osterlund at oracle.com>; 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 Erik,
On 7/05/2018 10:32 PM, Erik Österlund wrote:
> Hi David,
>
> On 2018-05-07 14:16, David Holmes wrote:
>> 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 ??
>
> Seq cst is strictly weaker than "conservative". Conservative has full
> trailing and leading fence. Seq cst basically has either full leading or
> trailing fence, depending on choice of bindings. That makes pairs of seq
> cst accesses totally ordered, but has potentially unintuitive
> consequences when mixing with weaker accesses. So ideally, we would have
> seq_cst loads and stores that could be used with those RMW operations.
Well I'm confused. If seq-cst is weaker than what we have and seq-cst
would be broken with only load-acquire/release-store, then doesn't that
imply what we have with "conservative" is also broken??
> I personally don't see a problem with introducing seq cst as an ordering
> you can choose for both RMW and loads/stores, that binds to precisely
> what we already have today for JMM volatile. That way, my Access API can
> call straight into OrderAccess/Atomic for such uses, instead of doing
> the conditional fence dance outside. But I don't mind if we do not either.
I'm not following what you are saying about JMM volatile here. The
semantics for JMM volatile are best expressed as barriers required
between pairs of accesses not the current way we deal with them as
treating each access as if it were preceded and followed by another
access that required the strongest barrier between them.
David
> Naturally, as with every other access, we have to take care that we know
> what we are doing when introducing this in lock-free code. But that
> arguably goes for the other ordering semantics as well.
>
> Thanks,
> /Erik
>
>> 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