locking/atomic: Fix atomic_try_cmpxchg() semantics

Dmitry noted that the new atomic_try_cmpxchg() primitive is broken when
the old pointer doesn't point to the local stack.

He writes:

  "Consider a classical lock-free stack push:

    node->next = atomic_read(&head);
    do {
    } while (!atomic_try_cmpxchg(&head, &node->next, node));

  This code is broken with the current implementation, the problem is
  with unconditional update of *__po.

  In case of success it writes the same value back into *__po, but in
  case of cmpxchg success we might have lose ownership of some memory
  locations and potentially over what __po has pointed to. The same
  holds for the re-read of *__po. "

He also points out that this makes it surprisingly different from the
similar C/C++ atomic operation.

After investigating the code-gen differences caused by this patch; and
a number of alternatives (Linus dislikes this interface lots), we
arrived at these results (size x86_64-defconfig/vmlinux):

  GCC-6.3.0:

  10735757        cmpxchg
  10726413        try_cmpxchg
  10730509        try_cmpxchg + patch
  10730445        try_cmpxchg-linus

  GCC-7 (20170327):

  10709514        cmpxchg
  10704266        try_cmpxchg
  10704266        try_cmpxchg + patch
  10704394        try_cmpxchg-linus

From this we see that the patch has the advantage of better code-gen
on GCC-7 and keeps the interface roughly consistent with the C
language variant.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
Fixes: a9ebf306f52c ("locking/atomic: Introduce atomic_try_cmpxchg()")
Signed-off-by: Ingo Molnar <mingo@kernel.org>

diff --git a/arch/x86/include/asm/cmpxchg.h b/arch/x86/include/asm/cmpxchg.h
index fb961db51a2a29f4398e34e906509a558c55a995..d90296d061e8267a97cd8a58328d8a20534289d4 100644 (file)
--- a/arch/x86/include/asm/cmpxchg.h
+++ b/arch/x86/include/asm/cmpxchg.h
@@ -212,8 +212,9 @@ extern void __add_wrong_size(void)
        default:                                                        \
                __cmpxchg_wrong_size();                                 \
        }                                                               \
-       *_old = __old;                                                  \
-       success;                                                        \
+       if (unlikely(!success))                                         \
+               *_old = __old;                                          \
+       likely(success);                                                \
 })
 
 #define __try_cmpxchg(ptr, pold, new, size)                            \

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index aae5953817d633aed12990ae7ad202b2060ae177..c56be74101305f74524bc816d9c8fdb67ccb895d 100644 (file)
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -428,9 +428,11 @@
 #define __atomic_try_cmpxchg(type, _p, _po, _n)                                \
 ({                                                                     \
        typeof(_po) __po = (_po);                                       \
-       typeof(*(_po)) __o = *__po;                                     \
-       *__po = atomic_cmpxchg##type((_p), __o, (_n));                  \
-       (*__po == __o);                                                 \
+       typeof(*(_po)) __r, __o = *__po;                                \
+       __r = atomic_cmpxchg##type((_p), __o, (_n));                    \
+       if (unlikely(__r != __o))                                       \
+               *__po = __r;                                            \
+       likely(__r == __o);                                             \
 })
 
 #define atomic_try_cmpxchg(_p, _po, _n)                __atomic_try_cmpxchg(, _p, _po, _n)
@@ -1022,9 +1024,11 @@ static inline int atomic_dec_if_positive(atomic_t *v)
 #define __atomic64_try_cmpxchg(type, _p, _po, _n)                      \
 ({                                                                     \
        typeof(_po) __po = (_po);                                       \
-       typeof(*(_po)) __o = *__po;                                     \
-       *__po = atomic64_cmpxchg##type((_p), __o, (_n));                \
-       (*__po == __o);                                                 \
+       typeof(*(_po)) __r, __o = *__po;                                \
+       __r = atomic64_cmpxchg##type((_p), __o, (_n));                  \
+       if (unlikely(__r != __o))                                       \
+               *__po = __r;                                            \
+       likely(__r == __o);                                             \
 })
 
 #define atomic64_try_cmpxchg(_p, _po, _n)              __atomic64_try_cmpxchg(, _p, _po, _n)