RFR: JDK-8061964: Insufficient compiler barriers for GCC in OrderAccess functions

Tue Nov 4 13:06:31 UTC 2014

Hi Mikael,

On 04/11/2014 13:35, Mikael Gerdin wrote:
> Hi David,
>
> On 2014-11-04 12:15, David Holmes wrote:
>> Hi Mikael,
>>
>> Thanks for fixing this.
>>
>> Given x86 is a TSO system my understanding from the previous discussion
>> in 6973570 is that once you have the compiler barrier the volatile write
>> to the dummy variable in storestore() is no longer needed - it's only
>> purpose was to introduce the compiler barrier (which it failed to do).
>
> I felt that removing the dummy store would be more risky since I have no
> good way of verifying that removing the volatile write doesn't break
> ordering for some other caller of storestore().

I agree with David that the dummy volatile store is no longer needed.

It should IMHO be removed to clean the code and avoid confusing future 
readers.

>> Secondly, do we observe the same bug with the release_store operations
>> or are we just being conservative? I would not expect to need the
>> compiler barrier prior to the Atomic::store, but I would assume this is
>> completely harmless and performance neutral, so we should have it for
>> completeness regardless.
>
> I tried changing the problematic code to do a release_store of
> _gc_time_stamp and saw the same reordering problem, that's the reason
> for adding compiler_barrier to the release_store variants.

I think David was just referring to the jlong versions, which use the 
Atomic class.

However, since the compiler_barrier() is performance neutral when 
redundant, it is IMHO better to keep it (avoiding to create a dependency 
towards how Atomic::store is specificed and implemented for jlong).

> Any suggestion on where to put compiler_barrier in order to use it from
> atomic_linux_x86.inline.hpp?
>
>>
>> Finally, you answered Dean regarding the simple acquire() function but
>> the load_acquire variants all rely on volatile semantics - which we've
>> seen not to provide what we expected. So perhaps the load_acquire
>> functions also need a compile_barrier inserted ?
>
> Perhaps. I haven't observed any problems with the acquire functions so I
> can't determine if they need the compiler barrier or not.
>
> It may be a good idea to add the compiler barrier to those functions as
> well, what are the other reviewers' opinions on this?

If the barrier is useless (e.g. the volatile load already provides the 
compiler barrier) then it is performance neutral. Hence, IMHO, it is 
better to play it safe and add the compiler_barrier in load_acquire 
(after the loads).

Regards,

Bertrand

>
> Thanks for the review David.
> /Mikael
>
>
>>
>> Thanks,
>> David
>>
>> On 3/11/2014 11:01 PM, Mikael Gerdin wrote:
>>> Hi all,
>>>
>>> Please review this attempt at fixing the OrderAccess functions on Linux
>>> x86 with GCC.
>>>
>>> While working on another bug I recently discovered that g++ was
>>> reordering stores across a call to OrderAccess::storestore on Linux x86.
>>>
>>> The G1 code attempts to do an ordered publishing of two values:
>>> _saved_mark_word = _top;
>>> OrderAccess::storestore();
>>> _gc_time_stamp = curr_gc_time_stamp;
>>>
>>> The types involved are
>>> HeapWord* _top, _saved_mark_word;
>>> volatile unsigned _gc_time_stamp;
>>>
>>> The incorrect behavior seems to have started when JDK-6973570 was fixed
>>> in JDK 7.
>>> Below, _top is at offset 0x58, _saved_mark_word at 0x18 and
>>> _gc_time_stamp at 0x138, %rbx is "this".
>>>
>>> /net/jre/onestop/jdk/1.7.0/promoted/all//b108/binaries/linux-x64/jre/lib/amd64/server/libjvm.so:
>>>
>>>
>>>
>>>    3d9f4d:       39 d0                   cmp    %edx,%eax
>>>    3d9f4f:       73 1c                   jae    3d9f6d
>>> <G1OffsetTableContigSpace::set_saved_mark()+0x3d>
>>>    3d9f51:       48 8b 43 58             mov    0x58(%rbx),%rax
>>>    3d9f55:       48 89 43 18             mov    %rax,0x18(%rbx)
>>>    3d9f59:       48 8b 05 40 f9 70 00    mov    0x70f940(%rip),%rax    #
>>> ae98a0 <_DYNAMIC+0x12f8>
>>>    3d9f60:       48 c7 00 00 00 00 00    movq   $0x0,(%rax)
>>>    3d9f67:       89 93 38 01 00 00       mov    %edx,0x138(%rbx)
>>>
>>> /net/jre/onestop/jdk/1.7.0/promoted/all//b109/binaries/linux-x64/jre/lib/amd64/server/libjvm.so
>>>
>>>
>>>
>>>    3da05d:       39 d0                   cmp    %edx,%eax
>>>    3da05f:       73 15                   jae    3da076
>>> <G1OffsetTableContigSpace::set_saved_mark()+0x36>
>>>    3da061:       48 8b 43 58             mov    0x58(%rbx),%rax
>>>    3da065:       c7 45 f4 00 00 00 00    movl   $0x0,-0xc(%rbp)
>>>    3da06c:       89 93 38 01 00 00       mov    %edx,0x138(%rbx)
>>>    3da072:       48 89 43 18             mov    %rax,0x18(%rbx)
>>>
>>> In b109 the store of %rax to 0x18(%rbx) has been ordered after the store
>>> of %edx to 0x138(%rbx) in the same build as JDK-6973570 was integrated.
>>>
>>> My suggestion to fix this is to extend all the OrderAccess::release*
>>> variants on x86 with a:
>>> __asm__ volatile ("" : : : "memory");
>>> to attempt to prevent GCC from reordering any memory accesses across
>>> those function calls.
>>>
>>> I've verified that this solves the issue in the assembly with our
>>> current JDK 9 build platform compilers.
>>> I've also verified that this particular piece of code is compiled
>>> correctly on our other x86 platforms: Solaris, Windows and OS X.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~mgerdin/8061964/webrev.0/
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8061964
>>> Testing:
>>> JPRT, inspecting generated assembly for the function
>>> G1OffsetTableContigSpace::record_top_and_timestamp (as the method is
>>> currently named).
>>> Suggestions of further testing is greatly appreciated.
>>>
>>> Thanks
>>> Mikael

-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38334 Saint Ismier,                                        FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~