[pypy-commit] stmgc default: Issue fix: if thread A is in stm_start_transaction(), it may call

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: 
Changeset: r1738:5da5ba86b2fa
Date: 2015-03-21 17:07 +0100
http://bitbucket.org/pypy/stmgc/changeset/5da5ba86b2fa/

Log:	Issue fix: if thread A is in stm_start_transaction(), it may call
	enter_safe_point_if_requested(), and thread B might then run
	stm_commit_transaction(). If doing so, the thread B notion of "what
	objects have the read marker in thread A" is wrong.

diff --git a/c7/stm/atomic.h b/c7/stm/atomic.h
--- a/c7/stm/atomic.h
+++ b/c7/stm/atomic.h
@@ -27,11 +27,13 @@
 # define HAVE_FULL_EXCHANGE_INSN
   static inline void spin_loop(void) { asm("pause" : : : "memory"); }
   static inline void write_fence(void) { asm("" : : : "memory"); }
+  static inline void read_fence(void) { asm("" : : : "memory"); }
 
 #else
 
   static inline void spin_loop(void) { asm("" : : : "memory"); }
   static inline void write_fence(void) { __sync_synchronize(); }
+  static inline void read_fence(void) { asm("" : : : "memory"); }
 
 #endif
 
diff --git a/c7/stm/core.c b/c7/stm/core.c
--- a/c7/stm/core.c
+++ b/c7/stm/core.c
@@ -323,6 +323,7 @@
         /* failed */
         stm_fatalerror("reset_transaction_read_version: %m");
     }
+    write_fence();
     STM_SEGMENT->transaction_read_version = 1;
 }
 
@@ -350,20 +351,22 @@
     STM_PSEGMENT->shadowstack_at_start_of_transaction = tl->shadowstack;
     STM_PSEGMENT->threadlocal_at_start_of_transaction = tl->thread_local_obj;
 
+    uint8_t rv = STM_SEGMENT->transaction_read_version + 1;
+    STM_SEGMENT->transaction_read_version = rv;
+
     enter_safe_point_if_requested();
     dprintf(("start_transaction\n"));
 
     s_mutex_unlock();
 
-    /* Now running the SP_RUNNING start.  We can set our
-       'transaction_read_version' after releasing the mutex,
-       because it is only read by a concurrent thread in
-       stm_commit_transaction(), which waits until SP_RUNNING
-       threads are paused.
+    /* Now running the SP_RUNNING start.  We can reset the
+       transaction read version here, but we need to increment it
+       before, otherwise we can end up in enter_safe_point_if_requested(),
+       and another thread runs stm_commit_transaction(), and it may find
+       that we have read some object (even though we didn't do any read
+       so far).
     */
-    uint8_t old_rv = STM_SEGMENT->transaction_read_version;
-    STM_SEGMENT->transaction_read_version = old_rv + 1;
-    if (UNLIKELY(old_rv == 0xff)) {
+    if (UNLIKELY(rv == 0xff)) {
         reset_transaction_read_version();
     }
 
diff --git a/c7/stm/core.h b/c7/stm/core.h
--- a/c7/stm/core.h
+++ b/c7/stm/core.h
@@ -295,6 +295,7 @@
     uint8_t other_transaction_read_version =
         ((struct stm_segment_info_s *)REAL_ADDRESS(base, STM_PSEGMENT))
             ->transaction_read_version;
+    read_fence();
     uint8_t rm = ((struct stm_read_marker_s *)
                   (base + (((uintptr_t)obj) >> 4)))->rm;
     assert(rm <= other_transaction_read_version);