MIME-Version: 1.0
In-Reply-To: <20170324142140.vpyzl755oj6rb5qv@hirez.programming.kicks-ass.net>
References: <CACT4Y+bG+a0w6j6v1AmBE7fqqMSPyPEm4QimCzCouicmHT8FqA@mail.gmail.com>
 <20170324142140.vpyzl755oj6rb5qv@hirez.programming.kicks-ass.net>
From: Dmitry Vyukov <dvyukov@google.com>
Date: Fri, 24 Mar 2017 15:23:50 +0100
Message-ID: <CACT4Y+aBcbKBQMMc+cwZdMNdnO+-tfQ+sTVEFeTVig+3c8ngCw@mail.gmail.com>
Subject: Re: locking/atomic: Introduce atomic_try_cmpxchg()
To: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
        Brian Gerst <brgerst@gmail.com>, Denys Vlasenko <dvlasenk@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Josh Poimboeuf <jpoimboe@redhat.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Paul McKenney <paulmck@linux.vnet.ibm.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2833
Lines: 57

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.