On Fri, Mar 24, 2017 at 3:21 PM, Peter Zijlstra <pet...@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.
