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

[David Holmes]

Hi Adrian,

Not really a review but I was curious about this ...

On 5/09/2017 8:00 PM, John Paul Adrian Glaubitz wrote:
> Hi!
> 
> Please review the changeset in [1] which fixes the incorrect use of 
> __m68k_cmpxchg().
> 
> The description for this change is [2]:
> 
> =============================================================
> 
> 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 am surprised this even works at all. So trying to follow the logic if 
initially "prev == oldval" then the cas actually succeeds, but the loop 
logic thinks it failed and so retries. It re-reads the current value 
into prev, which no longer equals oldval, so the loop terminates and it 
returns "prev" which may actually be the value that was updated by the 
successful cas; or it could be a different value if some other thread 
has since also performed a successful cas. So this function would always 
report failure, even on success (except in ABA situation)! I can't see 
how anything would work in that case ??

BTW m68k_compare_and_swap does not need to have a loop at all, it only 
has to do the cas and return the correct value. A loop would only be 
needed if the low-level cas can fail spuriously - which does not seem to 
be the case.

Cheers,
David

> 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.
> 
>> [1] http://68k.hax.com/CAS
> 
> =============================================================
> 
>> [1] http://cr.openjdk.java.net/~glaubitz/8187227/webrev.01/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8187227
>