[lxc-devel] [PATCH RFC] mutex cgmanager access
Stéphane Graber
stgraber at ubuntu.com
Fri Mar 21 14:51:25 UTC 2014
On Wed, Mar 19, 2014 at 11:55:00PM -0500, Serge Hallyn wrote:
> 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: Serge Hallyn <serge.hallyn at ubuntu.com>
If there's a chance this will make our tests more reliable, let's go for
it, we can always figure out a better way later.
Acked-by: Stéphane Graber <stgraber at ubuntu.com>
> ---
> src/lxc/cgmanager.c | 177 ++++++++++++++++++++--------------------------------
> 1 file changed, 69 insertions(+), 108 deletions(-)
>
> diff --git a/src/lxc/cgmanager.c b/src/lxc/cgmanager.c
> index 6cefacb..8a5039a 100644
> --- a/src/lxc/cgmanager.c
> +++ b/src/lxc/cgmanager.c
> @@ -66,48 +66,48 @@ struct cgm_data {
> const char *cgroup_pattern;
> };
>
> -#ifdef HAVE_TLS
> -static __thread NihDBusProxy *cgroup_manager = NULL;
> -static __thread DBusConnection *connection = NULL;
> -static __thread bool cgm_keep_connection = false;
> -
> -
> -struct cgm_key_t {
> - DBusConnection *connection;
> - NihDBusProxy *cgmanager;
> -} cgm_key;
> -
> -void destructor(void *arg) {
> - struct cgm_key_t *cgm_key = arg;
> - nih_free(cgm_key->cgmanager);
> - dbus_connection_flush(cgm_key->connection);
> - dbus_connection_close(cgm_key->connection);
> - dbus_connection_unref(cgm_key->connection);
> +static pthread_mutex_t cgm_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
> +static void lock_mutex(pthread_mutex_t *l)
> +{
> + int ret;
> +
> + if ((ret = pthread_mutex_lock(l)) != 0) {
> + fprintf(stderr, "pthread_mutex_lock returned:%d %s\n", ret, strerror(ret));
> + exit(1);
> + }
> +}
> +
> +static void unlock_mutex(pthread_mutex_t *l)
> +{
> + int ret;
> +
> + 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() {
> - pthread_key_create(&key, destructor);
> +
> +void cgm_lock(void)
> +{
> + 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;
> - 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);
> + unlock_mutex(&cgm_mutex);
> }
>
> -#else
> -static NihDBusProxy *cgroup_manager = NULL;
> -static DBusConnection *connection = NULL;
> -static bool cgm_keep_connection = false;
> -static inline void init_cgm_destructor(DBusConnection *c, NihDBusProxy *cgm) { }
> +#ifdef HAVE_PTHREAD_ATFORK
> +__attribute__((constructor))
> +static void process_lock_setup_atfork(void)
> +{
> + pthread_atfork(cgm_lock, cgm_unlock, cgm_unlock);
> +}
> #endif
>
> +static NihDBusProxy *cgroup_manager = NULL;
> +
> static struct cgroup_ops cgmanager_ops;
> static int nr_subsystems;
> static char **subsystems;
> @@ -115,24 +115,22 @@ static bool dbus_threads_initialized = false;
>
> static void cgm_dbus_disconnect(void)
> {
> - cgm_keep_connection = false;
> - if (cgroup_manager)
> - nih_free(cgroup_manager);
> - cgroup_manager = NULL;
> - if (connection) {
> - dbus_connection_flush(connection);
> - dbus_connection_close(connection);
> - dbus_connection_unref(connection);
> - }
> - connection = NULL;
> - init_cgm_destructor(NULL, NULL);
> + if (cgroup_manager) {
> + dbus_connection_flush(cgroup_manager->connection);
> + dbus_connection_close(cgroup_manager->connection);
> + nih_free(cgroup_manager);
> + }
> + cgroup_manager = NULL;
> + cgm_unlock();
> }
>
> #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;
> + static DBusConnection *connection;
>
> + cgm_lock();
> if (!dbus_threads_initialized) {
> // tell dbus to do struct locking for thread safety
> dbus_threads_init_default();
> @@ -146,6 +144,7 @@ static bool do_cgm_dbus_connect(void)
> DEBUG("Failed opening dbus connection: %s: %s",
> dbus_error.name, dbus_error.message);
> dbus_error_free(&dbus_error);
> + cgm_unlock();
> return false;
> }
> if (nih_dbus_setup(connection, NULL) < 0) {
> @@ -156,7 +155,7 @@ static bool do_cgm_dbus_connect(void)
> nih_free(nerr);
> dbus_error_free(&dbus_error);
> dbus_connection_unref(connection);
> - connection = NULL;
> + cgm_unlock();
> return false;
> }
> dbus_connection_set_exit_on_disconnect(connection, FALSE);
> @@ -164,6 +163,7 @@ static bool do_cgm_dbus_connect(void)
> cgroup_manager = nih_dbus_proxy_new(NULL, connection,
> NULL /* p2p */,
> "/org/linuxcontainers/cgmanager", NULL, NULL);
> + dbus_connection_unref(connection);
> if (!cgroup_manager) {
> NihError *nerr;
> nerr = nih_error_get();
> @@ -182,17 +182,9 @@ static bool do_cgm_dbus_connect(void)
> cgm_dbus_disconnect();
> return false;
> }
> - init_cgm_destructor(connection, cgroup_manager);
> 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)
> {
> struct msghdr msg = { 0 };
> @@ -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)
> {
> bool ret = true;
> - if (!cgm_dbus_connect()) {
> - ERROR("Error connecting to cgroup manager");
> - return false;
> - }
> if ( cgmanager_create_sync(NULL, cgroup_manager, controller,
> cgroup_path, existed) != 0) {
> NihError *nerr;
> @@ -246,21 +234,20 @@ static bool lxc_cgmanager_create(const char *controller, const char *cgroup_path
> ret = false;
> }
>
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> 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)
> {
> bool ret = true;
> pid_t me = getpid();
> int i;
>
> - if (!cgm_dbus_connect()) {
> - ERROR("Error connecting to cgroup manager");
> - return false;
> - }
> for (i = 0; i < nr_subsystems; i++) {
> if (cgmanager_move_pid_abs_sync(NULL, cgroup_manager,
> subsystems[i], "/", me) != 0) {
> @@ -274,8 +261,6 @@ static bool lxc_cgmanager_escape(void)
> }
> }
>
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> return ret;
> }
>
> @@ -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);
> }
>
> -/*
> - * 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)
> {
> struct cgm_data *d;
> @@ -459,7 +439,6 @@ static void *cgm_init(const char *name)
> cgm_dbus_disconnect();
> goto err1;
> }
> - cgm_keep_connection = true;
>
> /* if we are running as root, use system cgroup pattern, otherwise
> * just create a cgroup under the current one. But also fall back to
> @@ -470,6 +449,7 @@ static void *cgm_init(const char *name)
> d->cgroup_pattern = lxc_global_config_value("lxc.cgroup.pattern");
> if (!d->cgroup_pattern)
> d->cgroup_pattern = "%n";
> + // cgm_create immediately gets called so keep the connection open
> return d;
>
> err1:
> @@ -496,8 +476,7 @@ static void cgm_destroy(void *hdata)
> if (d->cgroup_path)
> free(d->cgroup_path);
> free(d);
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> }
>
> /*
> @@ -523,10 +502,6 @@ static inline bool cgm_create(void *hdata)
> // XXX we should send a hint to the cgmanager that when these
> // cgroups become empty they should be deleted. Requires a cgmanager
> // extension
> - if (!cgm_dbus_connect()) {
> - ERROR("Error connecting to cgroup manager");
> - return false;
> - }
>
> memset(result, 0, MAXPATHLEN);
> tmp = lxc_string_replace("%n", d->name, d->cgroup_pattern);
> @@ -569,16 +544,15 @@ again:
> goto bad;
> }
> d->cgroup_path = cgroup_path;
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return true;
> +
> next:
> cleanup_cgroups(tmp);
> index++;
> goto again;
> bad:
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return false;
> }
>
> @@ -630,8 +604,7 @@ static inline bool cgm_enter(void *hdata, pid_t pid)
> if (do_cgm_enter(pid, d->cgroup_path))
> ret = true;
> out:
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return ret;
> }
>
> @@ -675,8 +648,7 @@ static int cgm_get_nrtasks(void *hdata)
> }
> nih_free(pids);
> out:
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return pids_len;
> }
>
> @@ -711,12 +683,10 @@ static int cgm_get(const char *filename, char *value, size_t len, const char *na
> nerr = nih_error_get();
> nih_free(nerr);
> free(cgroup);
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return -1;
> }
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> free(cgroup);
> newlen = strlen(result);
> if (!value) {
> @@ -781,8 +751,7 @@ static int cgm_set(const char *filename, const char *value, const char *name, co
> return false;
> }
> ret = cgm_do_set(controller, filename, cgroup, value);
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> free(cgroup);
> return ret;
> }
> @@ -894,8 +863,7 @@ static bool cgm_unfreeze(void *hdata)
> ERROR("Error unfreezing %s", d->cgroup_path);
> ret = false;
> }
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return ret;
> }
>
> @@ -941,8 +909,7 @@ static bool cgm_setup_limits(void *hdata, struct lxc_list *cgroup_settings, bool
> ret = true;
> INFO("cgroup limits have been setup");
> out:
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return ret;
> }
>
> @@ -962,8 +929,7 @@ static bool cgm_chown(void *hdata, struct lxc_conf *conf)
> WARN("Failed to chown %s:%s to container root",
> subsystems[i], d->cgroup_path);
> }
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> + cgm_dbus_disconnect();
> return true;
> }
>
> @@ -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);
> 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
> // subsystems, so since this is a slow command over the cmd socket,
> // 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)
> ERROR("Error connecting to cgroup manager");
> 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);
>
> out:
> free(cgroup);
> lxc_container_put(c);
> - if (!cgm_keep_connection)
> - cgm_dbus_disconnect();
> return pass;
> }
>
> @@ -1083,6 +1044,6 @@ static struct cgroup_ops cgmanager_ops = {
> .attach = cgm_attach,
> .mount_cgroup = cgm_mount_cgroup,
> .nrtasks = cgm_get_nrtasks,
> - .disconnect = cgm_dbus_disconnect,
> + .disconnect = NULL,
> };
> #endif
> --
> 1.9.0
>
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140321/a9e4d734/attachment-0001.pgp>
More information about the lxc-devel
mailing list