diff mbox

[trans-mem] Release transaction objects on thread termination

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

Commit Message

Torvald Riegel Aug. 3, 2011, 11:16 a.m. UTC
This patch removes the per-thread free-list of gtm_transaction objects.
With a single stable gtm_transaction object per thread, it is not
necessary anymore. Furthermore, a destructor function of a pthread TLS
key is used to release a thread's gtm_transaction object when this
thread terminates. This works even when transactions are used in other
TLS key's destructors because the key is initialized whenever a
transaction is used for the first time in a thread (or after a previous
destruction in this thread).
The test for the release doesn't really check that the release happens
(and can't because we do black-box testing), but at least it will
trigger the release twice.

OK for branch?
commit 77fd947e31f5c51c01e3fb833cd1a5e9379c2dc9
Author: Torvald Riegel <triegel@redhat.com>
Date:   Fri Jul 29 20:37:30 2011 +0200

    Release transaction data on thread termination.
    
    	* config/generic/tls.h: Remove the free list for transactions and ...
    	* beginend.cc (GTM::gtm_transaction::operator new): ... allocate ...
    	(GTM::gtm_transaction::operator delete): ... and release here.
    	(thread_exit_handler): New. Delete tx when thread terminates.
    	(thread_exit_init): New.
    	(GTM::gtm_transaction::begin_transaction): Set up on-exit handler.
    	* testsuite/libitm.c/txrelease.c: New file.

Comments

Richard Henderson Aug. 4, 2011, 3:59 p.m. UTC | #1
On 08/03/2011 04:16 AM, Torvald Riegel wrote:
> This patch removes the per-thread free-list of gtm_transaction objects.
> With a single stable gtm_transaction object per thread, it is not
> necessary anymore.

If this is true, just move the entire gtm_transaction object
into gtm_thread and have it be statically allocated with TLS.
No need for any of the other gyrations.


r~
diff mbox

Patch

diff --git a/libitm/beginend.cc b/libitm/beginend.cc
index a16c3f9..08592a3 100644
--- a/libitm/beginend.cc
+++ b/libitm/beginend.cc
@@ -38,52 +38,58 @@  uint64_t GTM::gtm_spin_count_var = 1000;
 
 static _ITM_transactionId_t global_tid;
 
-/* Allocate a transaction structure.  Reuse an old one if possible.  */
+// Provides a on-thread-exit callback used to release per-thread data.
+static pthread_key_t tx_release_key;
+static pthread_once_t tx_release_once = PTHREAD_ONCE_INIT;
+
+
+/* Allocate a transaction structure.  */
 
 void *
 GTM::gtm_transaction::operator new (size_t s)
 {
-  gtm_thread *thr = setup_gtm_thr ();
   void *tx;
 
   assert(s == sizeof(gtm_transaction));
 
-  if (thr->free_tx_count == 0)
-    tx = xmalloc (sizeof (gtm_transaction));
-  else
-    {
-      thr->free_tx_count--;
-      tx = thr->free_tx[thr->free_tx_idx];
-      thr->free_tx_idx = (thr->free_tx_idx + 1) % gtm_thread::MAX_FREE_TX;
-    }
+  tx = xmalloc (sizeof (gtm_transaction), true);
   memset (tx, 0, sizeof (gtm_transaction));
 
   return tx;
 }
 
-/* Queue a transaction structure for freeing.  We never free the given
-   transaction immediately -- this is a requirement of abortTransaction
-   as the jmpbuf is used immediately after calling this function.  Thus
-   the requirement that this queue be per-thread.  */
+/* Free the given transaction. Raises an error if the transaction is still
+   in use.  */
 
 void
 GTM::gtm_transaction::operator delete(void *tx)
 {
-  gtm_thread *thr = gtm_thr ();
-  unsigned idx
-    = (thr->free_tx_idx + thr->free_tx_count) % gtm_thread::MAX_FREE_TX;
+  free(tx);
+}
 
-  if (thr->free_tx_count == gtm_thread::MAX_FREE_TX)
+static void
+thread_exit_handler(void *dummy __attribute__((unused)))
+{
+  gtm_transaction *tx = gtm_tx();
+  if (tx)
     {
-      thr->free_tx_idx = (thr->free_tx_idx + 1) % gtm_thread::MAX_FREE_TX;
-      free (thr->free_tx[idx]);
+      if (tx->nesting > 0)
+        GTM_fatal("Thread exit while a transaction is still active.");
+      delete tx;
+      set_gtm_tx(NULL);
     }
-  else
-    thr->free_tx_count++;
+  if (pthread_setspecific(tx_release_key, NULL))
+    GTM_fatal("Setting tx release TLS key failed.");
+}
 
-  thr->free_tx[idx] = tx;
+static void
+thread_exit_init()
+{
+  if (pthread_key_create(&tx_release_key, thread_exit_handler))
+    GTM_fatal("Creating tx release TLS key failed.");
 }
 
+
 #ifndef HAVE_64BIT_SYNC_BUILTINS
 static pthread_mutex_t global_tid_lock = PTHREAD_MUTEX_INITIALIZER;
 #endif
@@ -113,10 +119,17 @@  GTM::gtm_transaction::begin_transaction (uint32_t prop, const gtm_jmpbuf *jb)
     GTM_fatal("pr_undoLogCode not supported");
 
   tx = gtm_tx();
-  if (tx == NULL)
+  if (unlikely(tx == NULL))
     {
       tx = new gtm_transaction;
       set_gtm_tx(tx);
+
+      if (pthread_once(&tx_release_once, thread_exit_init))
+        GTM_fatal("Initializing tx release TLS key failed.");
+      // Any non-null value is sufficient to trigger releasing of this
+      // transaction when the current thread terminates.
+      if (pthread_setspecific(tx_release_key, tx))
+        GTM_fatal("Setting tx release TLS key failed.");
     }
 
   if (tx->nesting > 0)
diff --git a/libitm/config/generic/tls.h b/libitm/config/generic/tls.h
index 8a39b80..3eea204 100644
--- a/libitm/config/generic/tls.h
+++ b/libitm/config/generic/tls.h
@@ -1,4 +1,4 @@ 
-/* Copyright (C) 2008, 2009 Free Software Foundation, Inc.
+/* Copyright (C) 2008, 2009, 2011 Free Software Foundation, Inc.
    Contributed by Richard Henderson <rth@redhat.com>.
 
    This file is part of the GNU Transactional Memory Library (libitm).
@@ -41,15 +41,6 @@  struct gtm_thread
   abi_dispatch *disp;
 #endif
 
-  // The maximum number of free gtm_transaction structs to be kept.
-  // This number must be greater than 1 in order for transaction abort
-  // to be handled properly.
-  static const unsigned MAX_FREE_TX = 8;
-
-  // A queue of free gtm_transaction structs.
-  void *free_tx[MAX_FREE_TX];
-  unsigned free_tx_idx, free_tx_count;
-
   // 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/testsuite/libitm.c/txrelease.c b/libitm/testsuite/libitm.c/txrelease.c
new file mode 100644
index 0000000..8d8b697
--- /dev/null
+++ b/libitm/testsuite/libitm.c/txrelease.c
@@ -0,0 +1,47 @@ 
+/* This test triggers execution of the code that releases per-thread
+   transaction data when a thread exists, potentially repeatedly. However,
+   we currently cannot check whether the data has indeed been released.  */
+
+#include <stddef.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+static int round = 0;
+static pthread_key_t key;
+
+static void
+thread_exit_handler(void *dummy __attribute__((unused)))
+{
+  if (round == 0)
+    abort();
+  if (round == 1)
+    {
+      // ??? It would be good if we could check here that the transaction has
+      // indeed been released.
+      __transaction { round++; }
+      if (pthread_setspecific(key, &round))
+        abort();
+    }
+  // ??? It would be good if we could check here that the transaction has
+  // indeed been released (again).
+}
+
+static void *thread (void *dummy __attribute__((unused)))
+{
+  if (pthread_key_create(&key, thread_exit_handler))
+    abort();
+  if (pthread_setspecific(key, &round))
+    abort();
+  __transaction { round++; }
+  return NULL;
+}
+
+int main()
+{
+  pthread_t pt;
+  pthread_create(&pt, NULL, thread, NULL);
+  pthread_join(pt, NULL);
+  if (round != 2)
+    abort();
+  return 0;
+}