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

["Tulio Magno Quites Machado Filho" <tuliom at linux dot vnet dot ibm dot com>]

"Carlos O'Donell" <carlos@redhat.com> writes:

> On 08/24/2015 02:11 PM, Tulio Magno Quites Machado Filho wrote:
>> Changes since v2:
>>  - Moved part of the source code comments to the commit message.
>>  - Added O'Donnel's suggestion to the source code comments.
>> 
>> Changes since v1:
>>  - Improved commit message.
>>  - Added new comments in the source code alerting to the concurrency details
>>    intrinsic to this code.
>>  - Removed compiler barriers from this patch, which will be treated in another
>>    patch and will be synchronized with the GCC implementation.
>
>> +	  if (__builtin_tbegin (0))					\
>> +	    {								\
>> +	      if (is_lock_free)						\
>> +		{							\
>> +		  ret = 1;						\
>> +		  break;						\
>> +		}							\
>> +	      __builtin_tabort (_ABORT_LOCK_BUSY);			\
>> +	    }								\
>> +	  else								\
>> +	    if (!__get_new_count(&adapt_count))				\
>> +	      break;							\
>
> Toravld still suggests an atomic_load_relaxed here:
> https://www.sourceware.org/ml/libc-alpha/2015-08/msg00893.html
>
> Is there any technical objection to that request?
>
> It does highlight, as he notes, the transactional and non-transactional
> accesses for that evaluated value.

I'm not objecting, but I don't see any value in adding this as I explained at
the end of this message [1].

AFAIU, there are 2 reasons for doing this:
 1. Make the code easier to understand.
 2. Help moving to proper atomic types in the future.

IMHO, reason #1 is very personal and I could change my opinion if I had seen an
example where this kind of changes helped to make the code easier to
understand.

Reason #2 is a valid point, but is unrelated to this patch, i.e. I wouldn't
backport the atomic access to glibc 2.21 and 2.22 if the only reason for it
is #2.  So, it would be better as a separate patch.

[1] https://sourceware.org/ml/libc-alpha/2015-08/msg00885.html

-- 
Tulio Magno