RFR(xs): 8211387: [Zero] atomic_copy64: Use ldrexd for atomic reads on ARMv7

[David Holmes]

Hi Severin,

On 9/10/2018 2:20 AM, Severin Gehwolf wrote:
> Hi!
> 
> On Sun, 2018-10-07 at 07:52 +1000, David Holmes wrote:
>> On 7/10/2018 6:11 AM, dean.long at oracle.com wrote:
>>> On 10/5/18 5:18 PM, David Holmes wrote:
>>>> On 6/10/2018 12:18 AM, Andrew Haley wrote:
>>>>> On 10/05/2018 02:40 PM, Severin Gehwolf wrote:
>>>>>>> Also I thought that if ldrex was not paired with a strex then you
>>>>>>> should
>>>>>>> at least issue clrex - as done in generate_atomic_load_long() in
>>>>>>> stubGenerator_arm.cpp
>>>>>>
>>>>>> I'll defer to Andrew Haley for this.
>>>>>
>>>>> I'm skeptical that CLREX is necessary. The classic CAS for Arm is
>>>>>
>>>>>       MOV  R1, #0xFF             ; load the ?lock taken? value
>>>>> try LDREX    R0, [LockAddr]    ; load the lock value
>>>>>       CMP R0,  #0                ; is the lock free?
>>>>>       STREXEQ  R1, R0, [LockAddr]; try and claim the lock
>>>>>       CMPEQ R0, #0               ; did this succeed?
>>>>>       BNE try                    ; no ? try again. . .
>>>>> ; yes ? we have the lock
>>>>>
>>>>> There's no CLREX, and no-one ever reported any problem. CLREX could be
>>>>> a performance boost because it takes the cache line out of exclusive
>>>>> state back to shared state, reducing later bus traffic.
>>>>
>>>> I would see CLREX as a potential performance boost not something
>>>> needed for correctness. But you're right that failure paths from
>>>> typical CAS code won't execute the strex and don't include a clrex.
>>>>
>>>
>>> I think it's needed for correctness only at context-switch time.  If you
>>> context switch after the LDREX, you don't want the exclusive state to
>>> remain for the next thread scheduled.
>>
>> See also:
>>
>> https://community.arm.com/processors/f/discussions/6572/synchronization-primitives-do-i-need-clrex
>>
>> Seems if CLREX is needed for correctness it is at the OS level.
>>
>> On performance concern see:
>>
>> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150921/300951.html
>>
>> Bottom line seems to be:
>> - clrex is not essential
>> - clrex is probably not harmful to performance
>> - clrex may benefit performance
> 
> OK. David, how would you like me to update the webrev then?

Based on other responses your original is probably fine, but as you've 
already gone to the trouble ...

> 2a)
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8211387/webrev.02_a/

this one is fine.

Thanks,
David

> 2b)
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8211387/webrev.02_b/
> 
> FWIW, the performance gain for Zero is probably negligible, but it
> probably won't hurt either...
> 
> Thanks,
> Severin
>