RFR(M): 8155949: Support relaxed semantics in cmpxchg

[Doerr, Martin]

Hi Kim,

thank you very much for the detailed review.

I agree with your comments and I have made all your requested changes here:
http://cr.openjdk.java.net/~goetz/wr16/8155949-relaxed_cas/webrev.03/

It's correct that the change changes the semantics of the conservative cmpxchg. In case of failure, we also execute the sync instruction, now.
Advantage is that the new implementation is maximum conservative by default. I think this makes sense as long as the semantics of the hotspot C++ cmpxchg are not clearly specified.

For performance optimization, we should better use (or introduce additional) enum values.

Thanks and best regards,
Martin


-----Original Message-----
From: Kim Barrett [mailto:kim.barrett at oracle.com] 
Sent: Mittwoch, 18. Mai 2016 03:26
To: Doerr, Martin <martin.doerr at sap.com>
Cc: Hiroshi H Horii <HORII at jp.ibm.com>; David Holmes <david.holmes at oracle.com>; Tim Ellison <Tim_Ellison at uk.ibm.com>; ppc-aix-port-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(M): 8155949: Support relaxed semantics in cmpxchg

> On May 10, 2016, at 10:27 AM, Doerr, Martin <martin.doerr at sap.com> wrote:
> 
> Hello everybody,
>  
> thanks for finding this issue. New webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.02/
>  

------------------------------------------------------------------------------ 
src/share/vm/runtime/atomic.hpp
  30 typedef enum cmpxchg_cmpxchg_memory_order {
  31   memory_order_relaxed,
  32   // Use value which doesn't interfere with C++2011. We need to be more conservative.
  33   memory_order_conservative = 8
  34 } cmpxchg_memory_order;

This is C++, where enum tag names are types, so we don't need a
typedef here. Just use "enum cmpxchg_memory_order { ... };".

------------------------------------------------------------------------------ 
src/share/vm/runtime/atomic.cpp
59 unsigned Atomic::cmpxchg(unsigned int exchange_value,
  60                            volatile unsigned int* dest, unsigned int compare_value,
  61                            cmpxchg_memory_order order) {

Misaligned parameters.

I'm surprised this was ever out-of-line. But with this change it's
quite bad to be out-of-line, as that's going to kill the constant
propogation of the order value.

------------------------------------------------------------------------------ 
src/os_cpu/linux_ppc/vm/atomic_linux_ppc.inline.hpp 

In each use, the cmpxchg_post_membar's are after the exit label,
whereas the acquire fences they are replacing were before the exit
label.  This means we'll be fencing on failure exit, where we weren't
doing so before.

It's not clear whether this change is intentional.  Note that this
change is consistent with the C++11 one-order forms of cmpxchg, where
the single order argument is used as the sucess order and (with
potentially some modification) as the failure order.

[I was going to suggest the asm goto syntax might be used to obtain
the original ordering, but "An asm goto statement cannot have
outputs." So some non-trivial restructuring will probably be needed to
get the original ordering.]

Similarly in src/os_cpu/aix_ppc/vm/atomic_aix_ppc.inline.hpp. 

------------------------------------------------------------------------------ 
src/os_cpu/linux_ppc/vm/atomic_linux_ppc.inline.hpp 

These repeated comments need updating:
 315   // Note that cmpxchg guarantees a two-way memory barrier across
 316   // the cmpxchg, so it's really a a 'fence_cmpxchg_acquire'
 317   // (see atomic.hpp).

Similarly in src/os_cpu/aix_ppc/vm/atomic_aix_ppc.inline.hpp. 

------------------------------------------------------------------------------ 
src/os_cpu/linux_ppc/vm/atomic_linux_ppc.inline.hpp 

[pre-existing]

The cmpxchg asm sequences all originally looked like

  /* fence */
  strasm_sync
  ...
  /* acquire */
  strasm_sync
  ...

So they were using strasm_sync (the full fence) in both places, even
though the comments suggest it could/should have been
  strasm_fence ... strasm_acquire
However, the description in runtime/atomic.hpp seems to indicate
something stronger than "acquire" is required here, so the second
comment seems wrong. Maybe its a good thing the comments are being
removed by the proposed change.

Similarly in other corresponding places in other files.

------------------------------------------------------------------------------ 
src/os_cpu/linux_aarch64/vm/atomic_linux_aarch64.inline.hpp
 138   return (void *) cmpxchg_ptr((intptr_t) exchange_value,
 139                               (volatile intptr_t*) dest,
 140                               (intptr_t) compare_value, order);

I'd prefer the order argument be placed on its own line, rather than
added to an existing line where it's kind of hiding.

Similarly in other corresponding places in other files.

------------------------------------------------------------------------------ 
src/os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp 
 271 inline jint Atomic::cmpxchg(jint exchange_value,
 272                             volatile jint* dest,
 273                             jint compare_value, cmpxchg_memory_order order) {

and similarly elsewhere, I'd prefer the order parameter be on it's own
line like the other parameters.

Similarly in other corresponding places here and in other files.

------------------------------------------------------------------------------
src/os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp 
 306 inline void* Atomic::cmpxchg_ptr(void* exchange_value,
 307                                  volatile void* dest,
 308                                  void* compare_value, cmpxchg_memory_order order) {
 309 
 310   return (void *) cmpxchg_ptr((intptr_t) exchange_value,
 311                               (volatile intptr_t*) dest,
 312                               (intptr_t) compare_value);
 313 }

Inner cmpxchg_ptr is missing the order argument. This will discard an
outer relaxed order.  (atomic_linux_zero is OK.)

------------------------------------------------------------------------------ 
src/share/vm/runtime/atomic.inline.hpp

The unspecialized Atomic::cmpxchg for jbyte isn't passing the order
argument through to cmpxchg_general.  Of course, then we might want to
figure out what cmpxchg_general should be doing with the order.

------------------------------------------------------------------------------