gh-111964: Implement stop-the-world pauses #112471

colesbury · 2023-11-27T20:00:14Z

The --disable-gil builds occasionally need to pause all but one thread. Some examples include:

Cyclic garbage collection, where this is often called a "stop the world event"
Before calling fork(), to ensure a consistent state for internal data structures
During interpreter shutdown, to ensure that daemon threads aren't accessing Python objects

This adds the following functions to implement global and per-interpreter pauses:

_PyRuntimeState_StopTheWorld and _PyRuntimeState_StartTheWorld
_PyInterpreterState_StopTheWorld and _PyInterpreterState_StartTheWorld

These functions are no-ops outside of the --disable-gil build.

This also adds _PyRWMutex, a "readers-writer" lock, which is used to serialize global stop-the-world pauses with per-interpreter pauses.

Issue: Implement stop-the-world functionality (for --disable-gil builds) #111964

Include/internal/pycore_interp.h

Include/internal/pycore_pystate.h

colesbury · 2023-11-27T21:33:44Z

@eendebakpt - thanks!

gvanrossum · 2023-11-27T22:41:55Z

Mark and I are currently on vacation. I'm not back until after Dec 8. Mark should be back this week.

ericsnowcurrently

The change mostly makes sense, but it took me a little while to connect all the pieces in my head. I've left various comments related to that and to a number of other things that might be worth adjusting.

Include/internal/pycore_lock.h

Python/parking_lot.c

Include/internal/pycore_lock.h

Python/lock.c

Python/pystate.c

bedevere-app · 2023-12-06T23:18:11Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

colesbury

I split off the _PyRWMutex PR here: #112859. (This is now rebased on that one.)

I don't think the stop-the-world functionality belongs in ceval related files. It's fundamentally about managing PyThreadState objects (particularly, the "state" field) and is closely tied to the existing _PyThreadState_Attach/Detach functions. The connection to eval_breaker is only a small piece, similar to how the GC has an eval breaker bit.

colesbury · 2023-12-07T00:17:25Z

Include/internal/pycore_interp.h

+// Support for stop-the-world events. This exists in both the PyRuntime struct
+// for global pauses and in each PyInterpreterState for per-interpreter pauses.
+struct _stoptheworld_state {
+    PyMutex mutex;       // Serializes stop-the-world attempts.
+
+    // NOTE: The fields below are protected by HEAD_LOCK(runtime), not by the
+    // above mutex.
+    bool requested;      // Set when a pause is requested.
+    bool world_stopped;  // Set when the world is stopped.
+    bool is_global;      // Set when contained in PyRuntime struct.
+
+    PyEvent stop_event;  // Set when thread_countdown reaches zero.
+    Py_ssize_t thread_countdown;  // Number of threads that must pause.
+
+    PyThreadState *requester; // Thread that requested the pause (may be NULL).
+};


It's only loosely related to the eval_breaker. It's fundamentally about PyThreadState.state: transitioning threads to the _Py_THREAD_GC state. You could still have this without eval-breaker (it would just be much less responsive), but you can't implement it without PyThreadState.state.

Include/internal/pycore_lock.h

Include/internal/pycore_runtime.h

Python/pystate.c

Python/lock.c

colesbury · 2023-12-07T22:37:25Z

Include/internal/pycore_pystate.h

+// Perform a stop-the-world pause for all threads in all interpreters.
+//
+// Threads in the "attached" state are paused and transitioned to the "GC"
+// state. Threads in the "detached" state switch to the "GC" state, preventing
+// them from reattaching until the stop-the-world pause is complete.
+//
+// NOTE: This is a no-op outside of Py_GIL_DISABLED builds.
+extern void _PyRuntimeState_StopTheWorld(_PyRuntimeState *runtime);
+extern void _PyRuntimeState_StartTheWorld(_PyRuntimeState *runtime);
+
+// Perform a stop-the-world pause for threads in the specified interpreter.
+//
+// NOTE: This is a no-op outside of Py_GIL_DISABLED builds.
+extern void _PyInterpreterState_StopTheWorld(PyInterpreterState *interp);
+extern void _PyInterpreterState_StartTheWorld(PyInterpreterState *interp);


