On 2018/10/24 23:53:29 +0800, Junchang Wang wrote:
> Hi Akira and Paul,
>
> I checked the code today and the implementation of cmpxchg() looks
> very suspicious to me; Current cmpxchg() first executes function
> __atomic_compare_exchange_n, and then checks whether the value stored
> in field __actual (old) has been changed to decide if the CAS
> instruction has been successfully performed. However, I saw the *weak*
> field is set, which, as far as I know, means
> __atomic_compare_exchange_n could fail even if the value of *ptr is
> equal to __actual (old). Unfortunately, current cmpxchg will treat
> this case as a success because the value of __actual(old) does not
> change.
Thanks for looking into this!
I also suspected the use of "weak" semantics of
__atomic_compare_exchange_n(), but did not quite understand what
"spurious fail" actually means. Your theory sounds plausible to me.
I've suggested in a private email to Paul to modify the 4th argument
to false(0) as a workaround, which would prevent such "spurious fail".
Both approaches looks good to me. I'd defer to Paul on the choice.
Thanks, Akira
>
> This bug happens in both Power8 and ARMv8. It seems it affects
> architectures that use LL/SC to emulate CAS. Following patch helps
> solve this issue on my testbeds. Please take a look. Any thoughts?
>
> ---
> CodeSamples/api-pthreads/api-gcc.h | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/CodeSamples/api-pthreads/api-gcc.h
> b/CodeSamples/api-pthreads/api-gcc.h
> index 1dd26ca..38a16c0 100644
> --- a/CodeSamples/api-pthreads/api-gcc.h
> +++ b/CodeSamples/api-pthreads/api-gcc.h
> @@ -166,11 +166,9 @@ struct __xchg_dummy {
>
> #define cmpxchg(ptr, o, n) \
> ({ \
> - typeof(*ptr) _____actual = (o); \
> - \
> - (void)__atomic_compare_exchange_n(ptr, (void *)&_____actual, (n), 1, \
> - __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \
> - _____actual; \
> + typeof(*ptr) old = (o); \
> + (__atomic_compare_exchange_n(ptr, (void *)&old, (n), 1,
> __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST))? \
> + (o) : (n); \
> })
>
> static __inline__ int atomic_cmpxchg(atomic_t *v, int old, int new)
>
