RE: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.

[Lei Xu <lei dot xu at nxp dot com>]

Hello, Torvald

Thank you for your review and suggestions.
Add some comments below~

-----Original Message-----
From: Torvald Riegel [mailto:triegel@redhat.com] 
Sent: Monday, June 13, 2016 6:37 PM
To: Lei Xu
Cc: carlos@systemhalted.org; sjmunroe@us.ibm.com; libc-alpha@sourceware.org; Scott Wood
Subject: Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.

On Sun, 2016-06-12 at 17:30 +0800, Lei Xu wrote:
> In current code, atomic_compare_and_exchange_*_rel is defined as 
> atomic_compare_and_exchange_*_acq, however this is wrong on power 
> arch, especially __arch_*_rel has been defined on power arch.
> This has caused segmentation fault issue when doing 
> pthread_mutex_lock/unlock operations on PowerPC E6500.

I agree that on some archs, acquire and release memory order are not the same.
However, ./sysdeps/powerpc/atomic-machine.h does define atomic_compare_and_exchange_val_rel.  Is atomic_compare_and_exchange_bool_rel what is missing in the powerpc definitions?
[XuLei] Yes, you are right. The atomic_compare_and_exchange_bool_rel is missed in powerpc definitions.

> Considering generic code should never be assuming that an acquire 
> barrier can be used in place of a release barrier(If it's OK for a 
> particular arch let the arch define them as being the same), fix this issue as follows:

The generic file include/atomic.h does define *fallbacks* for atomic operations, assuming that arch-specific files will provide other definitions if the default fallbacks are not correct on the respective arch.  This scheme may not be better than not defining fallbacks that are not correct on all archs, but it is the one we have right now.

Therefore, I think the easiest way is to fix the powerpc side of this.
Note that the atomic operations you are changing here will be phased out eventually, because we're moving to the newer C11-like atomic operations (see the last part of include/atomic.h).

Your patch introduces a powerpc-specific change into the generic include/atomic.h (I don't think all archs define __arch_compare_and_exchange_val_32_rel, for example), which is not OK.
If you would like to change how include/atomic.h selects defaults, the patch should change this for all operations, and in a way that is correct for all archs we have and that does not decrease performance for any of the archs.

[XuLei] Yes, I agree with you. At first I plan to fix this to add atomic_compare_and_exchange_bool_rel in powerpc definitions.
Next I considered whether the default definitions in include/atomic.h should be changed:
The atomic_compare_and_exchange_val_acq was defined according to __arch_c_compare_and_exchange_val_32_acq,
Why not atomic_compare_and_exchange_val_rel? If there is no __arch_compare_and_exchange_val_32_rel, it can also use atomic_compare_and_exchange_val_acq.

Meantime we can define atomic_compare_and_exchange_bool_rel to use __arch_compare_and_exchange_bool_32_rel firstly, then atomic_compare_and_exchange_val_rel;
This is similarly with atomic_compare_and_exchange_bool_acq definitions.

These can be applied to generic but not powerpc specific and follows the same rules:
For acq, Firstly try to use __arch_*acq, then use atomic_*val_*acq;
For rel, firstly to use __arch_*rel, then use atomic_*val_*rel, then atomic_*val*acq;

But I did not test this on all of the archs, and just compile it successfully on x86 platform.

So the key point may be that whether it need to change the default generic files, if NOT, I will modify the files in powerpc definitions.
Thanks.
-------------------------[xulei]

A third option to improve this would be to remove atomic_compare_and_exchange_bool_rel and replace it with atomic_compare_exchange_weak_release (ie, the matching C11 atomic operation).  There are just three callsites that would have to change.