On Tue, Mar 04, 2014 at 12:54:16PM -0600, Serge Hallyn wrote: > Ok, the following patch, on top of the original, makes the changes > you suggest. I'm still a bit wary, but tests are passing. > > From 65590d3338289565580aa19471b2e1f2392ff717 Mon Sep 17 00:00:00 2001 > From: Serge Hallyn <[email protected]> > Date: Tue, 4 Mar 2014 12:18:08 -0600 > Subject: [PATCH 1/1] cgmanager: switch to TLS > > Drop the thread mutex. Set a (TLS) boolean at container start to > indicate that the connection should be kept open; set it back to false > only when container start is complete. Every cgm_ method opens the > connection if not already open, and closes it if cgm_keep_connection > is false. > > Signed-off-by: Serge Hallyn <[email protected]>
Acked-by: Stéphane Graber <[email protected]> > --- > src/lxc/cgmanager.c | 252 > +++++++++++++++++++++++++++++----------------------- > 1 file changed, 139 insertions(+), 113 deletions(-) > > diff --git a/src/lxc/cgmanager.c b/src/lxc/cgmanager.c > index c95b3d0..534e01a 100644 > --- a/src/lxc/cgmanager.c > +++ b/src/lxc/cgmanager.c > @@ -54,28 +54,6 @@ > #ifdef HAVE_CGMANAGER > lxc_log_define(lxc_cgmanager, lxc); > > -static pthread_mutex_t thread_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); > - } > -} > - > #include <nih-dbus/dbus_connection.h> > #include <cgmanager/cgmanager-client.h> > #include <nih/alloc.h> > @@ -88,19 +66,37 @@ 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; > +#else > static NihDBusProxy *cgroup_manager = NULL; > +static DBusConnection *connection = NULL; > +static bool cgm_keep_connection = false; > +#endif > + > static struct cgroup_ops cgmanager_ops; > static int nr_subsystems; > static char **subsystems; > -static DBusConnection *connection; > + > +static void cgm_dbus_disconnect(void) > +{ > + cgm_keep_connection = false; > + if (cgroup_manager) > + nih_free(cgroup_manager); > + cgroup_manager = NULL; > + if (connection) > + dbus_connection_unref(connection); > + connection = NULL; > +} > > #define CGMANAGER_DBUS_SOCK "unix:path=/sys/fs/cgroup/cgmanager/sock" > -static bool cgm_dbus_connect(void) > +static bool do_cgm_dbus_connect(void) > { > DBusError dbus_error; > dbus_error_init(&dbus_error); > > - lock_mutex(&thread_mutex); > connection = nih_dbus_connect(CGMANAGER_DBUS_SOCK, NULL); > if (!connection) { > NihError *nerr; > @@ -109,7 +105,6 @@ static bool cgm_dbus_connect(void) > nerr->message); > nih_free(nerr); > dbus_error_free(&dbus_error); > - unlock_mutex(&thread_mutex); > return false; > } > dbus_connection_set_exit_on_disconnect(connection, FALSE); > @@ -122,7 +117,7 @@ static bool cgm_dbus_connect(void) > nerr = nih_error_get(); > ERROR("Error opening cgmanager proxy: %s", nerr->message); > nih_free(nerr); > - unlock_mutex(&thread_mutex); > + cgm_dbus_disconnect(); > return false; > } > > @@ -132,19 +127,16 @@ static bool cgm_dbus_connect(void) > nerr = nih_error_get(); > ERROR("Error pinging cgroup manager: %s", nerr->message); > nih_free(nerr); > + cgm_dbus_disconnect(); > } > return true; > } > > -static void cgm_dbus_disconnect(void) > +static bool cgm_dbus_connect(void) > { > - if (cgroup_manager) > - nih_free(cgroup_manager); > - cgroup_manager = NULL; > if (connection) > - dbus_connection_unref(connection); > - connection = NULL; > - unlock_mutex(&thread_mutex); > + return true; > + return do_cgm_dbus_connect(); > } > > static int send_creds(int sock, int rpid, int ruid, int rgid) > @@ -183,11 +175,13 @@ static int send_creds(int sock, int rpid, int ruid, int > rgid) > return 0; > } > > -/* > - * Called during container startup. The cgmanager socket is open > - */ > 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; > @@ -195,16 +189,24 @@ static bool lxc_cgmanager_create(const char > *controller, const char *cgroup_path > ERROR("call to cgmanager_create_sync failed: %s", > nerr->message); > nih_free(nerr); > ERROR("Failed to create %s:%s", controller, cgroup_path); > - return false; > + ret = false; > } > > - return true; > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > + return ret; > } > > 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) { > @@ -213,11 +215,14 @@ static bool lxc_cgmanager_escape(void) > ERROR("call to cgmanager_move_pid_abs_sync(%s) failed: > %s", > subsystems[i], nerr->message); > nih_free(nerr); > - return false; > + ret = false; > + break; > } > } > > - return true; > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > + return ret; > } > > struct chown_data { > @@ -313,9 +318,7 @@ static int chown_cgroup_wrapper(void *data) > return do_chown_cgroup(arg->controller, arg->cgroup_path, arg->origuid); > } > > -/* > - * Called during container startup. The cgmanager socket is open > - */ > +/* Internal helper. Must be called with the cgmanager dbus socket open */ > static bool lxc_cgmanager_chmod(const char *controller, > const char *cgroup_path, const char *file, int mode) > { > @@ -330,9 +333,7 @@ static bool lxc_cgmanager_chmod(const char *controller, > return true; > } > > -/* > - * Called during container startup. The cgmanager socket is open > - */ > +/* Internal helper. Must be called with the cgmanager dbus socket open */ > static bool chown_cgroup(const char *controller, const char *cgroup_path, > struct lxc_conf *conf) > { > @@ -358,10 +359,12 @@ static bool chown_cgroup(const char *controller, const > char *cgroup_path, > return false; > if (!lxc_cgmanager_chmod(controller, cgroup_path, "cgroup.procs", 0775)) > return false; > + > return true; > } > > #define CG_REMOVE_RECURSIVE 1 > +/* Internal helper. Must be called with the cgmanager dbus socket open */ > static void cgm_remove_cgroup(const char *controller, const char *path) > { > int existed; > @@ -378,8 +381,9 @@ static void cgm_remove_cgroup(const char *controller, > const char *path) > } > > /* > - * We are starting up a container. We open the cgmanager socket, and leave > it > - * open (and mutex held) for the rest of container startup. > + * 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) > { > @@ -390,13 +394,18 @@ static void *cgm_init(const char *name) > return NULL; > } > d = malloc(sizeof(*d)); > - if (!d) > + if (!d) { > + cgm_dbus_disconnect(); > return NULL; > + } > > memset(d, 0, sizeof(*d)); > d->name = strdup(name); > - if (!d->name) > + if (!d->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 > @@ -414,10 +423,7 @@ err1: > return NULL; > } > > -/* > - * Called after a failed container startup. The cgmanager socket was just > - * closed at end of lxc_spawn. We need to re-open > - */ > +/* Called after a failed container startup */ > static void cgm_destroy(void *hdata) > { > struct cgm_data *d = hdata; > @@ -436,11 +442,13 @@ static void cgm_destroy(void *hdata) > if (d->cgroup_path) > free(d->cgroup_path); > free(d); > - cgm_dbus_disconnect(); > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > } > > /* > * remove all the cgroups created > + * called internally with dbus connection open > */ > static inline void cleanup_cgroups(char *path) > { > @@ -449,9 +457,6 @@ static inline void cleanup_cgroups(char *path) > cgm_remove_cgroup(subsystems[i], path); > } > > -/* > - * Called during container startup. The cgmanager socket is open > - */ > static inline bool cgm_create(void *hdata) > { > struct cgm_data *d = hdata; > @@ -464,14 +469,18 @@ 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); > if (!tmp) > - return false; > + goto bad; > if (strlen(tmp) >= MAXPATHLEN) { > free(tmp); > - return false; > + goto bad; > } > strcpy(result, tmp); > baselen = strlen(result); > @@ -482,19 +491,19 @@ static inline bool cgm_create(void *hdata) > again: > if (index == 100) { // turn this into a warn later > ERROR("cgroup error? 100 cgroups with this name already > running"); > - return false; > + goto bad; > } > if (index) { > ret = snprintf(result+baselen, MAXPATHLEN-baselen, "-%d", > index); > if (ret < 0 || ret >= MAXPATHLEN-baselen) > - return false; > + goto bad; > } > existed = 0; > for (i = 0; i < nr_subsystems; i++) { > if (!lxc_cgmanager_create(subsystems[i], tmp, &existed)) { > ERROR("Error creating cgroup %s:%s", subsystems[i], > result); > cleanup_cgroups(tmp); > - return false; > + goto bad; > } > if (existed == 1) > goto next; > @@ -503,14 +512,20 @@ again: > cgroup_path = strdup(tmp); > if (!cgroup_path) { > cleanup_cgroups(tmp); > - return false; > + goto bad; > } > d->cgroup_path = cgroup_path; > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > return true; > next: > cleanup_cgroups(tmp); > index++; > goto again; > +bad: > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > + return false; > } > > /* > @@ -518,6 +533,8 @@ next: > * hierarchy. > * All the subsystems in this hierarchy are co-mounted, so we only > * need to transition the task into one of the cgroups > + * > + * Internal helper, must be called with cgmanager dbus socket open > */ > static bool lxc_cgmanager_enter(pid_t pid, const char *controller, > const char *cgroup_path) > @@ -533,6 +550,7 @@ static bool lxc_cgmanager_enter(pid_t pid, const char > *controller, > return true; > } > > +/* Internal helper, must be called with cgmanager dbus socket open */ > static bool do_cgm_enter(pid_t pid, const char *cgroup_path) > { > int i; > @@ -544,16 +562,23 @@ static bool do_cgm_enter(pid_t pid, const char > *cgroup_path) > return true; > } > > -/* > - * called during container startup. cgmanager socket is open > - */ > static inline bool cgm_enter(void *hdata, pid_t pid) > { > struct cgm_data *d = hdata; > + bool ret = false; > > - if (!d || !d->cgroup_path) > + if (!cgm_dbus_connect()) { > + ERROR("Error connecting to cgroup manager"); > return false; > - return do_cgm_enter(pid, d->cgroup_path); > + } > + if (!d || !d->cgroup_path) > + goto out; > + if (do_cgm_enter(pid, d->cgroup_path)) > + ret = true; > +out: > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > + return ret; > } > > static const char *cgm_get_cgroup(void *hdata, const char *subsystem) > @@ -569,6 +594,8 @@ static const char *cgm_get_cgroup(void *hdata, const char > *subsystem) > * nrtasks is called by the utmp helper by the container monitor. > * cgmanager socket was closed after cgroup setup was complete, so we need > * to reopen here. > + * > + * Return -1 on error. > */ > static int cgm_get_nrtasks(void *hdata) > { > @@ -577,11 +604,11 @@ static int cgm_get_nrtasks(void *hdata) > size_t pids_len; > > if (!d || !d->cgroup_path) > - return false; > + return -1; > > if (!cgm_dbus_connect()) { > ERROR("Error connecting to cgroup manager"); > - return false; > + return -1; > } > if (cgmanager_get_tasks_sync(NULL, cgroup_manager, subsystems[0], > d->cgroup_path, &pids, &pids_len) != 0) { > @@ -589,18 +616,17 @@ static int cgm_get_nrtasks(void *hdata) > nerr = nih_error_get(); > ERROR("call to cgmanager_get_tasks_sync failed: %s", > nerr->message); > nih_free(nerr); > - cgm_dbus_disconnect(); > - return -1; > + pids_len = -1; > + goto out; > } > nih_free(pids); > - cgm_dbus_disconnect(); > +out: > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > return pids_len; > } > > -/* > - * cgm_get is called to get container cgroup settings. cgmanager is not > - * connected. > - */ > +/* cgm_get is called to get container cgroup settings, not during startup */ > static int cgm_get(const char *filename, char *value, size_t len, const char > *name, const char *lxcpath) > { > char *result, *controller, *key, *cgroup; > @@ -619,7 +645,6 @@ static int cgm_get(const char *filename, char *value, > size_t len, const char *na > return -1; > if (!cgm_dbus_connect()) { > ERROR("Error connecting to cgroup manager"); > - free(cgroup); > return -1; > } > if (cgmanager_get_value_sync(NULL, cgroup_manager, controller, cgroup, > filename, &result) != 0) { > @@ -632,10 +657,12 @@ static int cgm_get(const char *filename, char *value, > size_t len, const char *na > nerr = nih_error_get(); > nih_free(nerr); > free(cgroup); > - cgm_dbus_disconnect(); > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > return -1; > } > - cgm_dbus_disconnect(); > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > free(cgroup); > newlen = strlen(result); > if (!value) { > @@ -657,6 +684,7 @@ static int cgm_get(const char *filename, char *value, > size_t len, const char *na > return newlen; > } > > +/* internal helper - call with cgmanager dbus connection open */ > static int cgm_do_set(const char *controller, const char *file, > const char *cgroup, const char *value) > { > @@ -673,9 +701,7 @@ static int cgm_do_set(const char *controller, const char > *file, > return ret; > } > > -/* > - * cgm_set is called to change cgroup settings. cgmanager is not connected > - */ > +/* cgm_set is called to change cgroup settings, not during startup */ > static int cgm_set(const char *filename, const char *value, const char > *name, const char *lxcpath) > { > char *controller, *key, *cgroup; > @@ -698,10 +724,11 @@ static int cgm_set(const char *filename, const char > *value, const char *name, co > if (!cgm_dbus_connect()) { > ERROR("Error connecting to cgroup manager"); > free(cgroup); > - return -1; > + return false; > } > ret = cgm_do_set(controller, filename, cgroup, value); > - cgm_dbus_disconnect(); > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > free(cgroup); > return ret; > } > @@ -710,13 +737,11 @@ static void free_subsystems(void) > { > int i; > > - lock_mutex(&thread_mutex); > for (i = 0; i < nr_subsystems; i++) > free(subsystems[i]); > free(subsystems); > subsystems = NULL; > nr_subsystems = 0; > - unlock_mutex(&thread_mutex); > } > > static bool collect_subsytems(void) > @@ -728,10 +753,8 @@ static bool collect_subsytems(void) > if (subsystems) // already initialized > return true; > > - lock_mutex(&thread_mutex); > f = fopen_cloexec("/proc/cgroups", "r"); > if (!f) { > - unlock_mutex(&thread_mutex); > return false; > } > while (getline(&line, &sz, f) != -1) { > @@ -756,8 +779,6 @@ static bool collect_subsytems(void) > } > fclose(f); > > - unlock_mutex(&thread_mutex); > - > if (!nr_subsystems) { > ERROR("No cgroup subsystems found"); > return false; > @@ -767,7 +788,6 @@ static bool collect_subsytems(void) > > out_free: > fclose(f); > - unlock_mutex(&thread_mutex); > free_subsystems(); > return false; > } > @@ -785,10 +805,8 @@ struct cgroup_ops *cgm_ops_init(void) > goto err1; > > // root; try to escape to root cgroup > - if (geteuid() == 0 && !lxc_cgmanager_escape()) { > - cgm_dbus_disconnect(); > + if (geteuid() == 0 && !lxc_cgmanager_escape()) > goto err2; > - } > cgm_dbus_disconnect(); > > return &cgmanager_ops; > @@ -800,13 +818,11 @@ err1: > return NULL; > } > > -/* > - * unfreeze is called by the command api after killing a container. > - * cgmanager is not connected. > - */ > +/* unfreeze is called by the command api after killing a container. */ > static bool cgm_unfreeze(void *hdata) > { > struct cgm_data *d = hdata; > + bool ret = true; > > if (!d || !d->cgroup_path) > return false; > @@ -822,17 +838,13 @@ static bool cgm_unfreeze(void *hdata) > ERROR("call to cgmanager_set_value_sync failed: %s", > nerr->message); > nih_free(nerr); > ERROR("Error unfreezing %s", d->cgroup_path); > - cgm_dbus_disconnect(); > - return false; > + ret = false; > } > - cgm_dbus_disconnect(); > - return true; > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > + return ret; > } > > -/* > - * setup_limits is called during startup. cgmanager is connected, and mutex > - * is held > - */ > static bool cgm_setup_limits(void *hdata, struct lxc_list *cgroup_settings, > bool do_devices) > { > struct cgm_data *d = hdata; > @@ -846,6 +858,11 @@ static bool cgm_setup_limits(void *hdata, struct > lxc_list *cgroup_settings, bool > if (!d || !d->cgroup_path) > return false; > > + if (!cgm_dbus_connect()) { > + ERROR("Error connecting to cgroup manager"); > + return false; > + } > + > lxc_list_for_each(iterator, cgroup_settings) { > char controller[100], *p; > cg = iterator->elem; > @@ -870,6 +887,8 @@ 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(); > return ret; > } > > @@ -880,11 +899,17 @@ static bool cgm_chown(void *hdata, struct lxc_conf > *conf) > > if (!d || !d->cgroup_path) > return false; > + if (!cgm_dbus_connect()) { > + ERROR("Error connecting to cgroup manager"); > + return false; > + } > for (i = 0; i < nr_subsystems; i++) { > if (!chown_cgroup(subsystems[i], d->cgroup_path, conf)) > WARN("Failed to chown %s:%s to container root", > subsystems[i], d->cgroup_path); > } > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > return true; > } > > @@ -907,9 +932,9 @@ 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 (!collect_subsytems()) { > - ERROR("Error collecting cgroup subsystems"); > - goto out; > + if (!cgm_dbus_connect()) { > + ERROR("Error connecting to cgroup manager"); > + 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, > @@ -926,11 +951,12 @@ static bool cgm_attach(const char *name, const char > *lxcpath, pid_t pid) > } > if (!(pass = do_cgm_enter(pid, cgroup))) > ERROR("Failed to enter group %s", cgroup); > - cgm_dbus_disconnect(); > > out: > free(cgroup); > lxc_container_put(c); > + if (!cgm_keep_connection) > + cgm_dbus_disconnect(); > return pass; > } > > -- > 1.9.0 > > _______________________________________________ > lxc-devel mailing list > [email protected] > http://lists.linuxcontainers.org/listinfo/lxc-devel -- Stéphane Graber Ubuntu developer http://www.ubuntu.com
signature.asc
Description: Digital signature
_______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
