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

[Doerr, Martin]

Hi David,

thanks for your reply.

The new "memory_order_" enums are supposed to be a step towards C++2011. I think only the memory_order_seq_cst is not so clear (because the PPC64 reference implementation doesn't have a trailing full fence) which is why I haven't introduced that one, yet. The other ones should be the same as for C++2011. Would you like to have some kind of additional comments?

I was already thinking about renaming "cmpxchg_memory_order" to something like "atomic_memory_order". I'll do that when I create a new webrev.

If we were rewriting the VM from scratch, I'd vote for enforcing specifying the semantics by not providing a default. But with the existing code, I'm not sure if doing this is desirable. It would be a very large change to add the semantics to all Atomic usages.
 
> There was long discussion when a relaxed cmpxchg was attempted to be introduced and it was shown to not be correct.
I guess you're referring to JDK-8154736 "enhancement of cmpxchg and copy_to_survivor for ppc64". The early proposal was not correct because of a missing release barrier. But the key issue is that the full fences can be omitted or replaced by weaker barriers. I think IBM still wants to contribute this change (with correct barriers) which makes sense to me.

Using release and/or acquire is affordable, but the double-fence is a hammer. I guess nobody in the world would want to use that by default on PPC64. So I basically agree with Erik's reply to this mail thread.

Best regards,
Martin


-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com] 
Sent: Samstag, 21. April 2018 01:08
To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net; Erik Österlund <erik.osterlund at oracle.com>; 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 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
>