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

[Peter Bergner <bergner at vnet dot ibm dot com>]

On Thu, 2015-07-30 at 23:06 -0300, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> >> +    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 completely agree we need a barrier here, so I will fix that.  The question
I'm starting to wonder, is do we need more than just a memory barrier or
do we need a complete optimization barrier, so no code (even scalar code)
will move past the __builtin_tbegin() (and other HTM builtins)?

Let's look at the following test case:

  long
  foo (long dest, long src0, long src1, long tries)
  {
    long i;
    for (i = 0; i < tries; i++)
      {
        if (__builtin_tbegin (0))
	  {
	    dest = src0 + src1;
	    if (dest == 13)
	      __builtin_tabort(0);
	    __builtin_tend (0);
	  }
      }
    return dest;
  }

If I compile this with -O3 -mcpu=power8 -S, I see the "src0 + src1"
expression hoisted out of the loop, which I can accept as "ok", since
both src0 and src1 are constant for the whole function.  However, I
am also seeing the assignment of dest as well as the compare of dest
with 13 also being moved outside the tbegin/tend pair.  This doesn't
seem correct to me, since the update to dest should only ever be
observable if one or more of the transactions succeeds.  Torvald,
Adhemerval and Tulio, do you guys agree with that assessment?

Peter