diff options
| author | Peter Zijlstra <peterz@infradead.org> | 2017-03-27 13:54:38 +0200 |
|---|---|---|
| committer | Ingo Molnar <mingo@kernel.org> | 2017-03-30 09:35:54 +0200 |
| commit | 44fe84459faf1a7781595b7c64cd36daf2f2827d (patch) | |
| tree | 1fa467e66d19c23bfea059c11ebb4fee9386de41 /arch | |
| parent | 8ce371f9846ef1e8b3cc8f6865766cb5c1f17e40 (diff) | |
| download | linux-44fe84459faf1a7781595b7c64cd36daf2f2827d.tar.bz2 | |
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>
Diffstat (limited to 'arch')
| -rw-r--r-- | arch/x86/include/asm/cmpxchg.h | 5 |
1 files changed, 3 insertions, 2 deletions
diff --git a/arch/x86/include/asm/cmpxchg.h b/arch/x86/include/asm/cmpxchg.h index fb961db51a2a..d90296d061e8 100644 --- 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) \ |