This patch fixes a race condition in how transactions previously chose
the dispatch at transaction begin: default_dispatch in retry.cc was
read by transaction before they became either serial or nonserial
transactions (with the serial_lock). A concurrent change of
default_dispatch was possible to lead to a transaction starting with
some out-of-date dispatch that were potentially incompatible with the
actual current dispatch, leading in turn to all sorts of synchronization
failures.
This is fixed by this patch by moving the serial_lock acquisiton into
the dispatch choice code.
Tested on ppc64 with microbenchmarks. OK for trunk?
commit ce52924dedca632b24ea931329e060959782f89a
Author: Torvald Riegel <trie...@redhat.com>
Date: Mon Feb 13 23:49:55 2012 +0100
libitm: Fix race condition in dispatch choice at transaction begin.
libitm/
* beginend.cc (GTM::gtm_thread::begin_transaction): Move serial lock
acquisition to ...
* retry.cc (GTM::gtm_thread::decide_begin_dispatch): ... here.
(default_dispatch): Make atomic.
(GTM::gtm_thread::decide_retry_strategy,
GTM::gtm_thread::set_default_dispatch): Access atomically.
(GTM::gtm_thread::number_of_threads_changed): Initialize
default_dispatch here.
diff --git a/libitm/beginend.cc b/libitm/beginend.cc
index 08c2174..e6a84de 100644
--- a/libitm/beginend.cc
+++ b/libitm/beginend.cc
@@ -233,16 +233,6 @@ GTM::gtm_thread::begin_transaction (uint32_t prop, const
gtm_jmpbuf *jb)
{
// Outermost transaction
disp = tx->decide_begin_dispatch (prop);
- if (disp == dispatch_serialirr() || disp == dispatch_serial())
- {
- tx->state = STATE_SERIAL;
- if (disp == dispatch_serialirr())
- tx->state |= STATE_IRREVOCABLE;
- serial_lock.write_lock ();
- }
- else
- serial_lock.read_lock (tx);
-
set_abi_disp (disp);
}
diff --git a/libitm/retry.cc b/libitm/retry.cc
index d57bba0..08c5d80 100644
--- a/libitm/retry.cc
+++ b/libitm/retry.cc
@@ -27,8 +27,9 @@
#include <ctype.h>
#include "libitm_i.h"
-// The default TM method used when starting a new transaction.
-static GTM::abi_dispatch* default_dispatch = 0;
+// The default TM method used when starting a new transaction. Initialized
+// in number_of_threads_changed() below.
+static std::atomic<GTM::abi_dispatch*> default_dispatch;
// The default TM method as requested by the user, if any.
static GTM::abi_dispatch* default_dispatch_user = 0;
@@ -57,14 +58,18 @@ GTM::gtm_thread::decide_retry_strategy (gtm_restart_reason
r)
// given that re-inits should be very infrequent.
serial_lock.read_unlock(this);
serial_lock.write_lock();
- if (disp->get_method_group() == default_dispatch->get_method_group())
+ if (disp->get_method_group()
+ == default_dispatch.load(memory_order_relaxed)
+ ->get_method_group())
// Still the same method group.
disp->get_method_group()->reinit();
serial_lock.write_unlock();
serial_lock.read_lock(this);
- if (disp->get_method_group() != default_dispatch->get_method_group())
+ if (disp->get_method_group()
+ != default_dispatch.load(memory_order_relaxed)
+ ->get_method_group())
{
- disp = default_dispatch;
+ disp = default_dispatch.load(memory_order_relaxed);
set_abi_disp(disp);
}
}
@@ -124,48 +129,81 @@ GTM::gtm_thread::decide_retry_strategy
(gtm_restart_reason r)
// Decides which TM method should be used on the first attempt to run this
-// transaction.
+// transaction. Acquires the serial lock and sets transaction state
+// according to the chosen TM method.
GTM::abi_dispatch*
GTM::gtm_thread::decide_begin_dispatch (uint32_t prop)
{
+ abi_dispatch* dd;
// TODO Pay more attention to prop flags (eg, *omitted) when selecting
// dispatch.
+ // ??? We go irrevocable eagerly here, which is not always good for
+ // performance. Don't do this?
if ((prop & pr_doesGoIrrevocable) || !(prop & pr_instrumentedCode))
- return dispatch_serialirr();
+ dd = dispatch_serialirr();
- // If we might need closed nesting and the default dispatch has an
- // alternative that supports closed nesting, use it.
- // ??? We could choose another TM method that we know supports closed
- // nesting but isn't the default (e.g., dispatch_serial()). However, we
- // assume that aborts that need closed nesting are infrequent, so don't
- // choose a non-default method until we have to actually restart the
- // transaction.
- if (!(prop & pr_hasNoAbort) && !default_dispatch->closed_nesting()
- && default_dispatch->closed_nesting_alternative())
- return default_dispatch->closed_nesting_alternative();
+ else
+ {
+ // Load the default dispatch. We're not an active transaction and so it
+ // can change concurrently but will still be some valid dispatch.
+ abi_dispatch* dd_orig = default_dispatch.load(memory_order_relaxed);
+ dd = dd_orig;
+
+ // If we might need closed nesting and the default dispatch has an
+ // alternative that supports closed nesting, use it.
+ // ??? We could choose another TM method that we know supports closed
+ // nesting but isn't the default (e.g., dispatch_serial()). However, we
+ // assume that aborts that need closed nesting are infrequent, so don't
+ // choose a non-default method until we have to actually restart the
+ // transaction.
+ if (!(prop & pr_hasNoAbort) && !dd->closed_nesting()
+ && dd->closed_nesting_alternative())
+ dd = dd->closed_nesting_alternative();
+
+ if (dd != dispatch_serial() && dd != dispatch_serialirr())
+ {
+ // The current dispatch is supposedly a non-serial one. Become an
+ // active transaction and verify this.
+ serial_lock.read_lock (this);
+ if (default_dispatch.load(memory_order_relaxed) == dd_orig)
+ return dd;
- // No special case, just use the default dispatch.
- return default_dispatch;
+ // If we raced with a concurrent modification of default_dispatch,
+ // just fall back to serialirr. The dispatch choice might not be
+ // up-to-date anymore, but this is harmless.
+ serial_lock.read_unlock (this);
+ dd = dispatch_serialirr();
+ }
+ }
+
+ // We are some kind of serial transaction.
+ serial_lock.write_lock();
+ if (dd == dispatch_serialirr())
+ state = STATE_SERIAL | STATE_IRREVOCABLE;
+ else
+ state = STATE_SERIAL;
+ return dd;
}
void
GTM::gtm_thread::set_default_dispatch(GTM::abi_dispatch* disp)
{
- if (default_dispatch == disp)
+ abi_dispatch* dd = default_dispatch.load(memory_order_relaxed);
+ if (dd == disp)
return;
- if (default_dispatch)
+ if (dd)
{
// If we are switching method groups, initialize and shut down properly.
- if (default_dispatch->get_method_group() != disp->get_method_group())
+ if (dd->get_method_group() != disp->get_method_group())
{
- default_dispatch->get_method_group()->fini();
+ dd->get_method_group()->fini();
disp->get_method_group()->init();
}
}
else
disp->get_method_group()->init();
- default_dispatch = disp;
+ default_dispatch.store(disp, memory_order_relaxed);
}
@@ -233,6 +271,7 @@ GTM::gtm_thread::number_of_threads_changed(unsigned
previous, unsigned now)
{
initialized = true;
// Check for user preferences here.
+ default_dispatch = 0;
default_dispatch_user = parse_default_method();
}
}
