Skip to content

Commit

Permalink
Merge PR #44207 into master
Browse files Browse the repository at this point in the history 
* refs/pull/44207/head:
	mgr/ActivePyModule: avoid with_gil where possible
	mgr/ActivePyModules: push without_gil_t down into blocks

Reviewed-by: Kefu Chai <kchai@redhat.com>
  • Loading branch information
@liewegas
liewegas committed Dec 8, 2021
2 parents e714d6e + 6469a9a commit 60e0ea0
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 44 deletions.
98 changes: 55 additions & 43 deletions src/mgr/ActivePyModules.cc
View file
Expand Up @@ -116,7 +116,7 @@ PyObject *ActivePyModules::list_servers_python()
without_gil_t no_gil;
return daemon_state.with_daemons_by_server([this, &no_gil]
(const std::map<std::string, DaemonStateCollection> &all) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
PyFormatter f(false, true);
for (const auto &[hostname, daemon_state] : all) {
f.open_object_section("server");
Expand Down Expand Up @@ -171,28 +171,26 @@ PyObject *ActivePyModules::get_python(const std::string &what)
{
PyFormatter f;

// Drop the GIL, as most of the following blocks will block on
// a mutex -- they are all responsible for re-taking the GIL before
// touching the PyFormatter instance or returning from the function.
without_gil_t no_gil;

if (what == "fs_map") {
without_gil_t no_gil;
return cluster_state.with_fsmap([&](const FSMap &fsmap) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
fsmap.dump(&f);
return f.get();
});
} else if (what == "osdmap_crush_map_text") {
without_gil_t no_gil;
bufferlist rdata;
cluster_state.with_osdmap([&](const OSDMap &osd_map){
osd_map.crush->encode(rdata, CEPH_FEATURES_SUPPORTED_DEFAULT);
});
std::string crush_text = rdata.to_str();
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
return PyUnicode_FromString(crush_text.c_str());
} else if (what.substr(0, 7) == "osd_map") {
without_gil_t no_gil;
return cluster_state.with_osdmap([&](const OSDMap &osd_map){
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
if (what == "osd_map") {
osd_map.dump(&f);
} else if (what == "osd_map_tree") {
Expand All @@ -203,6 +201,7 @@ PyObject *ActivePyModules::get_python(const std::string &what)
return f.get();
});
} else if (what == "modified_config_options") {
without_gil_t no_gil;
auto all_daemons = daemon_state.get_all();
set<string> names;
for (auto& [key, daemon] : all_daemons) {
Expand All @@ -211,34 +210,36 @@ PyObject *ActivePyModules::get_python(const std::string &what)
names.insert(name);
}
}
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
f.open_array_section("options");
for (auto& name : names) {
f.dump_string("name", name);
}
f.close_section();
return f.get();
} else if (what.substr(0, 6) == "config") {
with_gil_t with_gil{no_gil};
if (what == "config_options") {
g_conf().config_options(&f);
} else if (what == "config") {
g_conf().show_config(&f);
}
return f.get();
} else if (what == "mon_map") {
without_gil_t no_gil;
return cluster_state.with_monmap([&](const MonMap &monmap) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
monmap.dump(&f);
return f.get();
});
} else if (what == "service_map") {
without_gil_t no_gil;
return cluster_state.with_servicemap([&](const ServiceMap &service_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
service_map.dump(&f);
return f.get();
});
} else if (what == "osd_metadata") {
without_gil_t no_gil;
auto dmc = daemon_state.get_by_service("osd");
for (const auto &[key, state] : dmc) {
std::lock_guard l(state->lock);
Expand All @@ -253,6 +254,7 @@ PyObject *ActivePyModules::get_python(const std::string &what)
}
return with_gil(no_gil, [&] { return f.get(); });
} else if (what == "mds_metadata") {
without_gil_t no_gil;
auto dmc = daemon_state.get_by_service("mds");
for (const auto &[key, state] : dmc) {
std::lock_guard l(state->lock);
Expand All @@ -267,6 +269,7 @@ PyObject *ActivePyModules::get_python(const std::string &what)
}
return with_gil(no_gil, [&] { return f.get(); });
} else if (what == "pg_summary") {
without_gil_t no_gil;
return cluster_state.with_pgmap(
[&f, &no_gil](const PGMap &pg_map) {
std::map<std::string, std::map<std::string, uint32_t> > osds;
Expand All @@ -282,7 +285,7 @@ PyObject *ActivePyModules::get_python(const std::string &what)
}
all[state]++;
}
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
f.open_object_section("by_osd");
for (const auto &i : osds) {
f.open_object_section(i.first.c_str());
Expand Down Expand Up @@ -313,22 +316,25 @@ PyObject *ActivePyModules::get_python(const std::string &what)
}
);
} else if (what == "pg_status") {
without_gil_t no_gil;
return cluster_state.with_pgmap(
[&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.print_summary(&f, nullptr);
return f.get();
}
);
} else if (what == "pg_dump") {
without_gil_t no_gil;
return cluster_state.with_pgmap(
[&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.dump(&f, false);
return f.get();
}
);
} else if (what == "devices") {
without_gil_t no_gil;
daemon_state.with_devices2(
[&] {
with_gil(no_gil, [&] { f.open_array_section("devices"); });
Expand All @@ -342,6 +348,7 @@ PyObject *ActivePyModules::get_python(const std::string &what)
});
} else if (what.size() > 7 &&
what.substr(0, 7) == "device ") {
without_gil_t no_gil;
string devid = what.substr(7);
if (!daemon_state.with_device(devid,
[&] (const DeviceState& dev) {
Expand All @@ -352,56 +359,62 @@ PyObject *ActivePyModules::get_python(const std::string &what)
}
return with_gil(no_gil, [&] { return f.get(); });
} else if (what == "io_rate") {
without_gil_t no_gil;
return cluster_state.with_pgmap(
[&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.dump_delta(&f);
return f.get();
}
);
} else if (what == "df") {
without_gil_t no_gil;
return cluster_state.with_osdmap_and_pgmap(
[&](
const OSDMap& osd_map,
const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.dump_cluster_stats(nullptr, &f, true);
pg_map.dump_pool_stats_full(osd_map, nullptr, &f, true);
return f.get();
});
} else if (what == "pg_stats") {
without_gil_t no_gil;
return cluster_state.with_pgmap([&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.dump_pg_stats(&f, false);
return f.get();
});
} else if (what == "pool_stats") {
without_gil_t no_gil;
return cluster_state.with_pgmap([&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.dump_pool_stats(&f);
return f.get();
});
} else if (what == "pg_ready") {
with_gil_t with_gil{no_gil};
server.dump_pg_ready(&f);
return f.get();
} else if (what == "osd_stats") {
without_gil_t no_gil;
return cluster_state.with_pgmap([&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.dump_osd_stats(&f, false);
return f.get();
});
} else if (what == "osd_ping_times") {
without_gil_t no_gil;
return cluster_state.with_pgmap([&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
pg_map.dump_osd_ping_times(&f);
return f.get();
});
} else if (what == "osd_pool_stats") {
without_gil_t no_gil;
int64_t poolid = -ENOENT;
return cluster_state.with_osdmap_and_pgmap([&](const OSDMap& osdmap,
const PGMap& pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
f.open_array_section("pool_stats");
for (auto &p : osdmap.get_pools()) {
poolid = p.first;
Expand All @@ -411,27 +424,29 @@ PyObject *ActivePyModules::get_python(const std::string &what)
return f.get();
});
} else if (what == "health") {
without_gil_t no_gil;
return cluster_state.with_health([&](const ceph::bufferlist &health_json) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
f.dump_string("json", health_json.to_str());
return f.get();
});
} else if (what == "mon_status") {
without_gil_t no_gil;
return cluster_state.with_mon_status(
[&](const ceph::bufferlist &mon_status_json) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
f.dump_string("json", mon_status_json.to_str());
return f.get();
});
} else if (what == "mgr_map") {
without_gil_t no_gil;
return cluster_state.with_mgrmap([&](const MgrMap &mgr_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
mgr_map.dump(&f);
return f.get();
});
} else if (what == "mgr_ips") {
entity_addrvec_t myaddrs = server.get_myaddrs();
with_gil_t with_gil{no_gil};
f.open_array_section("ips");
std::set<std::string> did;
for (auto& i : myaddrs.v) {
Expand All @@ -443,13 +458,13 @@ PyObject *ActivePyModules::get_python(const std::string &what)
f.close_section();
return f.get();
} else if (what == "have_local_config_map") {
with_gil_t with_gil{no_gil};
f.dump_bool("have_local_config_map", have_local_config_map);
return f.get();
} else if (what == "active_clean_pgs"){
without_gil_t no_gil;
cluster_state.with_pgmap(
[&](const PGMap &pg_map) {
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
f.open_array_section("pg_stats");
for (auto &i : pg_map.pg_stat) {
const auto state = i.second.state;
Expand All @@ -471,7 +486,6 @@ PyObject *ActivePyModules::get_python(const std::string &what)
return f.get();
} else {
derr << "Python module requested unknown data '" << what << "'" << dendl;
with_gil_t with_gil{no_gil};
Py_RETURN_NONE;
}
}
Expand Down Expand Up @@ -640,7 +654,7 @@ PyObject *ActivePyModules::get_typed_config(
}
if (found) {
PyModuleRef module = py_module_registry.get_module(module_name);
with_gil_t with_gil{no_gil};
no_gil.acquire_gil();
if (!module) {
derr << "Module '" << module_name << "' is not available" << dendl;
Py_RETURN_NONE;
Expand All @@ -657,7 +671,6 @@ PyObject *ActivePyModules::get_typed_config(
} else {
dout(10) << " " << key << " not found " << dendl;
}
with_gil_t with_gil{no_gil};
Py_RETURN_NONE;
}

Expand All @@ -667,20 +680,19 @@ PyObject *ActivePyModules::get_store_prefix(const std::string &module_name,
without_gil_t no_gil;
std::lock_guard l(lock);
std::lock_guard lock(module_config.lock);
no_gil.acquire_gil();

const std::string base_prefix = PyModule::mgr_store_prefix
+ module_name + "/";
const std::string global_prefix = base_prefix + prefix;
dout(4) << __func__ << " prefix: " << global_prefix << dendl;

return with_gil(no_gil, [&] {
PyFormatter f;
for (auto p = store_cache.lower_bound(global_prefix);
p != store_cache.end() && p->first.find(global_prefix) == 0; ++p) {
f.dump_string(p->first.c_str() + base_prefix.size(), p->second);
}
return f.get();
});
PyFormatter f;
for (auto p = store_cache.lower_bound(global_prefix);
p != store_cache.end() && p->first.find(global_prefix) == 0; ++p) {
f.dump_string(p->first.c_str() + base_prefix.size(), p->second);
}
return f.get();
}

void ActivePyModules::set_store(const std::string &module_name,
Expand Down Expand Up @@ -1090,7 +1102,7 @@ PyObject *ActivePyModules::get_foreign_config(
cmd.wait();
dout(10) << "ceph_foreign_option_get (mon command) " << who << " " << name << " = "
<< cmd.outbl.to_str() << dendl;
with_gil_t gil(no_gil);
no_gil.acquire_gil();
return get_python_typed_option_value(opt->type, cmd.outbl.to_str());
}

Expand Down Expand Up @@ -1150,7 +1162,7 @@ PyObject *ActivePyModules::get_foreign_config(
dout(10) << "ceph_foreign_option_get (configmap) " << who << " " << name << " = "
<< value << dendl;
lock.unlock();
with_gil_t with_gil(no_gil);
no_gil.acquire_gil();
return get_python_typed_option_value(opt->type, value);
}

Expand Down
2 changes: 1 addition & 1 deletion src/mgr/Gil.h
View file
Expand Up @@ -86,9 +86,9 @@ class Gil {
struct without_gil_t {
without_gil_t();
~without_gil_t();
private:
void release_gil();
void acquire_gil();
private:
PyThreadState *save = nullptr;
friend struct with_gil_t;
};
Expand Down

0 comments on commit 60e0ea0

Please sign in to comment.