Patchwork libitm: Fix handling of reentrancy in the HTM fastpath

login
register
mail settings
Submitter Torvald Riegel
Date June 19, 2013, 10:51 p.m.
Message ID <1371682261.964.3.camel@triegel.csb>
Download mbox | patch
Permalink /patch/252687/
State New
Headers show

Comments

Torvald Riegel - June 19, 2013, 10:51 p.m.
On Wed, 2013-06-19 at 14:43 -0500, Peter Bergner wrote: 
> On Wed, 2013-06-19 at 10:57 -0500, Peter Bergner wrote:
> > On Wed, 2013-06-19 at 10:49 -0500, Peter Bergner wrote:
> > > This is due to the following in _ITM_inTransaction():
> > > 
> > > 47	  if (tx && (tx->nesting > 0))
> > > (gdb) p tx
> > > $2 = (GTM::gtm_thread *) 0x10901bf0
> > > (gdb) p tx->nesting
> > > $3 = 1
> > > (gdb) step
> > > 49	      if (tx->state & gtm_thread::STATE_IRREVOCABLE)
> > > (gdb) p tx->state
> > > $4 = 3
> > > (gdb) p gtm_thread::STATE_IRREVOCABLE
> > > $5 = 2
> > > (gdb) step
> > > 50		return inIrrevocableTransaction;
> > 
> > Bah, ignore this.  It's a different call that is returning something other
> > than inIrrevocableTransaction.  Unfortunately, gdb is having problems inside
> > hw txns and 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!

Torvald
commit 02dde6bb91107792fb0cb9f5c4785d25b6aa0e3c
Author: Torvald Riegel <triegel@redhat.com>
Date:   Thu Jun 20 00:46:59 2013 +0200

    libitm: Handle HTM fastpath in status query functions.
Peter Bergner - June 20, 2013, 3:13 a.m.
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.


Peter
Andreas Krebbel - June 20, 2013, 5:09 p.m.
On 20/06/13 05:13, 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'd still like to hear from Andreas, whether the reentrant.c test case
> with both patches, now works on S390.

The patches fix the testcase also on s390. Thanks!

Bye,

-Andreas-

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..0ac3eda 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_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_transaction_active())
+    htm_abort();
+#endif
   struct gtm_thread *tx = gtm_thr();
   return (tx && (tx->nesting > 0)) ? tx->id : _ITM_noTransactionId;
 }