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

Wed May 11 22:50:21 UTC 2016

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
>>>> >>
>>>>
>>>