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

Torvald Riegel <triegel@redhat.com> writes:

> On Thu, 2015-07-30 at 23:06 -0300, Tulio Magno Quites Machado Filho
> wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> > 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 agree that a compiler should treat transaction begin / commit as
> something like a lock acquisition / release, assuming that the
> transaction semantics are equivalence to a single-global-lock semantics.

This has been reported to GCC as
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

> As far as glibc is concerned, I believe it's better to work around the
> compiler issue by creating a glibc-internal wrapper for __builtin_tbegin
> that enforces the semantics we believe it should have (eg, by adding the
> compiler barrier).

Agreed.  In fact, that's the right way of solving this issue.
The way I wrote my patch left some room for an instruction to be placed
between the compiler barrier and the begin of the transaction.

Anyway, I believe that's another problem and should not be confused with the
previous one.  So, I'll split this patch in 2: one that fixes BZ #18743 and
another one that fixes the compiler barriers on powerpc.  I'll synchronize
with GCC so that we use the same semantics (or as close as we can).

-- 
Tulio Magno