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

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2015-08-25 at 10:15 -0400, Carlos O'Donell wrote:
> As a concrete example the structure element that is accessed atomically is
> rwlock->__data.__lock. We access it atomically in lll_lock and also
> in the txnal region (is region the right word?).

Txnal region is used by some, so I think this works.  Just transaction
would work as well I think.

> The access is part of:
> 
> nptl/pthread_rwlock_wrlock.c 
> 
> 100   if (ELIDE_LOCK (rwlock->__data.__rwelision,
> 101                   rwlock->__data.__lock == 0
> 102                   && rwlock->__data.__writer == 0
> 103                   && rwlock->__data.__nr_readers == 0))
> 104     return 0;
> 
> With the intent being for the expression to be evaluted inside of the
> transaction and thus abort if another thread has touched any of those
> fields.
> 
> That entire expression expands into the usage of is_lock_free. Therefore
> shouldn't the change be at the caller site?

That would be an option, or we should pass in a function or something.

> e.g.
> 
> 100   if (ELIDE_LOCK (rwlock->__data.__rwelision,
> 101                   atomic_load_relaxed (rwlock->__data.__lock) == 0
> 102                   && rwlock->__data.__writer == 0
> 103                   && rwlock->__data.__nr_readers == 0))
> 104     return 0;
> 
> Since the powerpc elision backend doesn't know anything about the types
> that went into the evaluation of is_lock_free?
> 
> If anything I think *we* have the onus to fix these cases of missing
> atomic_load_relaxed not IBM?

Somebody should do it :)  I hadn't thought too much about for whom it
would be most convenient to do.  As I wrote in the review for Tulio's
patch, IMO passing in an expression into a macro that has to be
evaluated in there is pretty ugly and seems to be at least related to
the bug Tulio is fixing.  Maybe we can pass in a function that is
inlined by the compiler.
I do agree that IBM just used what the Intel implementation did, and
thus didn't create that in the first place.