This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ 18743] PowerPC: Fix a race condition when eliding a lock
- From: Torvald Riegel <triegel at redhat dot com>
- To: Peter Bergner <bergner at vnet dot ibm dot com>
- Cc: Tulio Magno Quites Machado Filho <tuliom at linux dot vnet dot ibm dot com>, Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, libc-alpha at sourceware dot org
- Date: Thu, 08 Oct 2015 18:13:40 +0200
- Subject: Re: [PATCH][BZ 18743] PowerPC: Fix a race condition when eliding a lock
- Authentication-results: sourceware.org; auth=none
- References: <1438274936-26493-1-git-send-email-tuliom at linux dot vnet dot ibm dot com> <55BA703D dot 7010303 at linaro dot org> <874mkl3wtq dot fsf at totoro dot lan> <1440103701 dot 5188 dot 46 dot camel at otta> <1440332176 dot 27492 dot 49 dot camel at localhost dot localdomain> <1441126884 dot 5089 dot 128 dot camel at otta>
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.