I like the common prefix for the global and per-interpreter APIs.

I don't think we should combine the APIs because you don't need a thread-state or interpreter state to start a global stop the world pause.

ericsnowcurrently · 2023-12-13T23:18:18Z

Include/internal/pycore_pystate.h

 //            (bound thread)        (stop-the-world thread)
-// [attached]       <->       [detached]       <->       [gc]
+// [attached]       <->       [detached]       <->       [suspended]
+//   +                                                        ^
+//   +--------------------------------------------------------+
+//                          (bound thread)


FWIW, the text above makes it sound like thread a thread can go from "suspended" straight to "attached".

Also, (bound thread) shows up at the top of the diagram and at the bottom. I'm not sure how to interpret that.

By text above do you mean the diagram or the paragraph above it? I've updated both in an attempt to make them more clear.

The "(bound thread)" and "(stop-the-world thread)" labels indicate which thread can perform the transition. It's at the bottom because it's labeling the one-way transition from "attached" to "suspended".

ericsnowcurrently · 2023-12-13T23:21:15Z

Include/internal/pycore_pystate.h

+// 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);


Would it make sense to fall back to detaching?

Yeah, I think that's conceptually simpler.

Include/internal/pycore_runtime.h

Python/pystate.c

ericsnowcurrently · 2023-12-14T00:05:17Z

Include/internal/pycore_pystate.h

+// Perform a stop-the-world pause for all threads in all interpreters.
+//
+// Threads in the "attached" state are paused and transitioned to the "GC"
+// state. Threads in the "detached" state switch to the "GC" state, preventing
+// them from reattaching until the stop-the-world pause is complete.
+//
+// NOTE: This is a no-op outside of Py_GIL_DISABLED builds.
+extern void _PyRuntimeState_StopTheWorld(_PyRuntimeState *runtime);
+extern void _PyRuntimeState_StartTheWorld(_PyRuntimeState *runtime);
+
+// Perform a stop-the-world pause for threads in the specified interpreter.
+//
+// NOTE: This is a no-op outside of Py_GIL_DISABLED builds.
+extern void _PyInterpreterState_StopTheWorld(PyInterpreterState *interp);
+extern void _PyInterpreterState_StartTheWorld(PyInterpreterState *interp);


My main reservation is that the PyInterpreterState_ and PyRuntimeState_ prefixes are used for either simple getters/setters or struct lifecycle. For anything beyond that we either use a Py_ prefix or a prefix that relates to a specific feature. (We're a bit freer with the PyThreadState_ prefix, but I'm okay with that.) In this case I got the sense that this is a ceval feature, hence suggesting the _PyEval_ prefix.

Regardless, let's find a prefix other than _Py*State_.

ericsnowcurrently

Mostly things are looking good. There's the _PyRWMutex PR, I have one comment here, and there are a couple still open from my earlier review.

colesbury

I renamed the functions to _PyEval_StopTheWorldAll, etc. at least as a placeholder. I couldn't think of anything better. We could also go with only the _Py prefix.

colesbury · 2023-12-14T17:59:50Z

Include/internal/pycore_pystate.h

 //            (bound thread)        (stop-the-world thread)
-// [attached]       <->       [detached]       <->       [gc]
+// [attached]       <->       [detached]       <->       [suspended]
+//   +                                                        ^
+//   +--------------------------------------------------------+
+//                          (bound thread)


By text above do you mean the diagram or the paragraph above it? I've updated both in an attempt to make them more clear.

The "(bound thread)" and "(stop-the-world thread)" labels indicate which thread can perform the transition. It's at the bottom because it's labeling the one-way transition from "attached" to "suspended".

colesbury · 2023-12-14T18:38:12Z

Include/internal/pycore_pystate.h

+// 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);


Yeah, I think that's conceptually simpler.

Python/pystate.c

