Re: [PATCH] x86: Optimize atomic_compare_and_exchange_[val|bool]_acq [BZ #28537]

[Oleh Derevenko <oleh.derevenko@gmail.com>]

Hi, H.J. Lu

You may not perform plain reads on values you want to be atomic. This
results in undefined behavior.
For example, the compiler IS NOT obliged to perform the read with a
single CPU instruction -- of course it will not, but it is allowed to
read it in two halves and compare them separately. Or it may reuse
cached value from previous evaluations.
This is only the compiler level issue. Similar issues will arise at
CPU level with all the kind of memory coherency, caching and
instruction reordering.
Or if the value would cross a cache line boundary the plain read might
return half-updated value with the part from one cache line being new
and the other part being old.
Finally, I'm not following this thread and I have very little
knowledge on function purposes in the library but the "_acq" suffix
bears the impression that the memory operation has to exhibit the
acquire semantics. I.e., it has to fetch memory updates and make them
visible for the thread. But this is what you are trying to eliminate
with your patch.

On Wed, Nov 3, 2021 at 5:05 PM H.J. Lu via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> From the CPU's point of view, getting a cache line for writing is more
> expensive than reading.  See Appendix A.2 Spinlock in:
>
> https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/xeon-lock-scaling-analysis-paper.pdf
>
> The full compare and swap will grab the cache line exclusive and cause
> excessive cache line bouncing.  Check the current memory value first and
> return immediately if writing cache line may fail to reduce cache line
> bouncing on contended locks.
>
> This fixes BZ# 28537.
> ---
>  sysdeps/x86/atomic-machine.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/sysdeps/x86/atomic-machine.h b/sysdeps/x86/atomic-machine.h
> index 2692d94a92..92c7cf58b7 100644
> --- a/sysdeps/x86/atomic-machine.h
> +++ b/sysdeps/x86/atomic-machine.h
> @@ -73,9 +73,19 @@ typedef uintmax_t uatomic_max_t;
>  #define ATOMIC_EXCHANGE_USES_CAS       0
>
>  #define atomic_compare_and_exchange_val_acq(mem, newval, oldval) \
> -  __sync_val_compare_and_swap (mem, oldval, newval)
> +  ({ __typeof (*(mem)) oldmem = *(mem), ret;                           \
> +     ret = (oldmem == (oldval)                                         \
> +           ? __sync_val_compare_and_swap (mem, oldval, newval)         \
> +           : oldmem);                                                  \
> +     ret; })
>  #define atomic_compare_and_exchange_bool_acq(mem, newval, oldval) \
> -  (! __sync_bool_compare_and_swap (mem, oldval, newval))
> +  ({ __typeof (*(mem)) old = *(mem);                                   \
> +     int ret;                                                          \
> +     if (old != (oldval))                                              \
> +       ret = 1;                                                                \
> +     else                                                              \
> +       ret = !__sync_bool_compare_and_swap (mem, oldval, newval);      \
> +     ret; })
>
>
>  #define __arch_c_compare_and_exchange_val_8_acq(mem, newval, oldval) \
> --
> 2.33.1
>


-- 

Oleh Derevenko

-- Skype with underscore