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

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

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

>> The previous code used to evaluate the preprocessor token is_lock_free to
>> a variable before starting a transaction.  This behavior can cause an
>> error if another thread got the lock (without using a transaction)
>> between the conversion of the token and the beginning of the transaction.
>> 
>> This patch delays the evaluation of is_lock_free to inside a transaction
>> by moving this part of the code to the macro ELIDE_LOCK.
>
> Could you elaborate more why this is a race-condition on the is_lock_free
> variable? My understanding is 'is_lock_free' is check iff the transaction
> already started and it should be safe since 'tbegin.' creates a memory
> barrier that immediately precedes the transaction (ISA 2.07, Book II, 
> Chapter 1.8):

I completely agree with you here.  The problem is just where the memory access
happens.
And we're dealing with 2 different problems related to the memory access.
Let me use your example to explain the first one:

>  40       if (__builtin_tbegin (0))
>  41         {
>  42           if (is_lock_free)

If we consider that the memory access happens in this line, there wouldn't be
a problem.  But in this context, is_lock_free is just an int and not the lock
struct we need to access.
In this context, is_lock_free is defined here:

  29 static inline bool
  30 __elide_lock (uint8_t *adapt_count, int is_lock_free)

Keep in mind this macro:

  69 # define ELIDE_LOCK(adapt_count, is_lock_free) \
  70   __elide_lock (&(adapt_count), is_lock_free)

Using pthread_rwlock_rdlock as an example, is_lock_free would be a preprocessor
token with the following contents:

 128   if (ELIDE_LOCK (rwlock->__data.__rwelision,
 129                   rwlock->__data.__lock == 0
 130                   && rwlock->__data.__writer == 0
 131                   && rwlock->__data.__nr_readers == 0))

And this is where the important memory access happens.
If one of these fields are changed by another thread after this point, but
before __builtin_tbegin, we're able to reproduce the first problem I mentioned
previously.

> So if the transaction fails it will just jump to else block.
>
>> +    else								\
>> +      for (int i = __elision_aconf.try_tbegin; i > 0; i--)		\
>> +	{								\
>> +	  asm volatile("" ::: "memory");				\
>
> I can't really understand the requirement of this compiler barrier here.  If
> compiler is moving around the 'is_lock_free' *before* __builtin_tbegin IMHO
> this is a compiler bug.

This is the second problem and I agree with you again.  IMHO,  __builtin_tbegin
starts a critical path and the compiler should not be moving a memory access out
of the critical path or into the critical path.
However, as the current implementations of GCC have this "issue" (some people
may not agree with us), I believe we could carry this compiler barrier at least
until all GCC versions supporting __builtin_tbegin are fixed.

I'm planning to report this to the GCC community.

-- 
Tulio Magno