This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, 2015-09-01 at 12:01 -0500, Peter Bergner wrote:
> On Sun, 2015-08-23 at 14:16 +0200, Torvald Riegel wrote:
> > To summarize, I think tbegin needs to have lock acquisition semantics
> > and an unknown return value, and tend needs to have lock release
> > semantics.
> > 
> > Does everyone agree with this reasoning?
> 
> I agree.  Since POWER's and I also believe Intel's and S390's TM
> hardware instructions have at least those semantics, I believe the
> only thing the compiler needs to do is enforce a memory barrier on
> those instruction patterns, so the compiler won't schedule loads and
> stores across those instructions, correct?

Yes, I think a (code motion) barrier is sufficient.  In the future,
these could be acquire/release like code motion barriers so that we can
move code into transactions, but we don't have these yet in GCC.  Also,
moving code into a transaction is fine from a safety point of view, but
the compiler would still have to ensure that forward progress
requirements aren't violated.  Thus, just not moving code into or out of
transactions seems best.

> My patch to do this on POWER fixes the unit test cases I have and I
> assume the TLE issue we ran into.  Intel and S390 will also need a
> similar patch.  I'll mention that as part of my patch, I created a
> __TM_FENCE__ macro so the user can tell whether the TM insn patterns
> have been fixed to act as fences or not.
> 
> 
> > Second, beyond concurrency, we have to consider whether we get
> > additional constraints on code generation because of txn aborts.
> 
> The only additional constraint I can think of that might be useful,
> is to mark the tabort. as noreturn.  However, noreturn is a function
> attribute, so we can't really attach it to a TM insn pattern.
> Even if we could, it wouldn't make sense to attach it to our
> conditional transaction abort instructions.  Probably users should just
> follow their __builtin_tabort (0) uses with __builtin_unreachable().
> 
> I will mention that I did end up adding memory barriers to our tabort
> and conditional tabort insn patterns.  It's "safe" and I assume not
> much of a performance issue, since aborting a transaction should be
> an uncommon occurrence and the tabort is also probably slow too.

Agreed.  (But it might not be strictly necessary, as long as the
compiler knows that tabort has an effect that can't be undone.  (If, for
example, a tabort will happen anyway, it doesn't matter whether some
memory access is moved to after the tabort. Moving before the tabort is
not always possible because the access might have an effect like raising
a segfault.)

> I also had to add it to our tsuspend and tresume patterns too, since
> we don't want loads/stores moving from a transaction area to a
> non-transaction area and vice versa, so it made sense to just
> attach the memory barrier to all our transaction insn patterns.

Good point.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]