Messages in this thread | | | From | Dmitry Vyukov <> | Date | Fri, 24 Mar 2017 15:23:50 +0100 | Subject | Re: locking/atomic: Introduce atomic_try_cmpxchg() |
| |
On Fri, Mar 24, 2017 at 3:21 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Mar 24, 2017 at 01:44:00PM +0100, Dmitry Vyukov wrote: >> >> The primitive has subtle difference with all other implementation that >> I know of, and can lead to very subtle bugs. Some time ago I've spent >> several days debugging a memory corruption caused by similar >> implementation. 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 here: > > Indeed. I had only considered stack local variables when I wrote that. > >> So I would suggest to change it to a safer and less surprising >> alternative: >> >> diff --git a/arch/x86/include/asm/cmpxchg.h b/arch/x86/include/asm/cmpxchg.h >> index fb961db51a2a..81fb985f51f4 100644 >> --- a/arch/x86/include/asm/cmpxchg.h >> +++ b/arch/x86/include/asm/cmpxchg.h >> @@ -212,7 +212,8 @@ extern void __add_wrong_size(void) >> default: \ >> __cmpxchg_wrong_size(); \ >> } \ >> - *_old = __old; \ >> + if (!success) \ >> + *_old = __old; \ >> success; \ >> }) > > I've no immediate objection, I'll double check what, if anything, it > does for code gen. > >> diff --git a/include/linux/atomic.h b/include/linux/atomic.h >> index aae5953817d6..f8098157f7c8 100644 >> --- a/include/linux/atomic.h >> +++ b/include/linux/atomic.h >> @@ -1023,8 +1023,11 @@ static inline int atomic_dec_if_positive(atomic_t *v) >> ({ \ >> typeof(_po) __po = (_po); \ >> typeof(*(_po)) __o = *__po; \ >> - *__po = atomic64_cmpxchg##type((_p), __o, (_n)); \ >> - (*__po == __o); \ >> + typeof(*(_po)) __v = atomic64_cmpxchg##type((_p), __o, (_n)); \ >> + if (__v == __o) \ >> + return true; \ >> + *__po = __v; \ >> + return false; \ >> }) > > Can you actually use return in statement-expressions?
Dunno. Just wanted to show the idea.
| |