RFR(M): 8155949: Support relaxed semantics in cmpxchg
David Holmes
david.holmes at oracle.com
Wed May 11 04:41:06 UTC 2016
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