RFR 8201799: Build failures after JDK-8195099 (Concurrent safe-memory-reclamation mechanism)

[Doerr, Martin]

Hi Erik,

thank you for bringing this issue up again.

In my opinion, good multi-threaded code specifies ordering semantics precisely for each lock-free access. I think this was done well enough here by adding a comment.

It'd be great if we could specify semantics for Atomic:add like we do for Atomic::cmpchgx.
However, the double-fence is a very bad default from performance perspective. I wonder if PPC64 is the only platform which gets hit.
Would it be acceptable to set the default to memory_order_acquire_release and specify memory_order_conservative for the new usage? I think this would fit perfectly for PPC64, but some people may not like it. One could say PPC64 is the problem, but one could also say the VM code is the problem ��

I don't really like the straight forward fix to insert a fence with #ifdef AIX.

Best regards,
Martin


-----Original Message-----
From: Erik Österlund [mailto:erik.osterlund at oracle.com] 
Sent: Mittwoch, 18. April 2018 14:47
To: David Holmes <david.holmes at oracle.com>; Aleksey Shipilev <shade at redhat.com>; hotspot-dev at openjdk.java.net; Doerr, Martin <martin.doerr at sap.com>
Subject: Re: RFR 8201799: Build failures after JDK-8195099 (Concurrent safe-memory-reclamation mechanism)

Hi,

Please also note that the Atomic::add used in write_synchronize assumes 
bidirectional fencing (which is the contract provided by Atomic::add). 
This is however not true for the PPC backend of Atomic::add - it has 
acq_rel semantics instead. So this mechanism is currently not safe to 
use on PPC.

 From globalCounter.cpp:
   61   // Atomic::add must provide fence since we have storeload 
dependency.
   62   volatile uintx gbl_cnt = Atomic::add((uintx)COUNTER_INCREMENT, 
&_global_counter._counter);

Might want to have a look at the safe use of this mechanism on PPC as a 
follow-up.

Thanks,
/Erik

On 2018-04-18 14:27, David Holmes wrote:
> Hi Aleksey,
>
> On 18/04/2018 9:50 PM, Aleksey Shipilev wrote:
>> Bug:
>>    https://bugs.openjdk.java.net/browse/JDK-8201799
>>
>> Fix:
>>
>> diff -r bfba4712d4ff 
>> src/hotspot/share/utilities/globalCounter.inline.hpp
>> --- a/src/hotspot/share/utilities/globalCounter.inline.hpp Wed Apr 18 
>> 11:36:48 2018 +0200
>> +++ b/src/hotspot/share/utilities/globalCounter.inline.hpp Wed Apr 18 
>> 13:50:34 2018 +0200
>> @@ -27,6 +27,7 @@
>>
>>   #include "runtime/orderAccess.inline.hpp"
>>   #include "runtime/thread.hpp"
>
> The above line can be deleted now.
>
>> +#include "runtime/thread.inline.hpp"
>
> Fix is good.
>
> Thanks,
> David
>
>>   #include "utilities/globalCounter.hpp"
>>
>>   inline void GlobalCounter::critical_section_begin(Thread *thread) {
>>
>> Testing: failing aarch64 build
>>
>> Thanks,
>> -Aleksey
>>