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

[Torvald Riegel <triegel at redhat dot com>]

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?

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

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.