Patchwork [trans-mem] Move state from gtm_thread to gtm_transaction

login
register
mail settings
Submitter Torvald Riegel
Date Aug. 3, 2011, 11:04 a.m.
Message ID <1312369498.3533.235.camel@triegel.csb>
Download mbox | patch
Permalink /patch/108118/
State New
Headers show

Comments

Torvald Riegel - Aug. 3, 2011, 11:04 a.m.
patch3:
After the previous change to flat nesting by default, checkpoints, and
thus having only a single gtm_transaction object per thread, we can move
the local_tid field to gtm_transaction too. As a result, we don't have
to access gtm_thread in GTM_begin_transaction anymore.

patch4:
A small fix that adds a missing set-up call for gtm_thread. And a
matching test case, which tests that the functions in query.cc work even
if no transaction has been executed so far in this thread.

OK for branch?
commit 33c3c869c7cb986bcca3f3f44b0412278e880a82
Author: Torvald Riegel <triegel@redhat.com>
Date:   Fri Jul 29 16:09:05 2011 +0200

    Move local_tid from gtm_thread to gtm_transaction.
    
    	* config/generic/tls.h (gtm_thread): Move local_tid from here ...
    	* libitm_i.h (local_tid): ... to here.
    	* beginend.cc (GTM::gtm_transaction::begin_transaction): Same.
    	(GTM::gtm_transaction::operator new): Set up gtm_thread if necessary.
commit 22c19978ba11c0ae0321449d8809260ab5f97741
Author: Torvald Riegel <triegel@redhat.com>
Date:   Fri Jul 29 16:19:41 2011 +0200

    Fix missing setup of gtm_thread if no transaction has run yet.
    
    	* query.cc (_ITM_getThreadnum): Set up gtm_thread if necessary.
    	* testsuite/libitm.c/notx.c: New file.

