RFR(M): 8202080: Introduce ordering semantics for Atomic::add

[Erik Österlund]

Hi Martin and David,

I for one welcome using C++11 atomic semantics, and would like to 
outline why.

1) It is compliant with the JMM. Work has been done to unify the two. I 
think I outlined for my access decorators which JMM property links to 
each MO_ decorator, which corresponds to a C++11 semantic, that I also 
have a short description of.
2) It has a well defined meaning, as lots of people with more time on 
their hands than us have spent many hours defining what it means.
3) It is well known, and new developers that know either of C++11 
atomics or the JMM can immediately start working with it.
4) Since it is so well-known, compilers know about it too. For platforms 
that don't offer a whole bunch of ABIs for these semantics, like x86, we 
have the opportunity to use intrinsics, and let the compiler know what 
we are trying to do. This allows more freedom such as using consume, 
that we can not represent with volatiles and inline assembly.
5) For similar reasons, the hardware know about it too. If we had a 
notion of seq_cst (which is what this counter needs), the AArch64 
hardware could provide the most appropriate instructions that map 
directly to that.
6) For our TSO platforms, it will make little difference. But for SAP 
and RedHat, this makes a much bigger difference, to the extent that PPC 
can not pragmatically use the conservative model we have provided. 
Therefore we have ended up in a situation where we have a contract that 
is not followed, and people like me writing workarounds out of 
politeness. I would rather have a weaker contract I can rely on, that is 
actually implemented properly in all backends, than have a strong 
contract that looks great, but is too unpragmatic to be implemented in 
the very backends where it matters the most, and hence is unreliable in 
practice.
7) The weaker contract forces us to think more about our lock-free 
interactions and why it works. But I would argue we should not write 
lock-free code if we do not understand why it works anyway.

Thanks,
/Erik

On 2018-04-21 01:08, David Holmes wrote:
> Hi Martin,
>
> You've added:
>
>   enum cmpxchg_memory_order {
>     memory_order_relaxed = 0,
> !   memory_order_acquire = 2,
> !   memory_order_release = 3,
> !   memory_order_acq_rel = 4,
>
> but not defined them. What do they mean? Are they the same as 
> C++11/17? Do we actually want that? There was a discussion on all this 
> when the enum was introduced. Also note the enum is named 
> cmpxchg_memory_order - if we're going to generalise this then it 
> should be renamed and behaviour clearly specified.
>
> !   inline static D add(I add_value, D volatile* dest,
> !                       cmpxchg_memory_order order = 
> memory_order_acq_rel);
>
> The default should be conservative - to match what is stated in 
> atomic.hpp regarding expectations for all atomic r-m-w operations.
>
>  63   // All of the atomic operations that imply a read-modify-write 
> action
>   64   // guarantee a two-way memory barrier across that operation. 
> Historically
>   65   // these semantics reflect the strength of atomic operations 
> that are
>   66   // provided on SPARC/X86. We assume that strength is necessary 
> unless
>   67   // we can prove that a weaker form is sufficiently safe.
>
> That's how "conservative" was defined when this was introduced. Some 
> of the ports chose to not follow this "contract" and that's up to 
> them, but only those ports should be utilising weaker operations 
> unless it can be proven that a weaker form is always correct. The onus 
> is on the person changing the code. (There was long discussion when a 
> relaxed cmpxchg was attempted to be introduced and it was shown to not 
> be correct.)
>
> David
> -----
>
> On 21/04/2018 2:04 AM, Doerr, Martin wrote:
>> Hi,
>>
>> JDK-8195099 “Concurrent safe-memory-reclamation mechanism” introduced 
>> a usage of Atomic::add for which we’re not using the correct memory 
>> barriers on PPC64 and s390.
>>
>> Some more description can be found in the bug: 
>> https://bugs.openjdk.java.net/browse/JDK-8202080
>>
>> We can fix this by the following change:
>>
>> http://cr.openjdk.java.net/~mdoerr/8202080_atomic_add/webrev.00/
>>
>> Please note that I only implemented the platform code for linux x86, 
>> PPC64 and s390 so far. The remaining ones should be trivial and I’ll 
>> take care of them after I got some feedback.
>>
>> I can also do the same change for other atomic functions like sub, 
>> inc and dec if you like.
>>
>> Intention of this change is only to fix this new code and possibly 
>> other future enhancements, not to change the behavior of any older 
>> usages of Atomic::add.
>>
>> Feedback is welcome.
>>
>> Best regards,
>>
>> Martin
>>