On Wed, 2013-06-19 at 22:13 -0500, Peter Bergner wrote: > On Thu, 2013-06-20 at 00:51 +0200, Torvald Riegel wrote: > > On Wed, 2013-06-19 at 14:43 -0500, Peter Bergner wrote: > > >> I'm having trouble seeing why/when _ITM_inTransaction() is > > >> returning something other than inIrrevocableTransaction. I'll see if I > > >> can > > >> determine why and will report back. > > > > > > Ok, we return outsideTransaction because the nesting level (tx->nesting) > > > is zero. > > > > That's a second bug in libitm, sorry. Can you try with the attached > > patch additionally to the previous one? Thanks! > > Ok, with this patch, plus adding a powerpc implementation of > htm_transaction_active(), reentrant.c now executes correctly > on both HTM and non-HTM hardware for me. Thanks for all of > your help with this! > > I'd still like to hear from Andreas, whether the reentrant.c test case > with both patches, now works on S390. > > I'll note unlike your x86 htm_transaction_active() implementation, > my implementation has to check for htm_available() first before > executing the htm instruction which tells me whether I'm in transaction > state or not, otherwise I'll get a SIGILL on non-HTM hardware. > Unfortunately, my htm_available() call uses getauxval() to query > the AUXV for a hwcap bit. Is there a place I can stash the result > of the first call, so that subsequent calls use the cached value? > Normally, I could use a static var, but I doubt that is what we want > to do in static inline functions.
You're right, that was missing for x86 as well. Please see the updated second patch that is attached. It additionally checks htm_fastpath to see whether we are actually using the HTM. This variable is initialized to the value that htm_init() returns. This should do the right thing I suppose. Torvald
commit c8352d4d9fa5cfa3453a61c581956835de9753e5 Author: Torvald Riegel <trie...@redhat.com> Date: Thu Jun 20 00:46:59 2013 +0200 libitm: Handle HTM fastpath in status query functions. diff --git a/libitm/config/x86/target.h b/libitm/config/x86/target.h index 77b627f..063c09e 100644 --- a/libitm/config/x86/target.h +++ b/libitm/config/x86/target.h @@ -125,6 +125,13 @@ htm_abort_should_retry (uint32_t begin_ret) { 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 diff --git a/libitm/query.cc b/libitm/query.cc index 5707321..39a35b3 100644 --- a/libitm/query.cc +++ b/libitm/query.cc @@ -43,6 +43,15 @@ _ITM_libraryVersion (void) _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_inTransaction (void) _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; }