diff --git a/libitm/query.cc b/libitm/query.cc
index 9a95211..02f1813 100644
--- a/libitm/query.cc
+++ b/libitm/query.cc
@@ -67,7 +67,7 @@ int ITM_REGPARM
 _ITM_getThreadnum (void)
 {
   static int global_num;
-  struct gtm_thread *thr = gtm_thr();
+  struct gtm_thread *thr = setup_gtm_thr();
   int num = thr->thread_num;
 
   if (num == 0)
diff --git a/libitm/testsuite/libitm.c/notx.c b/libitm/testsuite/libitm.c/notx.c
new file mode 100644
index 0000000..92ae3e1
--- /dev/null
+++ b/libitm/testsuite/libitm.c/notx.c
@@ -0,0 +1,36 @@
+/* These tests all check whether initialization happens properly even if no
+   transaction has been used in the current thread yet.  */
+#include <stdlib.h>
+#include <pthread.h>
+#include <libitm.h>
+
+static void *test1 (void *dummy __attribute__((unused)))
+{
+  if (_ITM_inTransaction() != outsideTransaction)
+    abort();
+  return NULL;
+}
+
+static void *test2 (void *dummy __attribute__((unused)))
+{
+  if (_ITM_getTransactionId() != _ITM_noTransactionId)
+    abort();
+  return NULL;
+}
+
+
+int main()
+{
+  pthread_t thread;
+
+  if (_ITM_getThreadnum() != 1)
+    abort();
+
+  pthread_create(&thread, NULL, test1, NULL);
+  pthread_join(thread, NULL);
+
+  pthread_create(&thread, NULL, test2, NULL);
+  pthread_join(thread, NULL);
+
+  return 0;
+}
Richard Henderson - Aug. 4, 2011, 3:43 p.m.
On 08/03/2011 04:04 AM, Torvald Riegel wrote:
>     Move local_tid from gtm_thread to gtm_transaction.
>     
>     	* config/generic/tls.h (gtm_thread): Move local_tid from here ...
>     	* libitm_i.h (local_tid): ... to here.
>     	* beginend.cc (GTM::gtm_transaction::begin_transaction): Same.
>     	(GTM::gtm_transaction::operator new): Set up gtm_thread if necessary.

This seems very wrong.  Why?

>     Fix missing setup of gtm_thread if no transaction has run yet.
>     
>     	* query.cc (_ITM_getThreadnum): Set up gtm_thread if necessary.
>     	* testsuite/libitm.c/notx.c: New file.

Ok.


r~
Torvald Riegel - Aug. 4, 2011, 4:22 p.m.
On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote:
> On 08/03/2011 04:04 AM, Torvald Riegel wrote:
> >     Move local_tid from gtm_thread to gtm_transaction.
> >     
> >     	* config/generic/tls.h (gtm_thread): Move local_tid from here ...
> >     	* libitm_i.h (local_tid): ... to here.
> >     	* beginend.cc (GTM::gtm_transaction::begin_transaction): Same.
> >     	(GTM::gtm_transaction::operator new): Set up gtm_thread if necessary.
> 
> This seems very wrong.  Why?

What seems wrong?

local_tid is per thread and there is one gtm_transaction object per
thread, so moving it is correct, or not?
The purpose of this is to not having to access gtm_thread anymore in
begin. And it's a step towards merging gtm_thread and gtm_transaction
completely.

Do you agree?
Richard Henderson - Aug. 4, 2011, 6:55 p.m.
On 08/04/2011 09:22 AM, Torvald Riegel wrote:
> On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote:
>> On 08/03/2011 04:04 AM, Torvald Riegel wrote:
>>>     Move local_tid from gtm_thread to gtm_transaction.
>>>     
>>>     	* config/generic/tls.h (gtm_thread): Move local_tid from here ...
>>>     	* libitm_i.h (local_tid): ... to here.
>>>     	* beginend.cc (GTM::gtm_transaction::begin_transaction): Same.
>>>     	(GTM::gtm_transaction::operator new): Set up gtm_thread if necessary.
>>
>> This seems very wrong.  Why?
> 
> What seems wrong?
> 
> local_tid is per thread and there is one gtm_transaction object per
> thread, so moving it is correct, or not?
> The purpose of this is to not having to access gtm_thread anymore in
> begin. And it's a step towards merging gtm_thread and gtm_transaction
> completely.
> 
> Do you agree?

If you're going to merge gtm_thread and gtm_transaction, why
are you moving things away from gtm_thread?

As for "not having to access gtm_thread", the non-glibc case
for accessing gtm_txn is to pull the value out of gtm_thread.

So I think you're moving data in the wrong direction.


r~
Torvald Riegel - Aug. 4, 2011, 7:24 p.m.
On Thu, 2011-08-04 at 11:55 -0700, Richard Henderson wrote:
> On 08/04/2011 09:22 AM, Torvald Riegel wrote:
> > On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote:
> >> On 08/03/2011 04:04 AM, Torvald Riegel wrote:
> >>>     Move local_tid from gtm_thread to gtm_transaction.
> >>>     
> >>>     	* config/generic/tls.h (gtm_thread): Move local_tid from here ...
> >>>     	* libitm_i.h (local_tid): ... to here.
> >>>     	* beginend.cc (GTM::gtm_transaction::begin_transaction): Same.
> >>>     	(GTM::gtm_transaction::operator new): Set up gtm_thread if necessary.
> >>
> >> This seems very wrong.  Why?
> > 
> > What seems wrong?
> > 
> > local_tid is per thread and there is one gtm_transaction object per
> > thread, so moving it is correct, or not?
> > The purpose of this is to not having to access gtm_thread anymore in
> > begin. And it's a step towards merging gtm_thread and gtm_transaction
> > completely.
> > 
> > Do you agree?
> 
> If you're going to merge gtm_thread and gtm_transaction, why
> are you moving things away from gtm_thread?

I believe the per-thread object should be gtm_transaction, not
gtm_thread. Regarding the name, "transaction" makes more sense to me
because it has almost no uses besides transaction-related stuff.
Second, I guess we'd have to keep it in libitm_i.h anyway, and not in
tls.h. Therefore, it seems to be the right thing to do to put it
together with the related stuff, even if we rename gtm_transaction to
gtm_thread later, or move it out of libitm_i.h (why would we? where
to?).

> As for "not having to access gtm_thread", the non-glibc case
> for accessing gtm_txn is to pull the value out of gtm_thread.

I'm aware of that, but non-glibc is not the common case we're optimizing
for. If so, I believe we'd have kept the tx param in all the _ITM_* load
and store functions.
Either way, when gtm_thread and gtm_transaction have been merged (and
I'm thinking about merging into gtm_transaction here...), this issue
should also have gone away.

I'm planning to come up with a separate patch for the actual merge. I
first need to think a bit more about whether the destruction can be done
with just a destructor of the TLS object, what happens when other
object's destructors use transactions, etc. Even with one txn per
thread, we still need to deregister it and release its dynamically
allocated memory. Perhaps adding a destruction-phase mode would help, in
which transactions are always destructed after they've run.

Torvald
Richard Henderson - Aug. 4, 2011, 7:35 p.m.
On 08/04/2011 12:24 PM, Torvald Riegel wrote:
> On Thu, 2011-08-04 at 11:55 -0700, Richard Henderson wrote:
>> On 08/04/2011 09:22 AM, Torvald Riegel wrote:
>>> On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote:
>>>> On 08/03/2011 04:04 AM, Torvald Riegel wrote:
>>>>>     Move local_tid from gtm_thread to gtm_transaction.
>>>>>     
>>>>>     	* config/generic/tls.h (gtm_thread): Move local_tid from here ...
>>>>>     	* libitm_i.h (local_tid): ... to here.
>>>>>     	* beginend.cc (GTM::gtm_transaction::begin_transaction): Same.
>>>>>     	(GTM::gtm_transaction::operator new): Set up gtm_thread if necessary.
>>>>
>>>> This seems very wrong.  Why?
>>>
>>> What seems wrong?
>>>
>>> local_tid is per thread and there is one gtm_transaction object per
>>> thread, so moving it is correct, or not?
>>> The purpose of this is to not having to access gtm_thread anymore in
>>> begin. And it's a step towards merging gtm_thread and gtm_transaction
>>> completely.
>>>
>>> Do you agree?
>>
>> If you're going to merge gtm_thread and gtm_transaction, why
>> are you moving things away from gtm_thread?
> 
> I believe the per-thread object should be gtm_transaction, not
> gtm_thread. 

I don't.  For the simple fact that *all* per-thread information
should be in gtm_thread, not just some of it.  If we ever have
any non-transaction per-thread information, then it should go
into gtm_thread.

Suppose we did eliminate everything not directly related to the
current transaction from gtm_thread, I think it would look like

   struct gtm_thread
   {
     gtm_transaction txn;
   };

But even that said, the specific case of local_tid seems like
exactly the sort of information that should remain in gtm_thread.
It's *not* related to the current transaction, it's related to
the current thread.

>> As for "not having to access gtm_thread", the non-glibc case
>> for accessing gtm_txn is to pull the value out of gtm_thread.
> 
> I'm aware of that, but non-glibc is not the common case we're optimizing
> for. If so, I believe we'd have kept the tx param in all the _ITM_* load
> and store functions.

But what you're suggesting is intentionally making non-glibc worse
for no good reason.  I, and probably others, take issue with that.


r~
Torvald Riegel - Aug. 5, 2011, 11:45 p.m.
On Thu, 2011-08-04 at 12:35 -0700, Richard Henderson wrote:
> On 08/04/2011 12:24 PM, Torvald Riegel wrote:
> > On Thu, 2011-08-04 at 11:55 -0700, Richard Henderson wrote:
> >> On 08/04/2011 09:22 AM, Torvald Riegel wrote:
> >>> On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote:
> >>>> On 08/03/2011 04:04 AM, Torvald Riegel wrote:
> >>>>>     Move local_tid from gtm_thread to gtm_transaction.
> >>>>>     
> >>>>>     	* config/generic/tls.h (gtm_thread): Move local_tid from here ...
> >>>>>     	* libitm_i.h (local_tid): ... to here.
> >>>>>     	* beginend.cc (GTM::gtm_transaction::begin_transaction): Same.
> >>>>>     	(GTM::gtm_transaction::operator new): Set up gtm_thread if necessary.
> >>>>
> >>>> This seems very wrong.  Why?
> >>>
> >>> What seems wrong?
> >>>
> >>> local_tid is per thread and there is one gtm_transaction object per
> >>> thread, so moving it is correct, or not?
> >>> The purpose of this is to not having to access gtm_thread anymore in
> >>> begin. And it's a step towards merging gtm_thread and gtm_transaction
> >>> completely.
> >>>
> >>> Do you agree?
> >>
> >> If you're going to merge gtm_thread and gtm_transaction, why
> >> are you moving things away from gtm_thread?
> > 
> > I believe the per-thread object should be gtm_transaction, not
> > gtm_thread. 
> 
> I don't.  For the simple fact that *all* per-thread information
> should be in gtm_thread, not just some of it.  If we ever have
> any non-transaction per-thread information, then it should go
> into gtm_thread.
> 
> Suppose we did eliminate everything not directly related to the
> current transaction from gtm_thread, I think it would look like
> 
>    struct gtm_thread
>    {
>      gtm_transaction txn;
>    };
> 
> But even that said, the specific case of local_tid seems like
> exactly the sort of information that should remain in gtm_thread.
> It's *not* related to the current transaction, it's related to
> the current thread.

As discussed off-list, local_tid can be moved into the transaction.

The question of whether and how to merge gtm_thread and gtm_transaction
is separate from that. I have provided a first draft for that merge in
my previous email to the list.

> >> As for "not having to access gtm_thread", the non-glibc case
> >> for accessing gtm_txn is to pull the value out of gtm_thread.
> > 
> > I'm aware of that, but non-glibc is not the common case we're optimizing
> > for. If so, I believe we'd have kept the tx param in all the _ITM_* load
> > and store functions.
> 
> But what you're suggesting is intentionally making non-glibc worse
> for no good reason.  I, and probably others, take issue with that.

That was a misunderstanding/non-issue, but it also considered in the
recent merge patch.

Torvald

Patch

diff --git a/libitm/beginend.cc b/libitm/beginend.cc
index d336d60..a16c3f9 100644
--- a/libitm/beginend.cc
+++ b/libitm/beginend.cc
@@ -43,7 +43,7 @@  static _ITM_transactionId_t global_tid;
 void *
 GTM::gtm_transaction::operator new (size_t s)
 {
-  gtm_thread *thr = gtm_thr ();
+  gtm_thread *thr = setup_gtm_thr ();
   void *tx;
 
   assert(s == sizeof(gtm_transaction));
@@ -112,8 +112,6 @@  GTM::gtm_transaction::begin_transaction (uint32_t prop, const gtm_jmpbuf *jb)
   if (unlikely(prop & pr_undoLogCode))
     GTM_fatal("pr_undoLogCode not supported");
 
-  gtm_thread *thr = setup_gtm_thr ();
-
   tx = gtm_tx();
   if (tx == NULL)
     {
@@ -214,18 +212,18 @@  GTM::gtm_transaction::begin_transaction (uint32_t prop, const gtm_jmpbuf *jb)
 
   // As long as we have not exhausted a previously allocated block of TIDs,
   // we can avoid an atomic operation on a shared cacheline.
-  if (thr->local_tid & (tid_block_size - 1))
-    tx->id = thr->local_tid++;
+  if (tx->local_tid & (tid_block_size - 1))
+    tx->id = tx->local_tid++;
   else
     {
 #ifdef HAVE_64BIT_SYNC_BUILTINS
       tx->id = __sync_add_and_fetch (&global_tid, tid_block_size);
-      thr->local_tid = tx->id + 1;
+      tx->local_tid = tx->id + 1;
 #else
       pthread_mutex_lock (&global_tid_lock);
       global_tid += tid_block_size;
       tx->id = global_tid;
-      thr->local_tid = tx->id + 1;
+      tx->local_tid = tx->id + 1;
       pthread_mutex_unlock (&global_tid_lock);
 #endif
     }
diff --git a/libitm/config/generic/tls.h b/libitm/config/generic/tls.h
index 82d7c66..8a39b80 100644
--- a/libitm/config/generic/tls.h
+++ b/libitm/config/generic/tls.h
@@ -50,12 +50,6 @@  struct gtm_thread
   void *free_tx[MAX_FREE_TX];
   unsigned free_tx_idx, free_tx_count;
 
-  // In order to reduce cacheline contention on global_tid during
-  // beginTransaction, we allocate a block of 2**N ids to the thread
-  // all at once.  This number is the next value to be allocated from
-  // the block, or 0 % 2**N if no such block is allocated.
-  _ITM_transactionId_t local_tid;
-
   // The value returned by _ITM_getThreadnum to identify this thread.
   // ??? At present, this is densely allocated beginning with 1 and
   // we don't bother filling in this value until it is requested.
diff --git a/libitm/libitm_i.h b/libitm/libitm_i.h
index 97f535a..7d475be 100644
--- a/libitm/libitm_i.h
+++ b/libitm/libitm_i.h
@@ -168,6 +168,12 @@  struct gtm_transaction
   // A bitmask of the above.
   uint32_t state;
 
+  // In order to reduce cacheline contention on global_tid during
+  // beginTransaction, we allocate a block of 2**N ids to the thread
+  // all at once.  This number is the next value to be allocated from
+  // the block, or 0 % 2**N if no such block is allocated.
+  _ITM_transactionId_t local_tid;
+
   // Data used by eh_cpp.c for managing exceptions within the transaction.
   uint32_t cxa_catch_count;
   void *cxa_unthrown;