[pypy-commit] stmgc default: Subtle race condition

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: 
Changeset: r1611:005668d99755
Date: 2015-02-08 22:15 +0100
http://bitbucket.org/pypy/stmgc/changeset/005668d99755/

Log:	Subtle race condition

diff --git a/c7/stm/core.c b/c7/stm/core.c
--- a/c7/stm/core.c
+++ b/c7/stm/core.c
@@ -312,6 +312,11 @@
                                      FIRST_READMARKER_PAGE * 4096UL);
     dprintf(("reset_transaction_read_version: %p %ld\n", readmarkers,
              (long)(NB_READMARKER_PAGES * 4096UL)));
+
+    /* see hashtable.c for why we need the privatization lock here
+       (grep for reset_transaction_read_version)
+    */
+    acquire_privatization_lock();
     if (mmap(readmarkers, NB_READMARKER_PAGES * 4096UL,
              PROT_READ | PROT_WRITE,
              MAP_FIXED | MAP_PAGES_FLAGS, -1, 0) != readmarkers) {
@@ -322,6 +327,7 @@
         memset(readmarkers, 0, NB_READMARKER_PAGES * 4096UL);
     }
     STM_SEGMENT->transaction_read_version = 1;
+    release_privatization_lock();
 }
 
 static uint64_t _global_start_time = 0;
diff --git a/c7/stm/hashtable.c b/c7/stm/hashtable.c
--- a/c7/stm/hashtable.c
+++ b/c7/stm/hashtable.c
@@ -299,16 +299,28 @@
             */
 
             /* First fetch the read marker of 'hashtableobj' in all
-               segments, before allocate_outside_nursery_large()
-               which might trigger a GC */
+               segments, before allocate_outside_nursery_large() which
+               might trigger a GC.  Synchronization guarantee: if
+               stm_read(hobj) in stm_hashtable_list() has set the read
+               marker, then it did synchronize with us here by
+               acquiring and releasing this hashtable' lock.  However,
+               the interval of time between reading the readmarkers of
+               hobj and copying them to the new entry object might be
+               enough for the other threads to do anything, including
+               a reset_transaction_read_version(), so that we might in
+               theory write bogus read markers that are not valid any
+               more.  To prevent this, reset_transaction_read_version()
+               acquires the privatization_lock too.
+            */
             long j;
             uint8_t readmarkers[NB_SEGMENTS];
+
+            acquire_privatization_lock();
             for (j = 1; j <= NB_SEGMENTS; j++) {
                 readmarkers[j - 1] = get_read_marker(get_segment_base(j),
                                                      hashtableobj)->rm;
             }
 
-            acquire_privatization_lock();
             char *p = allocate_outside_nursery_large(
                           sizeof(stm_hashtable_entry_t));
             entry = (stm_hashtable_entry_t *)(p - stm_object_pages);
@@ -323,12 +335,12 @@
                 e->object = NULL;
             }
             hashtable->additions += 0x100;
-            release_privatization_lock();
 
             for (j = 1; j <= NB_SEGMENTS; j++) {
                 get_read_marker(get_segment_base(j), (object_t *)entry)->rm =
                     readmarkers[j - 1];
             }
+            release_privatization_lock();
         }
         write_fence();     /* make sure 'entry' is fully initialized here */
         table->items[i] = entry;