Skip to content

L
lxc
Commit 79bff4f8 by Serge Hallyn Committed by Stéphane Graber
Options

mutex cgmanager access

It looks like either libdbus or libnih is showing some corruption with threaded access to the cgmanager-client library. Until we can straighten that out, mutex access to the cgmanager. The worst part of this is having to take and drop the mutex at every fork. This also means that we can't keep a connection open for the duration of container startup, since that would deadlock forks. If we were going to keep it like this, then we could get rid of some code in start.c. However we take a performance hit here which I really hope we can rectify soon. The other approach we could take would be to keep a global count of references to cgroup_manager. Mutex the open, close, and each use of the cgroup_manager proxy (and the inc/dec of the refcount). This way we could in fact keep the connection open for the duration of container start. The atfork handler child_fn would have to close the connection if open. Signed-off-by: 's avatarSerge Hallyn <serge.hallyn@ubuntu.com> Acked-by: 's avatarStéphane Graber <stgraber@ubuntu.com>
parent 23e88083
Showing with 69 additions and 108 deletions
src/lxc/cgmanager.c
...@@ -66,48 +66,48 @@ struct cgm_data { ...@@ -66,48 +66,48 @@ struct cgm_data {
const char *cgroup_pattern; const char *cgroup_pattern;
}; };
#ifdef HAVE_TLS static pthread_mutex_t cgm_mutex = PTHREAD_MUTEX_INITIALIZER;
static __thread NihDBusProxy *cgroup_manager = NULL;
static __thread DBusConnection *connection = NULL; static void lock_mutex(pthread_mutex_t *l)
static __thread bool cgm_keep_connection = false; {
int ret;
struct cgm_key_t { if ((ret = pthread_mutex_lock(l)) != 0) {
DBusConnection *connection; fprintf(stderr, "pthread_mutex_lock returned:%d %s\n", ret, strerror(ret));
NihDBusProxy *cgmanager; exit(1);
} cgm_key; }
}
void destructor(void *arg) {
struct cgm_key_t *cgm_key = arg; static void unlock_mutex(pthread_mutex_t *l)
nih_free(cgm_key->cgmanager); {
dbus_connection_flush(cgm_key->connection); int ret;
dbus_connection_close(cgm_key->connection);
dbus_connection_unref(cgm_key->connection); if ((ret = pthread_mutex_unlock(l)) != 0) {
fprintf(stderr, "pthread_mutex_unlock returned:%d %s\n", ret, strerror(ret));
exit(1);
}
} }
static pthread_key_t key;
static void make_key() { void cgm_lock(void)
pthread_key_create(&key, destructor); {
lock_mutex(&cgm_mutex);
} }
static void init_cgm_destructor(DBusConnection *c, NihDBusProxy *cgm) void cgm_unlock(void)
{ {
static pthread_once_t key_once = PTHREAD_ONCE_INIT; unlock_mutex(&cgm_mutex);
pthread_once(&key_once, make_key);
cgm_key.connection = c;
cgm_key.cgmanager = cgm;
if (!cgm)
pthread_setspecific(key, NULL);
else if (pthread_getspecific(key) == NULL)
pthread_setspecific(key, &cgm_key);
} }
#else #ifdef HAVE_PTHREAD_ATFORK
static NihDBusProxy *cgroup_manager = NULL; __attribute__((constructor))
static DBusConnection *connection = NULL; static void process_lock_setup_atfork(void)
static bool cgm_keep_connection = false; {
static inline void init_cgm_destructor(DBusConnection *c, NihDBusProxy *cgm) { } pthread_atfork(cgm_lock, cgm_unlock, cgm_unlock);
}
#endif #endif
static NihDBusProxy *cgroup_manager = NULL;
static struct cgroup_ops cgmanager_ops; static struct cgroup_ops cgmanager_ops;
static int nr_subsystems; static int nr_subsystems;
static char **subsystems; static char **subsystems;
...@@ -115,24 +115,22 @@ static bool dbus_threads_initialized = false; ...@@ -115,24 +115,22 @@ static bool dbus_threads_initialized = false;
static void cgm_dbus_disconnect(void) static void cgm_dbus_disconnect(void)
{ {
cgm_keep_connection = false; if (cgroup_manager) {
if (cgroup_manager) dbus_connection_flush(cgroup_manager->connection);
nih_free(cgroup_manager); dbus_connection_close(cgroup_manager->connection);
cgroup_manager = NULL; nih_free(cgroup_manager);
if (connection) { }
dbus_connection_flush(connection); cgroup_manager = NULL;
dbus_connection_close(connection); cgm_unlock();
dbus_connection_unref(connection);
}
connection = NULL;
init_cgm_destructor(NULL, NULL);
} }
#define CGMANAGER_DBUS_SOCK "unix:path=/sys/fs/cgroup/cgmanager/sock" #define CGMANAGER_DBUS_SOCK "unix:path=/sys/fs/cgroup/cgmanager/sock"
static bool do_cgm_dbus_connect(void) static bool cgm_dbus_connect(void)
{ {
DBusError dbus_error; DBusError dbus_error;
static DBusConnection *connection;
cgm_lock();
if (!dbus_threads_initialized) { if (!dbus_threads_initialized) {
// tell dbus to do struct locking for thread safety // tell dbus to do struct locking for thread safety
dbus_threads_init_default(); dbus_threads_init_default();
...@@ -146,6 +144,7 @@ static bool do_cgm_dbus_connect(void) ...@@ -146,6 +144,7 @@ static bool do_cgm_dbus_connect(void)
DEBUG("Failed opening dbus connection: %s: %s", DEBUG("Failed opening dbus connection: %s: %s",
dbus_error.name, dbus_error.message); dbus_error.name, dbus_error.message);
dbus_error_free(&dbus_error); dbus_error_free(&dbus_error);
cgm_unlock();
return false; return false;
} }
if (nih_dbus_setup(connection, NULL) < 0) { if (nih_dbus_setup(connection, NULL) < 0) {
...@@ -156,7 +155,7 @@ static bool do_cgm_dbus_connect(void) ...@@ -156,7 +155,7 @@ static bool do_cgm_dbus_connect(void)
nih_free(nerr); nih_free(nerr);
dbus_error_free(&dbus_error); dbus_error_free(&dbus_error);
dbus_connection_unref(connection); dbus_connection_unref(connection);
connection = NULL; cgm_unlock();
return false; return false;
} }
dbus_connection_set_exit_on_disconnect(connection, FALSE); dbus_connection_set_exit_on_disconnect(connection, FALSE);
...@@ -164,6 +163,7 @@ static bool do_cgm_dbus_connect(void) ...@@ -164,6 +163,7 @@ static bool do_cgm_dbus_connect(void)
cgroup_manager = nih_dbus_proxy_new(NULL, connection, cgroup_manager = nih_dbus_proxy_new(NULL, connection,
NULL /* p2p */, NULL /* p2p */,
"/org/linuxcontainers/cgmanager", NULL, NULL); "/org/linuxcontainers/cgmanager", NULL, NULL);
dbus_connection_unref(connection);
if (!cgroup_manager) { if (!cgroup_manager) {
NihError *nerr; NihError *nerr;
nerr = nih_error_get(); nerr = nih_error_get();
...@@ -182,17 +182,9 @@ static bool do_cgm_dbus_connect(void) ...@@ -182,17 +182,9 @@ static bool do_cgm_dbus_connect(void)
cgm_dbus_disconnect(); cgm_dbus_disconnect();
return false; return false;
} }
init_cgm_destructor(connection, cgroup_manager);
return true; return true;
} }
static bool cgm_dbus_connect(void)
{
if (connection)
return true;
return do_cgm_dbus_connect();
}
static int send_creds(int sock, int rpid, int ruid, int rgid) static int send_creds(int sock, int rpid, int ruid, int rgid)
{ {
struct msghdr msg = { 0 }; struct msghdr msg = { 0 };
...@@ -232,10 +224,6 @@ static int send_creds(int sock, int rpid, int ruid, int rgid) ...@@ -232,10 +224,6 @@ static int send_creds(int sock, int rpid, int ruid, int rgid)
static bool lxc_cgmanager_create(const char *controller, const char *cgroup_path, int32_t *existed) static bool lxc_cgmanager_create(const char *controller, const char *cgroup_path, int32_t *existed)
{ {
bool ret = true; bool ret = true;
if (!cgm_dbus_connect()) {
ERROR("Error connecting to cgroup manager");
return false;
}
if ( cgmanager_create_sync(NULL, cgroup_manager, controller, if ( cgmanager_create_sync(NULL, cgroup_manager, controller,
cgroup_path, existed) != 0) { cgroup_path, existed) != 0) {
NihError *nerr; NihError *nerr;
...@@ -246,21 +234,20 @@ static bool lxc_cgmanager_create(const char *controller, const char *cgroup_path ...@@ -246,21 +234,20 @@ static bool lxc_cgmanager_create(const char *controller, const char *cgroup_path
ret = false; ret = false;
} }
if (!cgm_keep_connection)
cgm_dbus_disconnect();
return ret; return ret;
} }
/*
* Escape to the root cgroup if we are root, so that the container will
* be in "/lxc/c1" rather than "/user/..../c1"
* called internally with connection already open
*/
static bool lxc_cgmanager_escape(void) static bool lxc_cgmanager_escape(void)
{ {
bool ret = true; bool ret = true;
pid_t me = getpid(); pid_t me = getpid();
int i; int i;
if (!cgm_dbus_connect()) {
ERROR("Error connecting to cgroup manager");
return false;
}
for (i = 0; i < nr_subsystems; i++) { for (i = 0; i < nr_subsystems; i++) {
if (cgmanager_move_pid_abs_sync(NULL, cgroup_manager, if (cgmanager_move_pid_abs_sync(NULL, cgroup_manager,
subsystems[i], "/", me) != 0) { subsystems[i], "/", me) != 0) {
...@@ -274,8 +261,6 @@ static bool lxc_cgmanager_escape(void) ...@@ -274,8 +261,6 @@ static bool lxc_cgmanager_escape(void)
} }
} }
if (!cgm_keep_connection)
cgm_dbus_disconnect();
return ret; return ret;
} }
...@@ -434,11 +419,6 @@ static void cgm_remove_cgroup(const char *controller, const char *path) ...@@ -434,11 +419,6 @@ static void cgm_remove_cgroup(const char *controller, const char *path)
INFO("cgroup removal attempt: %s:%s did not exist", controller, path); INFO("cgroup removal attempt: %s:%s did not exist", controller, path);
} }
/*
* We are starting up a container. We open the cgmanager socket, and set
* cgm_keep_connection to true so that helpers will keep the connection
* open.
*/
static void *cgm_init(const char *name) static void *cgm_init(const char *name)
{ {
struct cgm_data *d; struct cgm_data *d;
...@@ -459,7 +439,6 @@ static void *cgm_init(const char *name) ...@@ -459,7 +439,6 @@ static void *cgm_init(const char *name)
cgm_dbus_disconnect(); cgm_dbus_disconnect();
goto err1; goto err1;
} }
cgm_keep_connection = true;
/* if we are running as root, use system cgroup pattern, otherwise /* if we are running as root, use system cgroup pattern, otherwise
* just create a cgroup under the current one. But also fall back to * just create a cgroup under the current one. But also fall back to
...@@ -470,6 +449,7 @@ static void *cgm_init(const char *name) ...@@ -470,6 +449,7 @@ static void *cgm_init(const char *name)
d->cgroup_pattern = lxc_global_config_value("lxc.cgroup.pattern"); d->cgroup_pattern = lxc_global_config_value("lxc.cgroup.pattern");
if (!d->cgroup_pattern) if (!d->cgroup_pattern)
d->cgroup_pattern = "%n"; d->cgroup_pattern = "%n";
// cgm_create immediately gets called so keep the connection open
return d; return d;
err1: err1:
...@@ -496,8 +476,7 @@ static void cgm_destroy(void *hdata) ...@@ -496,8 +476,7 @@ static void cgm_destroy(void *hdata)
if (d->cgroup_path) if (d->cgroup_path)
free(d->cgroup_path); free(d->cgroup_path);
free(d); free(d);
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
} }
/* /*
...@@ -523,10 +502,6 @@ static inline bool cgm_create(void *hdata) ...@@ -523,10 +502,6 @@ static inline bool cgm_create(void *hdata)
// XXX we should send a hint to the cgmanager that when these // XXX we should send a hint to the cgmanager that when these
// cgroups become empty they should be deleted. Requires a cgmanager // cgroups become empty they should be deleted. Requires a cgmanager
// extension // extension
if (!cgm_dbus_connect()) {
ERROR("Error connecting to cgroup manager");
return false;
}
memset(result, 0, MAXPATHLEN); memset(result, 0, MAXPATHLEN);
tmp = lxc_string_replace("%n", d->name, d->cgroup_pattern); tmp = lxc_string_replace("%n", d->name, d->cgroup_pattern);
...@@ -569,16 +544,15 @@ again: ...@@ -569,16 +544,15 @@ again:
goto bad; goto bad;
} }
d->cgroup_path = cgroup_path; d->cgroup_path = cgroup_path;
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return true; return true;
next: next:
cleanup_cgroups(tmp); cleanup_cgroups(tmp);
index++; index++;
goto again; goto again;
bad: bad:
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return false; return false;
} }
...@@ -630,8 +604,7 @@ static inline bool cgm_enter(void *hdata, pid_t pid) ...@@ -630,8 +604,7 @@ static inline bool cgm_enter(void *hdata, pid_t pid)
if (do_cgm_enter(pid, d->cgroup_path)) if (do_cgm_enter(pid, d->cgroup_path))
ret = true; ret = true;
out: out:
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return ret; return ret;
} }
...@@ -675,8 +648,7 @@ static int cgm_get_nrtasks(void *hdata) ...@@ -675,8 +648,7 @@ static int cgm_get_nrtasks(void *hdata)
} }
nih_free(pids); nih_free(pids);
out: out:
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return pids_len; return pids_len;
} }
...@@ -711,12 +683,10 @@ static int cgm_get(const char *filename, char *value, size_t len, const char *na ...@@ -711,12 +683,10 @@ static int cgm_get(const char *filename, char *value, size_t len, const char *na
nerr = nih_error_get(); nerr = nih_error_get();
nih_free(nerr); nih_free(nerr);
free(cgroup); free(cgroup);
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return -1; return -1;
} }
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
free(cgroup); free(cgroup);
newlen = strlen(result); newlen = strlen(result);
if (!value) { if (!value) {
...@@ -781,8 +751,7 @@ static int cgm_set(const char *filename, const char *value, const char *name, co ...@@ -781,8 +751,7 @@ static int cgm_set(const char *filename, const char *value, const char *name, co
return false; return false;
} }
ret = cgm_do_set(controller, filename, cgroup, value); ret = cgm_do_set(controller, filename, cgroup, value);
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
free(cgroup); free(cgroup);
return ret; return ret;
} }
...@@ -894,8 +863,7 @@ static bool cgm_unfreeze(void *hdata) ...@@ -894,8 +863,7 @@ static bool cgm_unfreeze(void *hdata)
ERROR("Error unfreezing %s", d->cgroup_path); ERROR("Error unfreezing %s", d->cgroup_path);
ret = false; ret = false;
} }
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return ret; return ret;
} }
...@@ -941,8 +909,7 @@ static bool cgm_setup_limits(void *hdata, struct lxc_list *cgroup_settings, bool ...@@ -941,8 +909,7 @@ static bool cgm_setup_limits(void *hdata, struct lxc_list *cgroup_settings, bool
ret = true; ret = true;
INFO("cgroup limits have been setup"); INFO("cgroup limits have been setup");
out: out:
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return ret; return ret;
} }
...@@ -962,8 +929,7 @@ static bool cgm_chown(void *hdata, struct lxc_conf *conf) ...@@ -962,8 +929,7 @@ static bool cgm_chown(void *hdata, struct lxc_conf *conf)
WARN("Failed to chown %s:%s to container root", WARN("Failed to chown %s:%s to container root",
subsystems[i], d->cgroup_path); subsystems[i], d->cgroup_path);
} }
if (!cgm_keep_connection) cgm_dbus_disconnect();
cgm_dbus_disconnect();
return true; return true;
} }
...@@ -986,11 +952,6 @@ static bool cgm_attach(const char *name, const char *lxcpath, pid_t pid) ...@@ -986,11 +952,6 @@ static bool cgm_attach(const char *name, const char *lxcpath, pid_t pid)
ERROR("Could not load container %s:%s", lxcpath, name); ERROR("Could not load container %s:%s", lxcpath, name);
return false; return false;
} }
if (!cgm_dbus_connect()) {
ERROR("Error connecting to cgroup manager");
lxc_container_put(c);
return false;
}
// cgm_create makes sure that we have the same cgroup name for all // cgm_create makes sure that we have the same cgroup name for all
// subsystems, so since this is a slow command over the cmd socket, // subsystems, so since this is a slow command over the cmd socket,
// just get the cgroup name for the first one. // just get the cgroup name for the first one.
...@@ -1004,14 +965,14 @@ static bool cgm_attach(const char *name, const char *lxcpath, pid_t pid) ...@@ -1004,14 +965,14 @@ static bool cgm_attach(const char *name, const char *lxcpath, pid_t pid)
ERROR("Error connecting to cgroup manager"); ERROR("Error connecting to cgroup manager");
goto out; goto out;
} }
if (!(pass = do_cgm_enter(pid, cgroup))) pass = do_cgm_enter(pid, cgroup);
cgm_dbus_disconnect();
if (!pass)
ERROR("Failed to enter group %s", cgroup); ERROR("Failed to enter group %s", cgroup);
out: out:
free(cgroup); free(cgroup);
lxc_container_put(c); lxc_container_put(c);
if (!cgm_keep_connection)
cgm_dbus_disconnect();
return pass; return pass;
} }
...@@ -1083,6 +1044,6 @@ static struct cgroup_ops cgmanager_ops = { ...@@ -1083,6 +1044,6 @@ static struct cgroup_ops cgmanager_ops = {
.attach = cgm_attach, .attach = cgm_attach,
.mount_cgroup = cgm_mount_cgroup, .mount_cgroup = cgm_mount_cgroup,
.nrtasks = cgm_get_nrtasks, .nrtasks = cgm_get_nrtasks,
.disconnect = cgm_dbus_disconnect, .disconnect = NULL,
}; };
#endif #endif
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment