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

["Carlos O'Donell" <carlos at redhat dot com>]

On 08/21/2015 05:52 AM, Torvald Riegel wrote:
> On Thu, 2015-08-20 at 16:51 -0300, Adhemerval Zanella wrote:
>>
>>
>>>> +
>>>> +/* Returns 0 if the lock defined by is_lock_free was elided.
>>>> +   ADAPT_COUNT is a per-lock state variable.  */
>>>> +# define ELIDE_LOCK(adapt_count, is_lock_free)				\
>>>> +  ({									\
>>>> +    int ret = 0;							\
>>>> +    if (adapt_count > 0)						\
>>>> +      (adapt_count)--;							\
>>>> +    else								\
>>>> +      for (int i = __elision_aconf.try_tbegin; i > 0; i--)		\
>>>> +	{								\
>>>> +	  if (__builtin_tbegin (0))					\
>>>> +	    {								\
>>>> +	      if (is_lock_free)						\
>>>
>>> This should use a relaxed MO atomic access to highlight that this data *is*
>>> shared between threads, but that there are no ordering guarantees. It should
>>> result in a normal load, but it makes it clear in the code that some other
>>> thread is also accessing this data, and that the transaction as a barrier
>>> is all that is protecting this from being a data-race (undefined behaviour).
>>
>> Should we really highlight it using atomic operations? The idea is exactly
>> that any access within a transaction will not result in undefined behavior,
>> so no need to atomic for synchronization.
> 
> I think it's better to highlight it.  If the data would only be accessed
> from within transaction, the highlighting wouldn't be necessary.  But it
> is mixed transactional/nontransactional synchronization in this case, so
> we need the annotation.

I'll let you discuss this with Tulio. Unless there is a correctness issue
or a violation of our rule to be data race free, then there should be some
flexibility granted to IBM to do what they wish, and particularly that which
is optimal. While the atomic_load_relaxed doesn't appear to do anything that
would incur a serious performance penality, it does add an asm with inputs
that force the value to a register and this may limit the compiler in some
future way.

>From my perspective a code comment on concurrency seems sufficient?

If thread T1 and a thread T2 read or write memory that is used to evaluate
the value of is_lock_free, then the transaction would be aborted and thus
is_lock_free needs no atomic access?

> This also makes moving to proper atomic types easier should we ever
> decide to do that.

Could you please elaborate a bit more on this?

Cheers,
Carlos.