From: Tim Serong Date: Fri, 12 May 2017 12:24:19 +0000 (+1000) Subject: mgr: use new Gil class in place of PyGILState_*() API X-Git-Tag: v12.1.0~87^2~13^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=f36b0d9d4053e167f59b89286cc9378e8e1563f7;p=ceph.git mgr: use new Gil class in place of PyGILState_*() API Prep work for loading modules in separate python sub-interpreters. According to the Python C API docs, mixing the PyGILState_*() API with sub-interpreters is unsupported, so I've replaced these calls with a Gil class, which will acquire and release the GIL against a specific python thread state. Note the manual python thread state creation in MgrPyModule::serve(). The PyGILState_*() APIs would have done that for us, but we're not using them anymore, so have to do it by hand (see https://docs.python.org/2/c-api/init.html#non-python-created-threads for some description of this). Signed-off-by: Tim Serong --- diff --git a/src/mgr/Gil.h b/src/mgr/Gil.h new file mode 100644 index 000000000000..522d4b0e18ad --- /dev/null +++ b/src/mgr/Gil.h @@ -0,0 +1,96 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab +/* + * Ceph - scalable distributed file system + * + * Copyright (C) 2017 SUSE LLC + * + * This is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License version 2.1, as published by the Free Software + * Foundation. See file COPYING. + * + */ + +#ifndef GIL_H_ +#define GIL_H_ + +#include "Python.h" + +#include "common/debug.h" + +#define dout_context g_ceph_context +#define dout_subsys ceph_subsys_mgr +#undef dout_prefix +#define dout_prefix *_dout << "mgr " << __func__ << " " + +// +// Use one of these in any scope in which you need to hold Python's +// Global Interpreter Lock. +// +// Do *not* nest these, as a second GIL acquire will deadlock (see +// https://docs.python.org/2/c-api/init.html#c.PyEval_RestoreThread) +// +// If in doubt, explicitly put a scope around the block of code you +// know you need the GIL in. +// +// See the comment below for when to set new_thread == true +// +class Gil { +public: + Gil(const Gil&) = delete; + Gil& operator=(const Gil&) = delete; + + Gil(PyThreadState *ts, bool new_thread = false) : pThreadState(ts) + { + assert(pThreadState != nullptr); + + // Acquire the GIL, set the current thread state + PyEval_RestoreThread(pThreadState); + dout(20) << "GIL acquired for thread state " << pThreadState << dendl; + + // + // If called from a separate OS thread (i.e. a thread not created + // by Python, that does't already have a python thread state that + // was created when that thread was active), we need to manually + // create and switch to a python thread state specifically for this + // OS thread. + // + // Note that instead of requring the caller to set new_thread == true + // when calling this from a separate OS thread, we could figure out + // if this was necessary automatically, as follows: + // + // if (pThreadState->thread_id != PyThread_get_thread_ident()) { + // + // However, this means we're accessing pThreadState->thread_id, but + // the Python C API docs say that "The only public data member is + // PyInterpreterState *interp", i.e. doing this would violate + // something that's meant to be a black box. + // + if (new_thread) { + pNewThreadState = PyThreadState_New(pThreadState->interp); + PyThreadState_Swap(pNewThreadState); + dout(20) << "Switched to new thread state " << pNewThreadState << dendl; + } + } + + ~Gil() + { + if (pNewThreadState != nullptr) { + dout(20) << "Destroying new thread state " << pNewThreadState << dendl; + PyThreadState_Swap(pThreadState); + PyThreadState_Clear(pNewThreadState); + PyThreadState_Delete(pNewThreadState); + } + // Release the GIL, reset the thread state to NULL + PyEval_SaveThread(); + dout(20) << "GIL released for thread state " << pThreadState << dendl; + } + +private: + PyThreadState *pThreadState; + PyThreadState *pNewThreadState = nullptr; +}; + +#endif + diff --git a/src/mgr/MgrPyModule.cc b/src/mgr/MgrPyModule.cc index 078f7f503503..fb3719f9acdf 100644 --- a/src/mgr/MgrPyModule.cc +++ b/src/mgr/MgrPyModule.cc @@ -11,6 +11,7 @@ * Foundation. See file COPYING. */ +#include "Gil.h" #include "PyFormatter.h" @@ -49,29 +50,32 @@ std::string handle_pyerror() #undef dout_prefix #define dout_prefix *_dout << "mgr " << __func__ << " " -MgrPyModule::MgrPyModule(const std::string &module_name_) +MgrPyModule::MgrPyModule(const std::string &module_name_, PyThreadState *main_ts_) : module_name(module_name_), - pClassInstance(nullptr) -{} + pClassInstance(nullptr), + pMainThreadState(main_ts_) +{ + assert(pMainThreadState != nullptr); +} MgrPyModule::~MgrPyModule() { - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); + Gil gil(pMainThreadState); Py_XDECREF(pClassInstance); - - PyGILState_Release(gstate); } int MgrPyModule::load() { + Gil gil(pMainThreadState); + // Load the module PyObject *pName = PyString_FromString(module_name.c_str()); auto pModule = PyImport_Import(pName); Py_DECREF(pName); if (pModule == nullptr) { derr << "Module not found: '" << module_name << "'" << dendl; + derr << handle_pyerror() << dendl; return -ENOENT; } @@ -81,6 +85,7 @@ int MgrPyModule::load() Py_DECREF(pModule); if (pClass == nullptr) { derr << "Class not found in module '" << module_name << "'" << dendl; + derr << handle_pyerror() << dendl; return -EINVAL; } @@ -95,6 +100,7 @@ int MgrPyModule::load() Py_DECREF(pArgs); if (pClassInstance == nullptr) { derr << "Failed to construct class in '" << module_name << "'" << dendl; + derr << handle_pyerror() << dendl; return -EINVAL; } else { dout(1) << "Constructed class from module: " << module_name << dendl; @@ -107,8 +113,9 @@ int MgrPyModule::serve() { assert(pClassInstance != nullptr); - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); + // This method is called from a separate OS thread (i.e. a thread not + // created by Python), so tell Gil to wrap this in a new thread state. + Gil gil(pMainThreadState, true); auto pValue = PyObject_CallMethod(pClassInstance, const_cast("serve"), nullptr); @@ -122,8 +129,6 @@ int MgrPyModule::serve() return -EINVAL; } - PyGILState_Release(gstate); - return r; } @@ -132,8 +137,7 @@ void MgrPyModule::shutdown() { assert(pClassInstance != nullptr); - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); + Gil gil(pMainThreadState); auto pValue = PyObject_CallMethod(pClassInstance, const_cast("shutdown"), nullptr); @@ -144,16 +148,13 @@ void MgrPyModule::shutdown() derr << "Failed to invoke shutdown() on " << module_name << dendl; derr << handle_pyerror() << dendl; } - - PyGILState_Release(gstate); } void MgrPyModule::notify(const std::string ¬ify_type, const std::string ¬ify_id) { assert(pClassInstance != nullptr); - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); + Gil gil(pMainThreadState); // Execute auto pValue = PyObject_CallMethod(pClassInstance, @@ -170,16 +171,13 @@ void MgrPyModule::notify(const std::string ¬ify_type, const std::string ¬i // a hook to unload misbehaving modules when they have an // error somewhere like this } - - PyGILState_Release(gstate); } void MgrPyModule::notify_clog(const LogEntry &log_entry) { assert(pClassInstance != nullptr); - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); + Gil gil(pMainThreadState); // Construct python-ized LogEntry PyFormatter f; @@ -201,15 +199,12 @@ void MgrPyModule::notify_clog(const LogEntry &log_entry) // a hook to unload misbehaving modules when they have an // error somewhere like this } - - PyGILState_Release(gstate); } int MgrPyModule::load_commands() { - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); - + // Don't need a Gil here -- this is called from MgrPyModule::load(), + // which already has one. PyObject *command_list = PyObject_GetAttrString(pClassInstance, "COMMANDS"); assert(command_list != nullptr); const size_t list_size = PyList_Size(command_list); @@ -239,8 +234,6 @@ int MgrPyModule::load_commands() } Py_DECREF(command_list); - PyGILState_Release(gstate); - dout(10) << "loaded " << commands.size() << " commands" << dendl; return 0; @@ -254,8 +247,7 @@ int MgrPyModule::handle_command( assert(ss != nullptr); assert(ds != nullptr); - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); + Gil gil(pMainThreadState); PyFormatter f; cmdmap_dump(cmdmap, &f); @@ -283,8 +275,6 @@ int MgrPyModule::handle_command( r = -EINVAL; } - PyGILState_Release(gstate); - return r; } diff --git a/src/mgr/MgrPyModule.h b/src/mgr/MgrPyModule.h index 7d91275eacb4..ecb52310c0a0 100644 --- a/src/mgr/MgrPyModule.h +++ b/src/mgr/MgrPyModule.h @@ -44,13 +44,14 @@ class MgrPyModule private: const std::string module_name; PyObject *pClassInstance; + PyThreadState *pMainThreadState; std::vector commands; int load_commands(); public: - MgrPyModule(const std::string &module_name); + MgrPyModule(const std::string &module_name, PyThreadState *main_ts); ~MgrPyModule(); int load(); diff --git a/src/mgr/PyModules.cc b/src/mgr/PyModules.cc index 10f4ce4787d2..8cc3f674c0b9 100644 --- a/src/mgr/PyModules.cc +++ b/src/mgr/PyModules.cc @@ -13,6 +13,7 @@ // Include this first to get python headers earlier #include "PyState.h" +#include "Gil.h" #include #include "common/errno.h" @@ -403,16 +404,21 @@ int PyModules::init() PyEval_InitThreads(); } + // Drop the GIL and remember the main thread state (current + // thread state becomes NULL) + pMainThreadState = PyEval_SaveThread(); + // Load python code boost::tokenizer<> tok(g_conf->mgr_modules); for(const auto& module_name : tok) { dout(1) << "Loading python module '" << module_name << "'" << dendl; - auto mod = std::unique_ptr(new MgrPyModule(module_name)); + auto mod = std::unique_ptr(new MgrPyModule(module_name, pMainThreadState)); int r = mod->load(); if (r != 0) { + // Don't use handle_pyerror() here; we don't have the GIL + // or a thread state (this is deliberate). derr << "Error loading module '" << module_name << "': " << cpp_strerror(r) << dendl; - derr << handle_pyerror() << dendl; // Don't drop out here, load the other modules } else { // Success! @@ -420,9 +426,6 @@ int PyModules::init() } } - // Drop the GIL - PyEval_SaveThread(); - return 0; } @@ -438,15 +441,12 @@ public: void *entry() override { - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); running = true; + // No need to acquire the GIL here; the module does it. dout(4) << "Entering thread for " << mod->get_name() << dendl; mod->serve(); - PyGILState_Release(gstate); - running = false; return nullptr; } @@ -495,7 +495,7 @@ void PyModules::shutdown() modules.clear(); - PyGILState_Ensure(); + PyEval_RestoreThread(pMainThreadState); Py_Finalize(); // nobody needs me anymore. diff --git a/src/mgr/PyModules.h b/src/mgr/PyModules.h index c68366f2a176..4c858747306f 100644 --- a/src/mgr/PyModules.h +++ b/src/mgr/PyModules.h @@ -43,6 +43,8 @@ class PyModules std::string get_site_packages(); + PyThreadState *pMainThreadState = nullptr; + public: static std::string config_prefix; diff --git a/src/mgr/PyState.cc b/src/mgr/PyState.cc index 512098a6638c..1635f761d6c7 100644 --- a/src/mgr/PyState.cc +++ b/src/mgr/PyState.cc @@ -22,8 +22,10 @@ #include "common/version.h" #include "PyState.h" +#include "Gil.h" #define dout_context g_ceph_context +#define dout_subsys ceph_subsys_mgr PyModules *global_handle = NULL; @@ -32,13 +34,14 @@ class MonCommandCompletion : public Context { PyObject *python_completion; const std::string tag; + PyThreadState *pThreadState; public: std::string outs; bufferlist outbl; - MonCommandCompletion(PyObject* ev, const std::string &tag_) - : python_completion(ev), tag(tag_) + MonCommandCompletion(PyObject* ev, const std::string &tag_, PyThreadState *ts_) + : python_completion(ev), tag(tag_), pThreadState(ts_) { assert(python_completion != nullptr); Py_INCREF(python_completion); @@ -51,28 +54,28 @@ public: void finish(int r) override { - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); - - auto set_fn = PyObject_GetAttrString(python_completion, "complete"); - assert(set_fn != nullptr); - - auto pyR = PyInt_FromLong(r); - auto pyOutBl = PyString_FromString(outbl.to_str().c_str()); - auto pyOutS = PyString_FromString(outs.c_str()); - auto args = PyTuple_Pack(3, pyR, pyOutBl, pyOutS); - Py_DECREF(pyR); - Py_DECREF(pyOutBl); - Py_DECREF(pyOutS); - - auto rtn = PyObject_CallObject(set_fn, args); - if (rtn != nullptr) { - Py_DECREF(rtn); + dout(10) << "MonCommandCompletion::finish()" << dendl; + { + // Scoped so the Gil is released before calling notify_all() + Gil gil(pThreadState); + + auto set_fn = PyObject_GetAttrString(python_completion, "complete"); + assert(set_fn != nullptr); + + auto pyR = PyInt_FromLong(r); + auto pyOutBl = PyString_FromString(outbl.to_str().c_str()); + auto pyOutS = PyString_FromString(outs.c_str()); + auto args = PyTuple_Pack(3, pyR, pyOutBl, pyOutS); + Py_DECREF(pyR); + Py_DECREF(pyOutBl); + Py_DECREF(pyOutS); + + auto rtn = PyObject_CallObject(set_fn, args); + if (rtn != nullptr) { + Py_DECREF(rtn); + } + Py_DECREF(args); } - Py_DECREF(args); - - PyGILState_Release(gstate); - global_handle->notify_all("command", tag); } }; @@ -105,7 +108,7 @@ ceph_send_command(PyObject *self, PyObject *args) } Py_DECREF(set_fn); - auto c = new MonCommandCompletion(completion, tag); + auto c = new MonCommandCompletion(completion, tag, PyThreadState_Get()); if (std::string(type) == "mon") { global_handle->get_monc().start_mon_command( {cmd_json},