This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH v2] PowerPC: Fix a race condition when eliding a lock
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Tulio Magno Quites Machado Filho <tuliom at linux dot vnet dot ibm dot com>, triegel at redhat dot com
- Cc: adhemerval dot zanella at linaro dot org, libc-alpha at sourceware dot org, munroesj at linux dot vnet dot ibm dot com
- Date: Fri, 21 Aug 2015 11:25:07 -0400
- Subject: Re: [PATCH v2] PowerPC: Fix a race condition when eliding a lock
- Authentication-results: sourceware.org; auth=none
- References: <55BAEE77 dot 9000501 at redhat dot com> <1440084054-32243-1-git-send-email-tuliom at linux dot vnet dot ibm dot com> <55D628C2 dot 3080305 at redhat dot com> <87lhd5typ9 dot fsf at totoro dot lan>
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.
>> /* 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. ;-)
>> 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