Patchwork libitm: Fix privatization safety during upgrades to serial mode.

login
register
mail settings
Submitter Torvald Riegel
Date Dec. 22, 2011, 7:28 p.m.
Message ID <1324582129.4159.16748.camel@triegel.csb>
Download mbox | patch
Permalink /patch/132886/
State New
Headers show

Comments

Torvald Riegel - Dec. 22, 2011, 7:28 p.m.
During working on the atomics-in-libitm patch, I noticed that during
upgrades to serial mode, we were clearing the reader flag of the
upgrading transaction too early.  This would be fine for just the serial
lock but we also use this flag in the quiescence-based privatization
safety implementation; clearing the flag early would expose the
upgrading transaction's redo writes during commit or the undo writes
during rollback to nontransactional code running after the other
privatizing transaction (which in turn can lead to race conditions).
The attached patch fixes that by deferring unsetting the reader flag
until commit or rollback of the upgrader have finished.

OK for trunk?
commit c2a7ff4cc87c5bc19e88a30a454755a6725bd746
Author: Torvald Riegel <triegel@redhat.com>
Date:   Thu Dec 22 20:11:37 2011 +0100

    libitm: Fix privatization safety during upgrades to serial mode.
    
    	libitm/
    	* beginend.cc (GTM::gtm_thread::restart): Add and handle
    	finish_serial_upgrade parameter.
    	* libitm.h (GTM::gtm_thread::restart): Adapt declaration.
    	* config/linux/rwlock.cc (GTM::gtm_rwlock::write_lock_generic):
    	Don't unset reader flag.
    	(GTM::gtm_rwlock::write_upgrade_finish): New.
    	* config/posix/rwlock.cc: Same.
    	* config/linux/rwlock.h (GTM::gtm_rwlock::write_upgrade_finish):
    	Declare.
    	* config/posix/rwlock.h: Same.
    	* method-serial.cc (GTM::gtm_thread::serialirr_mode): Unset reader
    	flag after commit or after rollback when restarting.
Richard Henderson - Dec. 23, 2011, 9:54 p.m.
On 12/22/2011 11:28 AM, Torvald Riegel wrote:
>     libitm: Fix privatization safety during upgrades to serial mode.
>     
>     	libitm/
>     	* beginend.cc (GTM::gtm_thread::restart): Add and handle
>     	finish_serial_upgrade parameter.
>     	* libitm.h (GTM::gtm_thread::restart): Adapt declaration.
>     	* config/linux/rwlock.cc (GTM::gtm_rwlock::write_lock_generic):
>     	Don't unset reader flag.
>     	(GTM::gtm_rwlock::write_upgrade_finish): New.
>     	* config/posix/rwlock.cc: Same.
>     	* config/linux/rwlock.h (GTM::gtm_rwlock::write_upgrade_finish):
>     	Declare.
>     	* config/posix/rwlock.h: Same.
>     	* method-serial.cc (GTM::gtm_thread::serialirr_mode): Unset reader
>     	flag after commit or after rollback when restarting.

Ok.



r~

Patch

diff --git a/libitm/beginend.cc b/libitm/beginend.cc
index d0ad5a7..17f9d74 100644
--- a/libitm/beginend.cc
+++ b/libitm/beginend.cc
@@ -511,11 +511,19 @@  GTM::gtm_thread::trycommit ()
 }
 
 void ITM_NORETURN
-GTM::gtm_thread::restart (gtm_restart_reason r)
+GTM::gtm_thread::restart (gtm_restart_reason r, bool finish_serial_upgrade)
 {
   // Roll back to outermost transaction. Do not reset transaction state because
   // we will continue executing this transaction.
   rollback ();
+
+  // If we have to restart while an upgrade of the serial lock is happening,
+  // we need to finish this here, after rollback (to ensure privatization
+  // safety despite undo writes) and before deciding about the retry strategy
+  // (which could switch to/from serial mode).
+  if (finish_serial_upgrade)
+    gtm_thread::serial_lock.write_upgrade_finish(this);
+
   decide_retry_strategy (r);
 
   // Run dispatch-specific restart code. Retry until we succeed.
diff --git a/libitm/config/linux/rwlock.cc b/libitm/config/linux/rwlock.cc
index f87be2e..24e7042 100644
--- a/libitm/config/linux/rwlock.cc
+++ b/libitm/config/linux/rwlock.cc
@@ -121,17 +121,13 @@  gtm_rwlock::write_lock_generic (gtm_thread *tx)
   // readers that might still be active.
   // We don't need an extra barrier here because the CAS and the xchg
   // operations have full barrier semantics already.
-
-  // If this is an upgrade, we are not a reader anymore. This is only safe to
-  // do after we have acquired the writer lock.
   // TODO In the worst case, this requires one wait/wake pair for each
   // active reader. Reduce this!
-  if (tx != 0)
-    tx->shared_state.store (-1, memory_order_relaxed);
-
   for (gtm_thread *it = gtm_thread::list_of_threads; it != 0;
       it = it->next_thread)
     {
+      if (it == tx)
+        continue;
       // Use a loop here to check reader flags again after waiting.
       while (it->shared_state.load (memory_order_relaxed)
           != ~(typeof it->shared_state)0)
@@ -175,6 +171,18 @@  gtm_rwlock::write_upgrade (gtm_thread *tx)
 }
 
 
+// Has to be called iff the previous upgrade was successful and after it is
+// safe for the transaction to not be marked as a reader anymore.
+
+void
+gtm_rwlock::write_upgrade_finish (gtm_thread *tx)
+{
+  // We are not a reader anymore.  This is only safe to do after we have
+  // acquired the writer lock.
+  tx->shared_state.store (-1, memory_order_release);
+}
+
+
 // Release a RW lock from reading.
 
 void
diff --git a/libitm/config/linux/rwlock.h b/libitm/config/linux/rwlock.h
index e5a53c0..987e580 100644
--- a/libitm/config/linux/rwlock.h
+++ b/libitm/config/linux/rwlock.h
@@ -57,6 +57,7 @@  class gtm_rwlock
   void write_unlock ();
 
   bool write_upgrade (gtm_thread *tx);
+  void write_upgrade_finish (gtm_thread *tx);
 
  protected:
   bool write_lock_generic (gtm_thread *tx);
diff --git a/libitm/config/posix/rwlock.cc b/libitm/config/posix/rwlock.cc
index 7ef3998..f93baf2 100644
--- a/libitm/config/posix/rwlock.cc
+++ b/libitm/config/posix/rwlock.cc
@@ -155,10 +155,6 @@  gtm_rwlock::write_lock_generic (gtm_thread *tx)
   // path of read_lock()).
   atomic_thread_fence(memory_order_seq_cst);
 
-  // If this is an upgrade, we are not a reader anymore.
-  if (tx != 0)
-    tx->shared_state.store(-1, memory_order_relaxed);
-
   // Count the number of active readers to be able to decrease the number of
   // wake-ups and wait calls that are necessary.
   //
@@ -194,6 +190,8 @@  gtm_rwlock::write_lock_generic (gtm_thread *tx)
 	  it = it->next_thread)
 	{
 	  // Don't count ourself if this is an upgrade.
+          if (it == tx)
+            continue;
 	  if (it->shared_state.load(memory_order_relaxed) != (gtm_word)-1)
 	    readers++;
 	}