The `--disable-gil` builds occasionally need to pause all but one thread. Some examples include: * Cyclic garbage collection, where this is often called a "stop the world event" * Before calling `fork()`, to ensure a consistent state for internal data structures * During interpreter shutdown, to ensure that daemon threads aren't accessing Python objects This adds the following functions to implement global and per-interpreter pauses: * `_PyRuntimeState_StopTheWorld` and `_PyRuntimeState_StartTheWorld` * `_PyInterpreterState_StopTheWorld` and `_PyInterpreterState_StartTheWorld` These functions are no-ops outside of the `--disable-gil` build. This also adds `_PyRWMutex`, a "readers-writer" lock, which is used to serialize global stop-the-world pauses with per-interpreter pauses.

Starting in the "suspended" state blocks any _PyThreadState_Attach() call until the stop-the-world event completes.

_PyThreadState_Suspend now switches to "detached" if there is no active stop-the-world request.

colesbury · 2023-12-18T17:04:00Z

@ericsnowcurrently, I've rebased the PR onto main to get the changes from gh-112859 (_PyRWMutex).

colesbury · 2024-01-04T23:09:09Z

Hi @ericsnowcurrently - happy New Year! Would you please look this over and let me know if there are any issues that still need to be addressed?

I don't think we came to a decision on the names of _PyEval_StopTheWorldAll(), etc. functions. Especially as these are internal-only, I'm happy to go with whatever naming you prefer.

ericsnowcurrently

Sorry for the delay. LGTM

ericsnowcurrently

LGTM

ericsnowcurrently · 2024-01-23T00:11:21Z

Python/pystate.c

+void
+_PyEval_StopTheWorldAll(_PyRuntimeState *runtime)
+{
+#ifdef Py_GIL_DISABLED
+    stop_the_world(&runtime->stoptheworld);
+#endif
+}
+
+void
+_PyEval_StartTheWorldAll(_PyRuntimeState *runtime)
+{
+#ifdef Py_GIL_DISABLED
+    start_the_world(&runtime->stoptheworld);
+#endif
+}
+
+void
+_PyEval_StopTheWorld(PyInterpreterState *interp)
+{
+#ifdef Py_GIL_DISABLED
+    stop_the_world(&interp->stoptheworld);
+#endif
+}
+
+void
+_PyEval_StartTheWorld(PyInterpreterState *interp)
+{
+#ifdef Py_GIL_DISABLED
+    start_the_world(&interp->stoptheworld);
+#endif
+}


I still think these and the related static functions make more sense in Python/ceval_gil.c, but not enough to hold up this PR.

We can revisit some the discussion (including some of the alternate names I suggested) separately.

colesbury · 2024-01-23T17:15:32Z

Thanks @ericsnowcurrently! Would you please merge the PR?

ericsnowcurrently · 2024-01-23T18:08:46Z

Thanks, @colesbury!

The `--disable-gil` builds occasionally need to pause all but one thread. Some examples include: * Cyclic garbage collection, where this is often called a "stop the world event" * Before calling `fork()`, to ensure a consistent state for internal data structures * During interpreter shutdown, to ensure that daemon threads aren't accessing Python objects This adds the following functions to implement global and per-interpreter pauses: * `_PyEval_StopTheWorldAll()` and `_PyEval_StartTheWorldAll()` (for the global runtime) * `_PyEval_StopTheWorld()` and `_PyEval_StartTheWorld()` (per-interpreter) (The function names may change.) These functions are no-ops outside of the `--disable-gil` build.

colesbury added topic-C-API 3.13 bugs and security fixes topic-free-threading labels Nov 27, 2023

colesbury requested a review from ericsnowcurrently November 27, 2023 20:00

colesbury added the skip news label Nov 27, 2023

colesbury force-pushed the stop-the-world branch from 1ebcb70 to db1dd03 Compare November 27, 2023 20:23

bedevere-app bot mentioned this pull request Nov 27, 2023

Implement stop-the-world functionality (for --disable-gil builds) #111964

Closed

eendebakpt reviewed Nov 27, 2023

View reviewed changes

Include/internal/pycore_interp.h Show resolved Hide resolved

eendebakpt reviewed Nov 27, 2023

View reviewed changes

Include/internal/pycore_pystate.h Show resolved Hide resolved

