Changes from review

Author: colesbury

Include/cpython/pystate.h

@@ -102,7 +102,7 @@ struct _ts {
 #endif
     int _whence;
 
-    /* Thread state (_Py_THREAD_ATTACHED, _Py_THREAD_DETACHED, _Py_THREAD_GC).
+    /* Thread state (_Py_THREAD_ATTACHED, _Py_THREAD_DETACHED, _Py_THREAD_SUSPENDED).
        See Include/internal/pycore_pystate.h for more details. */
     int state;
 

Include/internal/pycore_llist.h

@@ -37,8 +37,7 @@ struct llist_node {
 };
 
 // Get the struct containing a node.
-#define llist_data(node, type, member) \
-    (type*)((char*)node - offsetof(type, member))
+#define llist_data(node, type, member) (_Py_CONTAINER_OF(node, type, member))
 
 // Iterate over a list.
 #define llist_for_each(node, head) \

Include/internal/pycore_pystate.h

@@ -19,23 +19,27 @@ extern "C" {
 // interpreter at the same time. Only the "bound" thread may perform the
 // transitions between "attached" and "detached" on its own PyThreadState.
 //
-// The "gc" state is used to implement stop-the-world pauses, such as for
-// cyclic garbage collection. It is only used in `--disable-gil` builds. It is
-// similar to the "detached" state, but only the thread performing a
-// stop-the-world pause may transition threads between the "detached" and "gc"
-// states. A thread trying to "attach" from the "gc" state will block until
-// it is transitioned back to "detached" when the stop-the-world pause is
-// complete.
+// The "suspended" state is used to implement stop-the-world pauses, such as
+// for cyclic garbage collection. It is only used in `--disable-gil` builds. It
+// is similar to the "detached" state, but only the thread performing a
+// stop-the-world pause may transition a thread from the "suspended" state back
+// to the "detached" state. A thread trying to "attach" from the "suspended"
+// state will block until it is transitioned back to "detached" when the
+// stop-the-world pause is complete.
 //
 // State transition diagram:
 //
 //            (bound thread)        (stop-the-world thread)
-// [attached]       <->       [detached]       <->       [gc]
+// [attached]       <->       [detached]       <->       [suspended]
+//   +                                                        ^
+//   +--------------------------------------------------------+
+//                          (bound thread)
 //
-// See `_PyThreadState_Attach()` and `_PyThreadState_Detach()`.
+// See `_PyThreadState_Attach()`, `_PyThreadState_Detach()`, and
+// `_PyThreadState_Suspend()`.
 #define _Py_THREAD_DETACHED     0
 #define _Py_THREAD_ATTACHED     1
-#define _Py_THREAD_GC           2
+#define _Py_THREAD_SUSPENDED    2
 
 
 /* Check if the current thread is the main thread.
@@ -146,13 +150,13 @@ extern void _PyThreadState_Attach(PyThreadState *tstate);
 // calls this function.
 extern void _PyThreadState_Detach(PyThreadState *tstate);
 
-// Temporarily pauses the thread in the GC state.
+// Detaches the current thread to the "suspended" state if a stop-the-world
+// pause is in progress.
 //
-// This is used to implement stop-the-world pauses. The thread must be in the
-// "attached" state. It will switch to the "GC" state and pause until the
-// stop-the-world event completes, after which it will switch back to the
-// "attached" state.
-extern void _PyThreadState_Park(PyThreadState *tstate);
+// If there is no stop-the-world pause in progress, then this function is
+// a no-op. Returns one if the thread was switched to the "suspended" state and
+// zero otherwise.
+extern int _PyThreadState_Suspend(PyThreadState *tstate);
 
 // Perform a stop-the-world pause for all threads in the all interpreters.
 //

Include/internal/pycore_runtime.h

@@ -227,6 +227,10 @@ typedef struct pyruntimestate {
     struct _faulthandler_runtime_state faulthandler;
     struct _tracemalloc_runtime_state tracemalloc;
 
+    // The rwmutex is used to prevent overlapping global and per-interpreter
+    // stop-the-world events. Global stop-the-world events lock the mutex
+    // exclusively (as a "writer"), while per-interpreter stop-the-world events
+    // lock it non-exclusively (as "readers").
     _PyRWMutex stoptheworld_mutex;
     struct _stoptheworld_state stoptheworld;
 

Python/ceval_gil.c

@@ -956,10 +956,14 @@ _Py_HandlePending(PyThreadState *tstate)
 {
     PyInterpreterState *interp = tstate->interp;
 
-    /* Pending signals */
+    /* Stop-the-world */
     if (_Py_eval_breaker_bit_is_set(interp, _PY_EVAL_PLEASE_STOP_BIT)) {
         _Py_set_eval_breaker_bit(interp, _PY_EVAL_PLEASE_STOP_BIT, 0);
-        _PyThreadState_Park(tstate);
+        if (_PyThreadState_Suspend(tstate)) {
+            /* The attach blocks until the stop-the-world event is complete. */
+            _PyThreadState_Attach(tstate);
+        }
+        // else: stale stop-the-world event, ignore it!
     }
 
     /* Pending signals */

Python/pystate.c

@@ -1564,7 +1564,7 @@ tstate_delete_common(PyThreadState *tstate)
     if (tstate->next) {
         tstate->next->prev = tstate->prev;
     }
-    if (tstate->state != _Py_THREAD_GC) {
+    if (tstate->state != _Py_THREAD_SUSPENDED) {
         // Any ongoing stop-the-world request should not wait for us because
         // our thread is getting deleted.
         if (interp->stoptheworld.requested) {
@@ -1804,9 +1804,9 @@ static void
 tstate_wait_attach(PyThreadState *tstate)
 {
     do {
-        int expected = _Py_THREAD_GC;
+        int expected = _Py_THREAD_SUSPENDED;
 
-        // Wait until we're switched out of GC to DETACHED.
+        // Wait until we're switched out of SUSPENDED to DETACHED.
         _PyParkingLot_Park(&tstate->state, &expected, sizeof(tstate->state),
                            /*timeout=*/-1, NULL, /*detach=*/0);
 
@@ -1870,19 +1870,8 @@ _PyThreadState_Detach(PyThreadState *tstate)
     detach_thread(tstate, _Py_THREAD_DETACHED);
 }
 
-// Decrease stop-the-world counter of remaining number of threads that need to
-// pause. If we are the final thread to pause, notify the requesting thread.
-static void
-decrement_stoptheworld_countdown(struct _stoptheworld_state *stw)
-{
-    assert(stw->thread_countdown > 0);
-    if (--stw->thread_countdown == 0) {
-        _PyEvent_Notify(&stw->stop_event);
-    }
-}
-
-void
-_PyThreadState_Park(PyThreadState *tstate)
+int
+_PyThreadState_Suspend(PyThreadState *tstate)
 {
     _PyRuntimeState *runtime = &_PyRuntime;
 
@@ -1899,25 +1888,30 @@ _PyThreadState_Park(PyThreadState *tstate)
     HEAD_UNLOCK(runtime);
 
     if (stw == NULL) {
-        // We might be processing a stale EVAL_PLEASE_STOP, in which
-        // case there is nothing to do. This can happen if a thread
-        // asks us to stop for a previous GC at the same time we detach.
-        return;
+        // Don't suspend if we are not in a stop-the-world event.
+        return 0;
     }
 
-    // Switch to GC state.
-    detach_thread(tstate, _Py_THREAD_GC);
+    // Switch to "suspended" state.
+    detach_thread(tstate, _Py_THREAD_SUSPENDED);
 
     // Decrease the count of remaining threads needing to park.
     HEAD_LOCK(runtime);
     decrement_stoptheworld_countdown(stw);
     HEAD_UNLOCK(runtime);
-
-    // Wait until we are switched back to DETACHED and then re-attach. After
-    // this we will be in the ATTACHED state, the same as before.
-    tstate_wait_attach(tstate);
+    return 1;
 }
 
+// Decrease stop-the-world counter of remaining number of threads that need to
+// pause. If we are the final thread to pause, notify the requesting thread.
+static void
+decrement_stoptheworld_countdown(struct _stoptheworld_state *stw)
+{
+    assert(stw->thread_countdown > 0);
+    if (--stw->thread_countdown == 0) {
+        _PyEvent_Notify(&stw->stop_event);
+    }
+}
 
 #ifdef Py_GIL_DISABLED
 // Interpreter for _Py_FOR_EACH_THREAD(). For global stop-the-world events,
@@ -1935,10 +1929,10 @@ interp_for_stop_the_world(struct _stoptheworld_state *stw)
 // Loops over threads for a stop-the-world event.
 // For global: all threads in all interpreters
 // For per-interpreter: all threads in the interpreter
-#define _Py_FOR_EACH_THREAD(stw)                                            \
-    for (PyInterpreterState *i = interp_for_stop_the_world((stw));          \
+#define _Py_FOR_EACH_THREAD(stw, i, t)                                      \
+    for (i = interp_for_stop_the_world((stw));                              \
             i != NULL; i = ((stw->is_global) ? i->next : NULL))             \
-        for (PyThreadState *t = i->threads.head; t; t = t->next)
+        for (t = i->threads.head; t; t = t->next)
 
 
 // Try to transition threads atomically from the "detached" state to the
@@ -1947,12 +1941,14 @@ static bool
 park_detached_threads(struct _stoptheworld_state *stw)
 {
     int num_parked = 0;
-    _Py_FOR_EACH_THREAD(stw) {
+    PyInterpreterState *i;
+    PyThreadState *t;
+    _Py_FOR_EACH_THREAD(stw, i, t) {
         int state = _Py_atomic_load_int_relaxed(&t->state);
         if (state == _Py_THREAD_DETACHED) {
-            // Atomically transition to _Py_THREAD_GC if in detached state.
+            // Atomically transition to "suspended" if in "detached" state.
             if (_Py_atomic_compare_exchange_int(&t->state,
-                                                &state, _Py_THREAD_GC)) {
+                                                &state, _Py_THREAD_SUSPENDED)) {
                 num_parked++;
             }
         }
@@ -1982,9 +1978,12 @@ stop_the_world(struct _stoptheworld_state *stw)
     HEAD_LOCK(runtime);
     stw->requested = 1;
     stw->thread_countdown = 0;
+    stw->stop_event = (PyEvent){0};  // zero-initialize (unset)
     stw->requester = _PyThreadState_GET();  // may be NULL
 
-    _Py_FOR_EACH_THREAD(stw) {
+    PyInterpreterState *i;
+    PyThreadState *t;
+    _Py_FOR_EACH_THREAD(stw, i, t) {
         if (t != stw->requester) {
             // Count all the other threads (we don't wait on ourself).
             stw->thread_countdown++;
@@ -2006,10 +2005,9 @@ stop_the_world(struct _stoptheworld_state *stw)
             break;
         }
 
-        int64_t wait_ns = 1000*1000;  // 1ms
+        _PyTime_t wait_ns = 1000*1000;  // 1ms (arbitrary, may need tuning)
         if (PyEvent_WaitTimed(&stw->stop_event, wait_ns)) {
             assert(stw->thread_countdown == 0);
-            stw->stop_event = (PyEvent){0};
             break;
         }
 
@@ -2029,12 +2027,12 @@ start_the_world(struct _stoptheworld_state *stw)
     stw->world_stopped = 0;
     stw->requester = NULL;
     // Switch threads back to the detached state.
-    _Py_FOR_EACH_THREAD(stw) {
-        int state = _Py_atomic_load_int_relaxed(&t->state);
-        if (state == _Py_THREAD_GC &&
-            _Py_atomic_compare_exchange_int(&t->state,
-                                            &state,
-                                            _Py_THREAD_DETACHED)) {
+    PyInterpreterState *i;
+    PyThreadState *t;
+    _Py_FOR_EACH_THREAD(stw, i, t) {
+        if (t != stw->requester) {
+            assert(t->state == _Py_THREAD_SUSPENDED);
+            _Py_atomic_store_int(&t->state, _Py_THREAD_DETACHED);
             _PyParkingLot_UnparkAll(&t->state);
         }
     }