Re: [PATCH v2] PowerPC: Fix a race condition when eliding a lock

["Carlos O'Donell" <carlos at redhat dot com>]

On 08/20/2015 05:52 PM, Tulio Magno Quites Machado Filho wrote:
>> This is looking better, but the comments talk only about the error
>> you are fixing, and not the overall concurrency scheme. The latter
>> is really what we want to document, and from that documentation it
>> should flow naturally that is_lock_free must, out of a requirement,
>> can be read within the transaction safely.
> 
> Just to be clear: when you say "the comments talk", are you referring to the
> commit message or source code comments?

Source code comments.
 
>>> +/* CONCURRENCY NOTES:
>>> +
>>> +   Always evaluate is_lock_free from inside a transaction in macros
>>> +   ELIDE_LOCK and ELIDE_TRYLOCK.
>>> +   Failing to do so, creates a race condition to the memory access specified
>>> +   in is_lock_free which can be triggered with the following order of
>>> +   events:
>>> +   1. The lock accessed by is_lock_free is free.
>>> +   2. Thread T1 evaluates is_lock_free and stores into register R1 that the
>>> +      lock is free.
>>> +   3. Thread T2 acquires the same lock used in is_lock_free.
>>> +   4. T1 begins the transaction, creating a memory barrier where is_lock_free
>>> +      is false, but R1 is true.
>>> +   5. T1 reads R1 and doesn't abort the transaction.
>>> +   6. T1 calls ELIDE_UNLOCK, which reads false from is_lock_free and decides
>>> +      to unlock a lock acquired by T2, leading to undefined behavior.  */
>>
>> This talks about this particular problem, but should instead talk about
>> why is_lock_free need not have any locks or atomic accesses.
>>
>> e.g.
>>
>> /* CONCURRENCY NOTES:
>>
>>    The value of is_lock_free is read and possibly written to by multiple
>>    threads, either during lock acquisition, or elision. In order to avoid
>>    this evaluation from becoming a data race the access of is_lock_free
>>    is placed *inside* the transaction. Within the transaction is_lock_free
>>    can be accessed with relaxed ordering. We are assured by the transaction
>>    that the value of is_lock_free is consistent with the state of the lock,
>>    otherwise the hardware will abort the transaction.
>>
>>    ... */
> 
> Agreed.  The source code becomes much more useful with your comments.  ;-)

Thanks.

>> Feel free to add back the notes on the invalid case if you want, but the
>> above is the minimum I'm expecting. It's a definition of the intent of the
>> current code.
> 
> Maybe the invalid case should only be available in the commit message.

Very good idea. Yes, put them into the commit message.

> Repeating it is a good idea because I was missing it.
> But I still fail to understand how this will make the code clear.
> In fact, I'm afraid it could make it more confusing, e.g. "if I have the
> atomic access, why do I need this transaction?  Let's remove it."
> 
> Maybe, if I had an example I'd better understand what both of you want.

I withdraw my suggestion. As I mentioned to Adhemerval, I can't come up with
a strong reason that isn't already solved by a high quality comment about
concurrency.

Cheers,
Carlos.