colesbury marked this pull request as ready for review November 27, 2023 22:21

colesbury requested review from markshannon and gvanrossum as code owners November 27, 2023 22:21

bedevere-app bot added the awaiting review label Nov 27, 2023

ericsnowcurrently requested changes Dec 6, 2023

View reviewed changes

bedevere-app bot removed the awaiting review label Dec 6, 2023

bedevere-app bot added the awaiting changes label Dec 6, 2023

colesbury commented Dec 7, 2023

View reviewed changes

colesbury force-pushed the stop-the-world branch from fcd642b to a6f5ab7 Compare December 7, 2023 22:51

colesbury requested a review from ericsnowcurrently December 7, 2023 22:51

ericsnowcurrently mentioned this pull request Dec 13, 2023

gh-111964: Add _PyRWMutex a "readers-writer" lock #112859

Merged

ericsnowcurrently reviewed Dec 13, 2023

View reviewed changes

ericsnowcurrently reviewed Dec 14, 2023

View reviewed changes

colesbury commented Dec 14, 2023

View reviewed changes

colesbury added 3 commits December 18, 2023 11:52

Changes from review

71ffa1d

Stop-the-world requests should block new threads.

4ca1e31

Starting in the "suspended" state blocks any _PyThreadState_Attach() call until the stop-the-world event completes.

colesbury added 2 commits December 18, 2023 11:52

Update comments and _PyThreadState_Suspend.

470298f

_PyThreadState_Suspend now switches to "detached" if there is no active stop-the-world request.

Rename stop-the-world functions

4f7b6ce

colesbury force-pushed the stop-the-world branch from 12c6585 to 4f7b6ce Compare December 18, 2023 16:54

colesbury mentioned this pull request Jan 22, 2024

gh-112529: Implement GC for free-threaded builds #114262

Merged

ericsnowcurrently approved these changes Jan 23, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Jan 23, 2024

ericsnowcurrently approved these changes Jan 23, 2024

View reviewed changes

ericsnowcurrently merged commit 441affc into python:main Jan 23, 2024
31 of 32 checks passed

bedevere-app bot removed the awaiting merge label Jan 23, 2024

colesbury deleted the stop-the-world branch February 15, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-111964: Implement stop-the-world pauses #112471

gh-111964: Implement stop-the-world pauses #112471

colesbury commented Nov 27, 2023 •

edited by bedevere-app bot

Loading

colesbury commented Nov 27, 2023

gvanrossum commented Nov 27, 2023

ericsnowcurrently left a comment

bedevere-app bot commented Dec 6, 2023

colesbury left a comment

colesbury Dec 7, 2023

colesbury Dec 7, 2023

ericsnowcurrently Dec 13, 2023

ericsnowcurrently Dec 13, 2023

colesbury Dec 14, 2023

ericsnowcurrently Dec 13, 2023 •

edited

Loading

colesbury Dec 14, 2023

ericsnowcurrently Dec 14, 2023

ericsnowcurrently left a comment

colesbury left a comment

colesbury Dec 14, 2023

colesbury Dec 14, 2023

colesbury commented Dec 18, 2023

colesbury commented Jan 4, 2024

ericsnowcurrently left a comment

ericsnowcurrently left a comment

ericsnowcurrently Jan 23, 2024

colesbury commented Jan 23, 2024

ericsnowcurrently commented Jan 23, 2024

gh-111964: Implement stop-the-world pauses #112471

gh-111964: Implement stop-the-world pauses #112471

Conversation

colesbury commented Nov 27, 2023 • edited by bedevere-app bot Loading

colesbury commented Nov 27, 2023

gvanrossum commented Nov 27, 2023

ericsnowcurrently left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Dec 6, 2023

colesbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently left a comment

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesbury commented Dec 18, 2023

colesbury commented Jan 4, 2024

ericsnowcurrently left a comment

Choose a reason for hiding this comment

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesbury commented Jan 23, 2024

ericsnowcurrently commented Jan 23, 2024

colesbury commented Nov 27, 2023 •

edited by bedevere-app bot

Loading

ericsnowcurrently Dec 13, 2023 •

edited

Loading