RFR: 8187227: __m68k_cmpxchg() is not being used correctly

[David Holmes]

Previous RFR:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028266.html

David

On 18/11/2017 10:36 AM, John Paul Adrian Glaubitz wrote:
> Hi!
> 
> On m68k, linux-zero has platform-specific implementations for compare_and_swap(),
> add_and_fetch() and lock_test_and_set(). These functions are all using __m68k_cmpxchg()
> which is basically a wrapper around the m68k assembly instruction CAS.
> 
> Currently, all three functions make incorrect assumptions about how CAS and its
> wrapper actually work and consequently use __m68k_cmpxchg() incorrectly. The source
> code comment for __m68_cmpxchg() states:
> 
>    * Atomically store newval in *ptr if *ptr is equal to oldval for user space.
>    * Returns newval on success and oldval if no exchange happened.
>    * This implementation is processor specific and works on
>    * 68020 68030 68040 and 68060.
> 
> However, looking at the documentation for the CAS instruction on m68k [1] and the
> implementation of __m68k_cmpxchg(), this is actually not how the function works.
> It does not return the update value on a successful exchange but rather the contents
> the compare operand, i.e. oldval. If no exchange happened, it will actually return
> the contents of the memory location. newval is never returned and consequently
> testing for "newval" in compare_and_swap(), add_and_fetch() and lock_test_and_set()
> is a bug.
> 
> I have preapred a patch that fixes this issue by making correct use of __m68k_cmpxchg()
> in compare_and_swap(), add_and_fetch() and lock_test_and_set(). This patch has been
> tested to work on Debian m68k.
> 
> Additional notes:
> 
> In a previous of RFR of this bug - which I cannot find anymore - David Holmes was
> questioning whether the use of the loops within the atomic helper functions was
> correct. After thinking about the problem again, the answer is very simple: The
> CAS operation, while guaranteed to perform the swap atomically, is actually not
> guaranteed to succeed on first attempt trying to swap a memory operand with a
> direct operand. Hence, we have to keep trying to perform the compare-and-swap
> operation until it succeeds. That's what the loops are foor.
> 
> Updated webrev can be found in [2].
> 
> Thanks,
> Adrian
> 
>> [1] http://68k.hax.com/CAS
>> [2] http://cr.openjdk.java.net/~glaubitz/8187227/webrev.02/
>