[pypy-commit] pypy ec-keepalive: - fix the 'ready = 0', which should be after memset(), otherwise it

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: ec-keepalive
Changeset: r81565:c9440e002f09
Date: 2016-01-05 01:58 +0100
http://bitbucket.org/pypy/pypy/changeset/c9440e002f09/

Log:	- fix the 'ready = 0', which should be after memset(), otherwise it
	is overridden

	- fix a rare case of concurrent changes to the doubly-linked list by
	adding a lock

diff --git a/rpython/rlib/rthread.py b/rpython/rlib/rthread.py
--- a/rpython/rlib/rthread.py
+++ b/rpython/rlib/rthread.py
@@ -394,11 +394,13 @@
 
         def _trace_tlref(gc, obj, callback, arg):
             p = llmemory.NULL
+            llop.threadlocalref_acquire(lltype.Void)
             while True:
                 p = llop.threadlocalref_enum(llmemory.Address, p)
                 if not p:
                     break
                 gc._trace_callback(callback, arg, p + offset)
+            llop.threadlocalref_release(lltype.Void)
         _lambda_trace_tlref = lambda: _trace_tlref
         TRACETLREF = lltype.GcStruct('TRACETLREF')
         _tracetlref_obj = lltype.malloc(TRACETLREF, immortal=True)
diff --git a/rpython/rtyper/llinterp.py b/rpython/rtyper/llinterp.py
--- a/rpython/rtyper/llinterp.py
+++ b/rpython/rtyper/llinterp.py
@@ -950,6 +950,10 @@
         return self.op_raw_load(RESTYPE, _address_of_thread_local(), offset)
     op_threadlocalref_get.need_result_type = True
 
+    def op_threadlocalref_acquire(self, prev):
+        raise NotImplementedError
+    def op_threadlocalref_release(self, prev):
+        raise NotImplementedError
     def op_threadlocalref_enum(self, prev):
         raise NotImplementedError
 
diff --git a/rpython/rtyper/lltypesystem/lloperation.py b/rpython/rtyper/lltypesystem/lloperation.py
--- a/rpython/rtyper/lltypesystem/lloperation.py
+++ b/rpython/rtyper/lltypesystem/lloperation.py
@@ -547,6 +547,8 @@
 
     'threadlocalref_addr':  LLOp(),                   # get (or make) addr of tl
     'threadlocalref_get':   LLOp(sideeffects=False),  # read field (no check)
+    'threadlocalref_acquire':  LLOp(),                # lock for enum
+    'threadlocalref_release':  LLOp(),                # lock for enum
     'threadlocalref_enum':  LLOp(sideeffects=False),  # enum all threadlocalrefs
 
     # __________ debugging __________
diff --git a/rpython/translator/c/src/threadlocal.c b/rpython/translator/c/src/threadlocal.c
--- a/rpython/translator/c/src/threadlocal.c
+++ b/rpython/translator/c/src/threadlocal.c
@@ -6,6 +6,20 @@
 #include "src/threadlocal.h"
 
 
+/* this is a spin-lock that must be acquired around each doubly-linked-list
+   manipulation (because such manipulations can occur without the GIL) */
+static long pypy_threadlocal_lock = 0;
+
+void _RPython_ThreadLocals_Acquire(void) {
+    while (!lock_test_and_set(&pypy_threadlocal_lock, 1)) {
+        /* busy loop */
+    }
+}
+void _RPython_ThreadLocals_Release(void) {
+    lock_release(&pypy_threadlocal_lock);
+}
+
+
 pthread_key_t pypy_threadlocal_key
 #ifdef _WIN32
 = TLS_OUT_OF_INDEXES
@@ -48,23 +62,29 @@
                   where it is not the case are rather old nowadays. */
 #    endif
 #endif
+    _RPython_ThreadLocals_Acquire();
     oldnext = linkedlist_head.next;
     tls->prev = &linkedlist_head;
     tls->next = oldnext;
     linkedlist_head.next = tls;
     oldnext->prev = tls;
     tls->ready = 42;
+    _RPython_ThreadLocals_Release();
 }
 
 static void threadloc_unlink(void *p)
 {
+    /* warning: this can be called at completely random times without
+       the GIL. */
     struct pypy_threadlocal_s *tls = (struct pypy_threadlocal_s *)p;
+    _RPython_ThreadLocals_Acquire();
     if (tls->ready == 42) {
-        tls->ready = 0;
         tls->next->prev = tls->prev;
         tls->prev->next = tls->next;
         memset(tls, 0xDD, sizeof(struct pypy_threadlocal_s));  /* debug */
+        tls->ready = 0;
     }
+    _RPython_ThreadLocals_Release();
 #ifndef USE___THREAD
     free(p);
 #endif
@@ -110,6 +130,7 @@
        a non-null thread-local value).  This is needed even in the
        case where we use '__thread' below, for the destructor.
     */
+    assert(pypy_threadlocal_lock == 0);
 #ifdef _WIN32
     pypy_threadlocal_key = TlsAlloc();
     if (pypy_threadlocal_key == TLS_OUT_OF_INDEXES)
@@ -122,6 +143,12 @@
         abort();
     }
     _RPython_ThreadLocals_Build();
+
+#ifndef _WIN32
+    pthread_atfork(_RPython_ThreadLocals_Acquire,
+                   _RPython_ThreadLocals_Release,
+                   _RPython_ThreadLocals_Release);
+#endif
 }
 
 
@@ -136,7 +163,7 @@
 
 char *_RPython_ThreadLocals_Build(void)
 {
-    RPyAssert(pypy_threadlocal.ready == 0, "corrupted thread-local");
+    RPyAssert(pypy_threadlocal.ready == 0, "unclean thread-local");
     _RPy_ThreadLocals_Init(&pypy_threadlocal);
 
     /* we also set up &pypy_threadlocal as a POSIX thread-local variable,
diff --git a/rpython/translator/c/src/threadlocal.h b/rpython/translator/c/src/threadlocal.h
--- a/rpython/translator/c/src/threadlocal.h
+++ b/rpython/translator/c/src/threadlocal.h
@@ -19,11 +19,17 @@
    current thread, and if not, calls the following helper. */
 RPY_EXTERN char *_RPython_ThreadLocals_Build(void);
 
+RPY_EXTERN void _RPython_ThreadLocals_Acquire(void);
+RPY_EXTERN void _RPython_ThreadLocals_Release(void);
+
+/* Must acquire/release the thread-local lock around a series of calls
+   to the following function */
 RPY_EXTERN struct pypy_threadlocal_s *
 _RPython_ThreadLocals_Enum(struct pypy_threadlocal_s *prev);
 
-#define OP_THREADLOCALREF_ENUM(p, r)            \
-    r = _RPython_ThreadLocals_Enum(p)
+#define OP_THREADLOCALREF_ACQUIRE(r)   _RPython_ThreadLocals_Acquire()
+#define OP_THREADLOCALREF_RELEASE(r)   _RPython_ThreadLocals_Release()
+#define OP_THREADLOCALREF_ENUM(p, r)   r = _RPython_ThreadLocals_Enum(p)
 
 
 /* ------------------------------------------------------------ */