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:28 -0300, Adhemerval Zanella wrote:
> 
> On 24-08-2015 15:58, Carlos O'Donell wrote:
> > 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.
> 
> No 'technical' objections, only that the transaction idea is exactly to
> avoid the use of atomic accesses altogether.  For relaxed accesses it
> won't change anything,

Yes.

> but if the idea is to highlight every access
> within transaction with atomics,

No.  The rule I'd like to follow is that if we need atomic accesses to a
memory object somewhere in the code base, that we then consistently use
atomic accesses for all accesses to this object (with the exception of
initialization code, where things like memset can be simpler).

Thus, if you use just transactions to prevent data races for an object,
no atomics are required.  This holds for the vast majority of objects
accessed in transactions (except lock implementations and such).

IMO, not using atomics for accessing is_lock_free in the txn creates an
exception in our rules, and given that there's no problem to used an
MO-relaxed access there, it's just easier to avoid creating an
exception.  And it makes it possible to use an atomic type for it in the
future.

> it might be case where the algorithm
> will require another semantic (like a acquire or release) and it will
> require to drop the atomic usage to avoid generate subpar code.

I don't quite understand what you mean, sorry.