@@ -231,6 +229,18 @@  gtm_rwlock::write_upgrade (gtm_thread *tx)
 }
 
 
+// Has to be called iff the previous upgrade was successful and after it is
+// safe for the transaction to not be marked as a reader anymore.
+
+void
+gtm_rwlock::write_upgrade_finish (gtm_thread *tx)
+{
+  // We are not a reader anymore.  This is only safe to do after we have
+  // acquired the writer lock.
+  tx->shared_state.store (-1, memory_order_release);
+}
+
+
 // Release a RW lock from reading.
 
 void
diff --git a/libitm/config/posix/rwlock.h b/libitm/config/posix/rwlock.h
index 359991a..a1a6042 100644
--- a/libitm/config/posix/rwlock.h
+++ b/libitm/config/posix/rwlock.h
@@ -72,6 +72,7 @@  class gtm_rwlock
   void write_unlock ();
 
   bool write_upgrade (gtm_thread *tx);
+  void write_upgrade_finish (gtm_thread *tx);
 
  protected:
   bool write_lock_generic (gtm_thread *tx);
diff --git a/libitm/libitm_i.h b/libitm/libitm_i.h
index d57872e..b53792a 100644
--- a/libitm/libitm_i.h
+++ b/libitm/libitm_i.h
@@ -231,7 +231,8 @@  struct gtm_thread
   // In beginend.cc
   void rollback (gtm_transaction_cp *cp = 0, bool aborting = false);
   bool trycommit ();
-  void restart (gtm_restart_reason) ITM_NORETURN;
+  void restart (gtm_restart_reason, bool finish_serial_upgrade = false)
+        ITM_NORETURN;
 
   gtm_thread();
   ~gtm_thread();
diff --git a/libitm/method-serial.cc b/libitm/method-serial.cc
index 5e85653..bf79826 100644
--- a/libitm/method-serial.cc
+++ b/libitm/method-serial.cc
@@ -239,7 +239,6 @@  void
 GTM::gtm_thread::serialirr_mode ()
 {
   struct abi_dispatch *disp = abi_disp ();
-  bool need_restart = true;
 
   if (this->state & STATE_SERIAL)
     {
@@ -254,7 +253,6 @@  GTM::gtm_thread::serialirr_mode ()
       bool ok = disp->trycommit (priv_time);
       // Given that we're already serial, the trycommit better work.
       assert (ok);
-      need_restart = false;
     }
   else if (serial_lock.write_upgrade (this))
     {
@@ -263,18 +261,18 @@  GTM::gtm_thread::serialirr_mode ()
       // would do for an outermost commit.
       // We have successfully upgraded to serial mode, so we don't need to
       // ensure privatization safety for other transactions here.
+      // However, we are still a reader (wrt. privatization safety) until we
+      // have either committed or restarted, so finish the upgrade after that.
       gtm_word priv_time = 0;
-      if (disp->trycommit (priv_time))
-	need_restart = false;
+      if (!disp->trycommit (priv_time))
+        restart (RESTART_SERIAL_IRR, true);
+      gtm_thread::serial_lock.write_upgrade_finish(this);
     }
-
-  if (need_restart)
-    restart (RESTART_SERIAL_IRR);
   else
-    {
-      this->state |= (STATE_SERIAL | STATE_IRREVOCABLE);
-      set_abi_disp (dispatch_serialirr ());
-    }
+    restart (RESTART_SERIAL_IRR, false);
+
+  this->state |= (STATE_SERIAL | STATE_IRREVOCABLE);
+  set_abi_disp (dispatch_serialirr ());
 }
 
 void ITM_REGPARM