pybind/rbd: fix Python 3.12+ sub-interpreter crashes in callbacks#66446
pybind/rbd: fix Python 3.12+ sub-interpreter crashes in callbacks#66446
Conversation
|
jenkins test api |
30e5d0c to
0204169
Compare
0204169 to
c463c4b
Compare
|
jenkins test windows |
src/pybind/rbd/rbd.pyx
Outdated
|
|
||
| On Python 3.12+, returns a ProgressCallbackWrapper that switches to the | ||
| correct interpreter context when invoked. On Python < 3.12, returns the | ||
| callback unchanged for performance (no wrapper overhead). |
There was a problem hiding this comment.
I wonder if there is a way to switch on whether sub-interpreters are in use? If the overhead is measurable, it would be nice to avoid it for any users outside of rbd_support module (i.e. ceph-mgr framework).
There was a problem hiding this comment.
@idryomov hi Ilya, i performed a simple benchmark to understand the performance impact:
Real-World Operation Overhead
- Image flatten (100 callbacks): 0.001 ms
- Migration (200 callbacks): 0.003 ms
- Bulk AIO (1000 ops): 0.059 ms
These operations take seconds to minutes, making sub-millisecond overhead <0.001% of total operation time.
Raw Performance Numbers
Per-Call Overhead (with PyThreadState_Swap):
- 0.016 microseconds per callback invocation
Progress Callbacks (10,000 calls):
- Simple: +124% overhead (+0.194 ms absolute). The callback func just return immediately, without doing anything
- Moderate: +49% overhead (+0.180 ms absolute). The callback just perform simple calculation before returning.
- Complex: +26% overhead (+0.506 ms absolute). In addition to performing simple calculation, the callback performs string interpolation using the param passed in the callback.
AIO Callbacks (1,000 completions):
- +238% overhead (+0.059 ms absolute)
Conditional Wrapping Results
Testing in main interpreter (where detection would skip wrapping):
- Current (always wrap): 0.502 ms
- Conditional (skip): 0.374 ms
- Savings: 25.4% (0.127 ms for 10,000 calls)
Based on the test results, I'm adding a check before swapping the interpreter context in the latest revision.
There was a problem hiding this comment.
Testing in main interpreter (where detection would skip wrapping):
- Current (always wrap): 0.502 ms
- Conditional (skip): 0.374 ms
- Savings: 25.4% (0.127 ms for 10,000 calls)
The millisecond numbers show the overhead, right? IOW is it true that 0.374 ms for conditional swapping should be compared to 0 ms for never checking (and consequently never swapping, of course)?
There was a problem hiding this comment.
Honestly, I'm having some doubts about whether this kind of explicit support for subinterpreters is needed irrespective of the overhead. #66244 moves the entire manager away from them, and even though Sam's commit message mentions potentially reintroducing them for some modules, I'm yet to hear a convincing argument for using subinterpreters in the first place given that until Python 3.12 they all shared a single GIL. Perhaps you know some of the history behind the use of subinterpreters in the manager?
This changes the default behavior of ceph-mgr from running each module in a distinct subinterpreter to running them all in the same main interpreter. We can reintroduce subinterpreter support over time by adding modules to the list as we test them.
There was a problem hiding this comment.
Perhaps you know some of the history behind the use of subinterpreters in the manager?
@tserong has since clarified that it came about because of cherrypy:
It was impossible to have multiple mgr modules using cherrypy at the same time. Here's the original email thread on this topic from back in April 2017:
I'm assuming that #67227 takes care of that (and is basically what makes #66244 possible).
c463c4b to
205f02a
Compare
Python 3.12+ introduced significant changes to sub-interpreter isolation which cause crashes when callbacks are invoked from C code in a different interpreter context than where the callback was created. This affects both progress callbacks and AIO oncomplete callbacks when used in ceph-mgr's rbd_support module, which runs in a sub-interpreter. This patch implements a CallbackWrapper that uses PyThreadState_Swap() to switch to the correct interpreter context when callbacks are invoked: Implementation: - Captures the interpreter state when callback is created (in Python code) - Switches to that interpreter when callback is invoked (from C code) - Switches back to the original interpreter after callback completes - Optimized: skips swap if callback invoked in same interpreter (25% faster) - Generic: single wrapper class handles all callback types (*args, **kwargs) - Zero overhead on Python < 3.12 (no wrapping needed) The wrapper automatically fixes: Progress callbacks (6 sites): - RBD.remove() - RBD.trash_remove() - RBD.migration_execute() - RBD.migration_commit() - RBD.migration_abort() - Image.flatten() AIO oncomplete callbacks (all AIO methods): - RBD.aio_open_image() - Image.aio_close() - Image.aio_read/write/discard/write_zeroes/flush() - Image.aio_mirror_image_create_snapshot() - Image.aio_mirror_image_get_info() - Image.aio_mirror_image_get_mode() Benefits: - Fixes crashes in ceph-mgr rbd_support module on Python 3.12+ - Minimal overhead: 0.037 µs per callback invocation Benchmark results (10,000 callback invocations, Python 3.13): - With optimization: 0.374 ms (0.037 µs per call) - Without optimization: 0.502 ms (0.050 µs per call) - Improvement: -25.4% Real-world overhead remains negligible: - Image flatten (100 callbacks): ~0.004 ms - Migration (200 callbacks): ~0.007 ms - Bulk AIO (1000 ops): ~0.037 ms Refs: https://tracker.ceph.com/issues/73930 Fixes: https://tracker.ceph.com/issues/73857 Fixes: https://tracker.ceph.com/issues/72713 Signed-off-by: Kefu Chai <k.chai@proxmox.com>
2e9f547 to
7167eaf
Compare
|
changelog:
|
|
jenkins test api |
|
@idryomov hi Ilya, could you take another look at your convenience? |
|
@idryomov hi Ilya, ping? |
1 similar comment
|
@idryomov hi Ilya, ping? |
Hi Kefu, I have responded in one of the threads. Apologies for the delay! |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
I have pinned this PR pending a decision on whether support for subinterpreters across Python bindings for different components is something of interest. |
Python 3.12+ introduced significant changes to sub-interpreter isolation
which cause crashes when callbacks are invoked from C code in a different
interpreter context than where the callback was created. This affects both
progress callbacks and AIO oncomplete callbacks when used in ceph-mgr's
rbd_support module, which runs in a sub-interpreter.
This patch implements a CallbackWrapper that uses PyThreadState_Swap() to
switch to the correct interpreter context when callbacks are invoked:
Implementation:
The wrapper automatically fixes:
Progress callbacks (6 sites):
AIO oncomplete callbacks (all AIO methods):
Benefits:
Benchmark results (10,000 callback invocations, Python 3.13):
Real-world overhead remains negligible:
Refs: https://tracker.ceph.com/issues/73930
Refs: https://tracker.ceph.com/issues/73857
Fixes: https://tracker.ceph.com/issues/72713
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins test classic perfJenkins Job | Jenkins Job Definitionjenkins test crimson perfJenkins Job | Jenkins Job Definitionjenkins test signedJenkins Job | Jenkins Job Definitionjenkins test make checkJenkins Job | Jenkins Job Definitionjenkins test make check arm64Jenkins Job | Jenkins Job Definitionjenkins test submodulesJenkins Job | Jenkins Job Definitionjenkins test dashboardJenkins Job | Jenkins Job Definitionjenkins test dashboard cephadmJenkins Job | Jenkins Job Definitionjenkins test apiJenkins Job | Jenkins Job Definitionjenkins test docsReadTheDocs | Github Workflow Definitionjenkins test ceph-volume allJenkins Jobs | Jenkins Jobs Definitionjenkins test windowsJenkins Job | Jenkins Job Definitionjenkins test rook e2eJenkins Job | Jenkins Job DefinitionYou must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.