| Message ID | 1393637537.26373.4.camel@otta |
|---|---|
| State | New |
| Headers | show |
On Sat, Mar 1, 2014 at 2:32 AM, Peter Bergner <bergner@vnet.ibm.com> wrote: > I'd like to ask for permission to backport the following two LIBITM bug > fixes to the FSF 4.8 branch. Although these are not technically fixing > regressions, they do fix the libitm.c/reentrant.c testsuite failure on > s390 and powerpc (or at least it will when we finally get our power8 > code backported to FSF 4.8). It also fixes a real bug on x86 that is > latent because we don't currently have a test case that warms up the > x86's RTM hardware enough such that its xbegin succeeds exposing the > bug. I'd like this backport so that the 4.8 based distros won't need > to carry this as an add-on patch. > > It should also be fairly safe as well, since the fixed code is limited > to the arches (x86, s390 and powerpc) that define USE_HTM_FASTPATH, > so all others definitely won't see a difference. Works for me in case nobody else objects within 24h. Thanks, Richard. > I'll note I CC'd some of the usual suspects interested in TM as well > as the normal RMs, because LIBITM doesn't seem to have a maintainer > or reviewer listed in the MAINTAINERS file. Is that an oversight or??? > > Peter > > Backport from mainline > 2013-06-20 Torvald Riegel <triegel@redhat.com> > > * query.cc (_ITM_inTransaction): Abort when using the HTM fastpath. > (_ITM_getTransactionId): Same. > * config/x86/target.h (htm_transaction_active): New. > > 2013-06-20 Torvald Riegel <triegel@redhat.com> > > PR libitm/57643 > * beginend.cc (gtm_thread::begin_transaction): Handle reentrancy in > the HTM fastpath. > > Index: libitm/beginend.cc > =================================================================== > --- libitm/beginend.cc (revision 208151) > +++ libitm/beginend.cc (working copy) > @@ -197,6 +197,8 @@ > // We are executing a transaction now. > // Monitor the writer flag in the serial-mode lock, and abort > // if there is an active or waiting serial-mode transaction. > + // Note that this can also happen due to an enclosing > + // serial-mode transaction; we handle this case below. > if (unlikely(serial_lock.is_write_locked())) > htm_abort(); > else > @@ -219,6 +221,14 @@ > tx = new gtm_thread(); > set_gtm_thr(tx); > } > + // Check whether there is an enclosing serial-mode transaction; > + // if so, we just continue as a nested transaction and don't > + // try to use the HTM fastpath. This case can happen when an > + // outermost relaxed transaction calls unsafe code that starts > + // a transaction. > + if (tx->nesting > 0) > + break; > + // Another thread is running a serial-mode transaction. Wait. > serial_lock.read_lock(tx); > serial_lock.read_unlock(tx); > // TODO We should probably reset the retry count t here, unless > Index: libitm/config/x86/target.h > =================================================================== > --- libitm/config/x86/target.h (revision 208151) > +++ libitm/config/x86/target.h (working copy) > @@ -125,6 +125,13 @@ > { > return begin_ret & _XABORT_RETRY; > } > + > +/* Returns true iff a hardware transaction is currently being executed. */ > +static inline bool > +htm_transaction_active () > +{ > + return _xtest() != 0; > +} > #endif > > > Index: libitm/query.cc > =================================================================== > --- libitm/query.cc (revision 208151) > +++ libitm/query.cc (working copy) > @@ -43,6 +43,15 @@ > _ITM_howExecuting ITM_REGPARM > _ITM_inTransaction (void) > { > +#if defined(USE_HTM_FASTPATH) > + // If we use the HTM fastpath, we cannot reliably detect whether we are > + // in a transaction because this function can be called outside of > + // a transaction and thus we can't deduce this by looking at just the serial > + // lock. This function isn't used in practice currently, so the easiest > + // way to handle it is to just abort. > + if (htm_fastpath && htm_transaction_active()) > + htm_abort(); > +#endif > struct gtm_thread *tx = gtm_thr(); > if (tx && (tx->nesting > 0)) > { > @@ -58,6 +67,11 @@ > _ITM_transactionId_t ITM_REGPARM > _ITM_getTransactionId (void) > { > +#if defined(USE_HTM_FASTPATH) > + // See ITM_inTransaction. > + if (htm_fastpath && htm_transaction_active()) > + htm_abort(); > +#endif > struct gtm_thread *tx = gtm_thr(); > return (tx && (tx->nesting > 0)) ? tx->id : _ITM_noTransactionId; > } > >
On Fri, 2014-02-28 at 19:32 -0600, Peter Bergner wrote: > I'd like to ask for permission to backport the following two LIBITM bug > fixes to the FSF 4.8 branch. Although these are not technically fixing > regressions, they do fix the libitm.c/reentrant.c testsuite failure on > s390 and powerpc (or at least it will when we finally get our power8 > code backported to FSF 4.8). It also fixes a real bug on x86 that is > latent because we don't currently have a test case that warms up the > x86's RTM hardware enough such that its xbegin succeeds exposing the > bug. I'd like this backport so that the 4.8 based distros won't need > to carry this as an add-on patch. > > It should also be fairly safe as well, since the fixed code is limited > to the arches (x86, s390 and powerpc) that define USE_HTM_FASTPATH, > so all others definitely won't see a difference. Looks good to me. > I'll note I CC'd some of the usual suspects interested in TM as well > as the normal RMs, because LIBITM doesn't seem to have a maintainer > or reviewer listed in the MAINTAINERS file. Is that an oversight or??? I'm reviewing all libitm patches that I'm aware of (but I don't read gcc-patches regularly). Should I add myself as maintainer for libitm? Does this come with any other responsibilities than reviewing patches? Torvald
On 03/03/2014 04:48 AM, Torvald Riegel wrote: > Should I add myself as maintainer for libitm? Yes. > Does this come with any other responsibilities than reviewing patches? No. r~
On Mon, 2014-03-03 at 13:48 +0100, Torvald Riegel wrote: > On Fri, 2014-02-28 at 19:32 -0600, Peter Bergner wrote: > > I'd like to ask for permission to backport the following two LIBITM bug > > fixes to the FSF 4.8 branch. Although these are not technically fixing > > regressions, they do fix the libitm.c/reentrant.c testsuite failure on > > s390 and powerpc (or at least it will when we finally get our power8 > > code backported to FSF 4.8). It also fixes a real bug on x86 that is > > latent because we don't currently have a test case that warms up the > > x86's RTM hardware enough such that its xbegin succeeds exposing the > > bug. I'd like this backport so that the 4.8 based distros won't need > > to carry this as an add-on patch. > > > > It should also be fairly safe as well, since the fixed code is limited > > to the arches (x86, s390 and powerpc) that define USE_HTM_FASTPATH, > > so all others definitely won't see a difference. > > Looks good to me. Ok, committed as revision 208295. Thanks everyone! Peter
Index: libitm/beginend.cc =================================================================== --- libitm/beginend.cc (revision 208151) +++ libitm/beginend.cc (working copy) @@ -197,6 +197,8 @@ // We are executing a transaction now. // Monitor the writer flag in the serial-mode lock, and abort // if there is an active or waiting serial-mode transaction. + // Note that this can also happen due to an enclosing + // serial-mode transaction; we handle this case below. if (unlikely(serial_lock.is_write_locked())) htm_abort(); else @@ -219,6 +221,14 @@ tx = new gtm_thread(); set_gtm_thr(tx); } + // Check whether there is an enclosing serial-mode transaction; + // if so, we just continue as a nested transaction and don't + // try to use the HTM fastpath. This case can happen when an + // outermost relaxed transaction calls unsafe code that starts + // a transaction. + if (tx->nesting > 0) + break; + // Another thread is running a serial-mode transaction. Wait. serial_lock.read_lock(tx); serial_lock.read_unlock(tx); // TODO We should probably reset the retry count t here, unless Index: libitm/config/x86/target.h =================================================================== --- libitm/config/x86/target.h (revision 208151) +++ libitm/config/x86/target.h (working copy) @@ -125,6 +125,13 @@ { return begin_ret & _XABORT_RETRY; } + +/* Returns true iff a hardware transaction is currently being executed. */ +static inline bool +htm_transaction_active () +{ + return _xtest() != 0; +} #endif Index: libitm/query.cc =================================================================== --- libitm/query.cc (revision 208151) +++ libitm/query.cc (working copy) @@ -43,6 +43,15 @@ _ITM_howExecuting ITM_REGPARM _ITM_inTransaction (void) { +#if defined(USE_HTM_FASTPATH) + // If we use the HTM fastpath, we cannot reliably detect whether we are + // in a transaction because this function can be called outside of + // a transaction and thus we can't deduce this by looking at just the serial + // lock. This function isn't used in practice currently, so the easiest + // way to handle it is to just abort. + if (htm_fastpath && htm_transaction_active()) + htm_abort(); +#endif struct gtm_thread *tx = gtm_thr(); if (tx && (tx->nesting > 0)) { @@ -58,6 +67,11 @@ _ITM_transactionId_t ITM_REGPARM _ITM_getTransactionId (void) { +#if defined(USE_HTM_FASTPATH) + // See ITM_inTransaction. + if (htm_fastpath && htm_transaction_active()) + htm_abort(); +#endif struct gtm_thread *tx = gtm_thr(); return (tx && (tx->nesting > 0)) ? tx->id : _ITM_noTransactionId; }