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

[David Holmes]

Hi Adrian,

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.

Without seeing how that wrapper actually works, it is hard to know 
whether what you are saying and your fix is correct. The description of 
the CAS at [1] doesn't help without knowing how it actually gets used.

David

> 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/
>