Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

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>

authored by

Peter Zijlstra and committed by
Ingo Molnar
44fe8445 8ce371f9

+13 -8
+3 -2
arch/x86/include/asm/cmpxchg.h
··· 212 212 default: \ 213 213 __cmpxchg_wrong_size(); \ 214 214 } \ 215 - *_old = __old; \ 216 - success; \ 215 + if (unlikely(!success)) \ 216 + *_old = __old; \ 217 + likely(success); \ 217 218 }) 218 219 219 220 #define __try_cmpxchg(ptr, pold, new, size) \
+10 -6
include/linux/atomic.h
··· 428 428 #define __atomic_try_cmpxchg(type, _p, _po, _n) \ 429 429 ({ \ 430 430 typeof(_po) __po = (_po); \ 431 - typeof(*(_po)) __o = *__po; \ 432 - *__po = atomic_cmpxchg##type((_p), __o, (_n)); \ 433 - (*__po == __o); \ 431 + typeof(*(_po)) __r, __o = *__po; \ 432 + __r = atomic_cmpxchg##type((_p), __o, (_n)); \ 433 + if (unlikely(__r != __o)) \ 434 + *__po = __r; \ 435 + likely(__r == __o); \ 434 436 }) 435 437 436 438 #define atomic_try_cmpxchg(_p, _po, _n) __atomic_try_cmpxchg(, _p, _po, _n) ··· 1024 1022 #define __atomic64_try_cmpxchg(type, _p, _po, _n) \ 1025 1023 ({ \ 1026 1024 typeof(_po) __po = (_po); \ 1027 - typeof(*(_po)) __o = *__po; \ 1028 - *__po = atomic64_cmpxchg##type((_p), __o, (_n)); \ 1029 - (*__po == __o); \ 1025 + typeof(*(_po)) __r, __o = *__po; \ 1026 + __r = atomic64_cmpxchg##type((_p), __o, (_n)); \ 1027 + if (unlikely(__r != __o)) \ 1028 + *__po = __r; \ 1029 + likely(__r == __o); \ 1030 1030 }) 1031 1031 1032 1032 #define atomic64_try_cmpxchg(_p, _po, _n) __atomic64_try_cmpxchg(, _p, _po, _n)