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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Thu May 12 08:50:09 UTC 2016 

Hi,

atomic_bsd_zero.inline.hpp:303
The order argument is not passed on to the inner cmpxchg_ptr call.
But I guess this is not really relevant as the argument is not used 
anyways.  (This method should be moved to the shared atomic.inline.hpp 
file, but not in this change.)

Besides that the change looks good.   Reviewed.

In case this now really is too late (http://openjdk.java.net/projects/jdk9/ states 26.5. for dev close, but hs is more early?)
will there be jdk10 repos soon, or jdk9u?

Best regards,
  Goetz.





> -----Original Message-----
> From: hotspot-gc-dev [mailto:hotspot-gc-dev-bounces at openjdk.java.net]
> On Behalf Of David Holmes
> Sent: Donnerstag, 12. Mai 2016 00:50
> To: Doerr, Martin <martin.doerr at sap.com>; Hiroshi H Horii
> <HORII at jp.ibm.com>
> Cc: Tim Ellison <Tim_Ellison at uk.ibm.com>; ppc-aix-port-
> dev at openjdk.java.net; hotspot-dev developers <hotspot-
> 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
> 
> This has about 3 hours to be reviewed and pushed to make the FC deadline.
> 
> David
> 
> On 11/05/2016 2:41 PM, David Holmes wrote:
> > Adding hotspot-dev to cc to expand scope of reviewer pool :)
> >
> > On 11/05/2016 6:56 AM, David Holmes wrote:
> >> On 11/05/2016 12:27 AM, Doerr, Martin wrote:
> >>> Hello everybody,
> >>>
> >>> thanks for finding this issue. New webrev is here:
> >>>
> >>> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.02/
> >>
> >> Unfortunately my test run hit a crash on Solaris sparc:
> >>
> >> # Problematic frame:
> >> # V  [libjvm.so+0xcc35c4]
> >> markOopDesc*markOopDesc::displaced_mark_helper()const+0x64
> >>
> >> I'm going to have to do some more testing to see if that is actually
> >> related to the change. I know it should not be, but given we CAS marks I
> >> have to wonder if there's some subtle bad interaction here. :(
> >
> > Further testing has not shown any failures on Solaris sparc, and the
> > same testing showed some spurious failures on other platforms even
> > without these changes. So while I will file a bug for this crash I think
> > it unlikely to be related to the current changes.
> >
> > So on that note we just need a second hotspot reviewer to sign off on this.
> >
> > Thanks,
> > David
> >
> >
> >> David
> >> -----
> >>
> >>>
> >>>
> >>> Best regards,
> >>>
> >>> Martin
> >>>
> >>>
> >>>
> >>> *From:*Hiroshi H Horii [mailto:HORII at jp.ibm.com]
> >>> *Sent:* Dienstag, 10. Mai 2016 15:18
> >>> *To:* David Holmes <david.holmes at oracle.com>
> >>> *Cc:* hotspot-gc-dev at openjdk.java.net;
> >>> hotspot-runtime-dev at openjdk.java.net; ppc-aix-port-
> dev at openjdk.java.net;
> >>> Tim Ellison <Tim_Ellison at uk.ibm.com>; Doerr, Martin
> >>> <martin.doerr at sap.com>
> >>> *Subject:* Re: RFR(M): 8155949: Support relaxed semantics in cmpxchg
> >>>
> >>>
> >>>
> >>> Hi David,
> >>>
> >>>> Just need another reviewer to chime in - given you and Martin are both
> >>>> contributors. Or are you the main contributor with Martin being a
> >>>> reviewer?
> >>>
> >>> Martin and I are contributors of this change.
> >>>
> >>>> Still a problem on Solaris sparc:
> >>>
> >>> Martin, could you create a new change in webrev with the patch that
> >>> David sent?
> >>>
> >>> Regards,
> >>> Hiroshi
> >>> -----------------------
> >>> Hiroshi Horii, Ph.D.
> >>> IBM Research - Tokyo
> >>>
> >>>
> >>> David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>
> >>> wrote on 05/10/2016 21:29:53:
> >>>
> >>>> From: David Holmes <david.holmes at oracle.com
> >>>> <mailto:david.holmes at oracle.com>>
> >>>> To: Hiroshi H Horii/Japan/IBM at IBMJP, "hotspot-runtime-
> >>>> dev at openjdk.java.net <mailto:dev at openjdk.java.net>"
> >>> <hotspot-runtime-dev at openjdk.java.net
> >>> <mailto:hotspot-runtime-dev at openjdk.java.net>>
> >>>> Cc: Tim Ellison <Tim_Ellison at uk.ibm.com
> >>>> <mailto:Tim_Ellison at uk.ibm.com>>, "ppc-aix-port-
> >>>> dev at openjdk.java.net <mailto:dev at openjdk.java.net>"
> >>> <ppc-aix-port-dev at openjdk.java.net
> >>> <mailto:ppc-aix-port-dev at openjdk.java.net>>, "hotspot-
> >>>> gc-dev at openjdk.java.net <mailto:gc-dev at openjdk.java.net>"
> >>> <hotspot-gc-dev at openjdk.java.net
> >>> <mailto:hotspot-gc-dev at openjdk.java.net>>
> >>>> Date: 05/10/2016 21:31
> >>>> Subject: Re: RFR(M): 8155949: Support relaxed semantics in cmpxchg
> >>>>
> >>>> On 10/05/2016 9:04 PM, David Holmes wrote:
> >>>> > Hi Hiroshi,
> >>>> >
> >>>> > On 10/05/2016 8:44 PM, Hiroshi H Horii wrote:
> >>>> >> Hi All,
> >>>> >>
> >>>> >> Can I please request reviews for the following change?
> >>>> >>
> >>>> >> Code change:
> >>>> >>
> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.01/
> >>>> >
> >>>> > Changes look good. I'm currently running them through our internal
> >>>> build
> >>>> > system. I will sponsor this and push the change through JPRT.
> >>>>
> >>>> Still a problem on Solaris sparc:
> >>>>
> >>>> "/opt/jprt/T/P1/102505.daholme/s/hotspot/src/share/vm/runtime/
> >>>> atomic.inline.hpp",
> >>>> line 96: Error: Could not find a match for static
> >>>> Atomic::cmpxchg(signed
> >>>> char, volatile signed char*, signed char).
> >>>> 1 Error(s) detected.
> >>>>
> >>>> Needs this patch:
> >>>>
> >>>> diff -r 68853ef19be9 src/share/vm/runtime/atomic.inline.hpp
> >>>> --- a/src/share/vm/runtime/atomic.inline.hpp
> >>>> +++ b/src/share/vm/runtime/atomic.inline.hpp
> >>>> @@ -92,7 +92,7 @@
> >>>>
> >>>>   #ifndef VM_HAS_SPECIALIZED_CMPXCHG_BYTE
> >>>>   // See comment in atomic.cpp how to override.
> >>>> -inline jbyte Atomic::cmpxchg(jbyte exchange_value, volatile jbyte
> >>>> *dest, jbyte comparand)
> >>>> +inline jbyte Atomic::cmpxchg(jbyte exchange_value, volatile jbyte
> >>>> *dest, jbyte comparand, cmpxchg_memory_order order)
> >>>>   {
> >>>>     return cmpxchg_general(exchange_value, dest, comparand);
> >>>>   }
> >>>>
> >>>> David
> >>>> -----
> >>>>
> >>>> > Just need another reviewer to chime in - given you and Martin are
> >>>> both
> >>>> > contributors. Or are you the main contributor with Martin being a
> >>>> reviewer?
> >>>> >
> >>>> > Thanks,
> >>>> > David
> >>>> >
> >>>> > PS. It's my night now so I'll be signing off and will pick this up in
> >>>> > the morning.
> >>>> >
> >>>> >> This change follows the discussion started from these mails.
> >>>> >> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
> >>>> April/018960.html
> >>>> >>
> >>>> >> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
> >>>> April/019148.html
> >>>> >>
> >>>> >> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
> >>>> May/019320.html
> >>>> >>
> >>>> >>
> >>>> >> Description:
> >>>> >> This change provides relaxed compare-and-exchange by introducing
> >>>> >> relaxed memory order. As described in
> atomic_linux_ppc.inline.hpp,
> >>>> >> the current implementation of cmpxchg is fence_cmpxchg_acquire.
> >>>> >> This implementation is useful for general purposes because twice
> >>>> calls of
> >>>> >> sync before and after cmpxchg will provide strict consistency.
> >>>> >> However, they sometimes cause overheads because sync
> instructions
> >>>> are
> >>>> >> very expensive in the current POWER chip design.
> >>>> >>
> >>>> >> We confirmed this change improves performance of
> copy_to_survivor
> >>>> >> in the parallel GC. However, we will need more investigation of GC
> >>>> >> by more experts. So, We would like to request a review of the
> change
> >>>> >> of cmpxchg first (as Martin requested).
> >>>> >> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
> >>>> April/019188.html
> >>>> >>
> >>>> >>
> >>>> >> Summary of source code changes:
> >>>> >>
> >>>> >> * src/share/vm/runtime/atomic.hpp
> >>>> >>      - Defines enum memory_order and adds a parameter to
> cmpxchg.
> >>>> >>
> >>>> >> * src/share/vm/runtime/atomic.cpp
> >>>> >> * src/os_cpu/bsd_x86/vm/atomic_bsd_x86.inline.hpp
> >>>> >> * src/os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp
> >>>> >> * src/os_cpu/linux_aarch64/vm/atomic_linux_aarch64.inline.hpp
> >>>> >> * src/os_cpu/linux_sparc/vm/atomic_linux_sparc.inline.hpp
> >>>> >> * src/os_cpu/linux_x86/vm/atomic_linux_x86.inline.hpp
> >>>> >> * src/os_cpu/linux_zero/vm/atomic_linux_zero.inline.hpp
> >>>> >> * src/os_cpu/solaris_sparc/vm/atomic_solaris_sparc.inline.hpp
> >>>> >> * src/os_cpu/solaris_x86/vm/atomic_solaris_x86.inline.hpp
> >>>> >> * src/os_cpu/windows_x86/vm/atomic_windows_x86.inline.hpp
> >>>> >>      - Added a parameter for each cmpxchg function to follow
> >>>> >>         the change of atomic.hpp. Their implementations are not
> >>>> changed.
> >>>> >>
> >>>> >> * src/os_cpu/aix_ppc/vm/atomic_aix_ppc.inline.hpp
> >>>> >> * src/os_cpu/linux_ppc/vm/atomic_linux_ppc.inline.hpp
> >>>> >>      - Added a parameter for each cmpxchg function to follow
> >>>> >>         the change of atomic.hpp. In addition, implementations
> >>>> >>         are changed corresponding to the specified memory_order.
> >>>> >>
> >>>> >> Regards,
> >>>> >> Hiroshi
> >>>> >> -----------------------
> >>>> >> Hiroshi Horii, Ph.D.
> >>>> >> IBM Research - Tokyo
> >>>> >>
> >>>>
> >>>

More information about the hotspot-runtime-dev mailing list