Re: BZ 20822 :powerpc: race condition in __lll_unlock_elision

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2016-11-22 at 09:55 +0100, Florian Weimer wrote:
> On 11/22/2016 09:44 AM, Torvald Riegel wrote:
> 
> >> In this case (with the proposed patch) the adapt_count update is within
> >> the critical region and adding C11 atomics does not add value and would
> >> generally make the logical harder to read.
> >>
> >> __atomic_store_n (adapt_count, (__atomic_load_n(adapt_count,
> >> __ATOMIC_RELAXED) - 1), __ATOMIC_RELAXED)
> >>
> >> Is a lot of syntax without any real value.
> >
> > First, it would actually be this in glibc:
> > atomic_store_relaxed (&adapt_count,
> >   atomic_load_relaxed(&adapt_count) - 1);
> >
> > Second, it does add value in that it makes it clear that there are
> > concurrent accesses elsewhere that are not in transactions.  We have
> > consensus to use the new C11-like atomics in new or changed code, and I
> > don't see a reason to make an exception here.
> 
> A possible reason: The code does not actually follow the C11 memory 
> model, so using C11-like atomics is rather misleading.

While it is true that the C11 memory model does not model HW
transactions, it is not true to claim that this code is unrelated to the
C11 memory model.  First of all, there is code outside of transactions
that accesses adapt_count, so we obviously need atomic operations there.
Second, there needs to be *some* memory model, and it needs to
incorporate the compiler too.

> And unlike code 
> using incorrect synchronization or legacy atomics, we don't have a path 
> towards the C11 model because what the code does is completely outside 
> it, so there's no argument on favor of gradual/partial conversion.

The HW transaction has to synchronize with nontransactional code.  We're
obviously going to use the C11 model for nontransactional code.  Thus,
it cannot be unrelated.  It's true that we don't have a detailed
description of how HW transactions *extend* the C11 model, but it
clearly would be an extension because it simply has to fit in there and
it will use the same underlying mechanisms.

One can also think about it this way: There is nontransactional code
that has to use atomics (because there are concurrent accesses from
other nontransactional code as well as transactional code).  So we are
going to use atomics there.  Then, even if just for consistency, we're
going to use (relaxed MO) atomic accesses too for all other concurrent
accesses to the same data.