diff mbox

libitm: Fix handling of reentrancy in the HTM fastpath

Message ID 1371721764.964.468.camel@triegel.csb
State New
Headers show

Commit Message

Torvald Riegel June 20, 2013, 9:49 a.m. UTC 
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 <triegel@redhat.com>
Date:   Thu Jun 20 00:46:59 2013 +0200

    libitm: Handle HTM fastpath in status query functions.

Comments

Peter Bergner June 20, 2013, 1:49 p.m. UTC | #1 
On Thu, 2013-06-20 at 11:49 +0200, Torvald Riegel wrote:
> 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.

Of course you're right too.  I totally missed we were already caching
that value.  Ok, I removed the unneeded call to htm_available() in my
htm_transaction_active() and all is well using your second patch.
Thanks again!

Peter
Richard Henderson June 20, 2013, 4:22 p.m. UTC | #2 
On 06/20/2013 02:49 AM, Torvald Riegel wrote:
> commit c8352d4d9fa5cfa3453a61c581956835de9753e5
> Author: Torvald Riegel <triegel@redhat.com>
> Date:   Thu Jun 20 00:46:59 2013 +0200
> 
>     libitm: Handle HTM fastpath in status query functions.

Ok.


r~
diff mbox

Patch

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;
 }