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

[Torvald Riegel <triegel at redhat dot com>]

On Mon, 2015-08-24 at 16:19 -0300, Tulio Magno Quites Machado Filho
wrote:
> "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.

The synchronization around is_lock_free is mixed txnal/nontxnal
synchronization.  Given that, it's not as simple as purely txnal/txnal
synchronization.
As I mentioned in my reply to Carlos, we do have the general rule that
we don't use atomic types right now but assume that every object that
needs to be atomically accessed at some place is atomically accessed
everywhere (ie, what a C11/C++11 atomic type would provide).  Making an
exception for mixed txnal/nontxnal synchronization is creating more
complexity, just because there is another exception.

> 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.

I wouldn't object if you split this into two parts, and only backport
those patches that are necessary for correctness.