[PATCH][BZ 18743] PowerPC: Fix a race condition when eliding a lock

[Adhemerval Zanella]

On 31-07-2015 18:44, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> On 31-07-2015 11:57, Tulio Magno Quites Machado Filho wrote:
>>> Let's say t1 is the first one to read rwlock->__data fields and they're all 0.
>>> t1 will set is_lock_free to 0.
>>
>> I believe you mean is_lock_free will be set to '1' if all fields are '0' (since
>> the lock will not be held).
> 
> Yes!  I'm sorry.
> 
>> I see now a window where the pthread_rwlock_t internal structures might have 
>> concurrent access and the transaction is never finished. However I do not 
>> think this is a powerpc specific and thus I think it should be fixed in generic
>> algorithm instead. Something like this:
>>
>> ...
>>
>> I did not fix the x86_64 code, which would require some adjustments.  I checked
>> on ppc64le and make nptl/tests passes.
> 
> I didn't test your patch, but after reading it, I'm sure it does fix the
> problem too.
> 
>> It would be good if you could create a
>> testcase to stress this issue (and I do see this could be quite hard since it is
>> very timing dependent).
> 
> Yes.
> I tried, but I didn't like the idea of creating a testcase that would only
> provide intermittent failures.  Especially because it's very difficult to
> reproduce this failure.
> Any suggestions?
> 
>> The problem with this approach is it couple the elide macros with rdlock
>> fields, and I think the idea would make this header more generic for multiple
>> elide algorithms.  Suggestions?
> 
> And that's why I preferred to modify only the powerpc implementation.
> Both proposals are identical, in the sense that they move the memory access
> to the transaction.
> 
> I'd appreciate if you could explain why you prefer to change the argument list
> of ELIDE_LOCK instead of the powerpc specific code.
> 

Mainly because I see this issue is not powerpc specific and I think we should
also fix for x86 (that currently also uses the same mechanism) and for future
arches that also might potentially implement lock elision using this.