From: S.Çağlar Onur Date: Fri, 1 Nov 2013 20:16:10 +0000 (-0400) Subject: valgrind drd tool shows conflicting stores happening at lxc_global_config_value@src... X-Git-Tag: lxc-1.0.0.alpha3~35 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=052616ebc639aa865c9da3805227cd46618346b6;p=thirdparty%2Flxc.git valgrind drd tool shows conflicting stores happening at lxc_global_config_value@src/lxc/utils.c (v2) Conflict occurs between following lines [...] 269 if (values[i]) 270 return values[i]; [...] and [...] 309 /* could not find value, use default */ 310 values[i] = (*ptr)[1]; [...] fix it using a specific lock dedicated to that problem as Serge suggested. Also introduce a new autoconf parameter (--enable-mutex-debugging) to convert mutexes to error reporting type and to provide a stacktrace when locking fails. Signed-off-by: S.Çağlar Onur Signed-off-by: Serge Hallyn --- diff --git a/configure.ac b/configure.ac index 9fedf55a1..6004b356b 100644 --- a/configure.ac +++ b/configure.ac @@ -178,6 +178,15 @@ AM_COND_IF([ENABLE_PYTHON], PKG_CHECK_MODULES([PYTHONDEV], [python3 >= 3.2],[],[AC_MSG_ERROR([You must install python3-dev])]) AC_DEFINE_UNQUOTED([ENABLE_PYTHON], 1, [Python3 is available])]) +# Enable dumping stack traces +AC_ARG_ENABLE([mutex-debugging], + [AC_HELP_STRING([--enable-mutex-debugging], [Makes mutexes to report error and provide stack trace])], + [enable_mutex_debugging=yes], [enable_mutex_debugging=no]) +AM_CONDITIONAL([MUTEX_DEBUGGING], [test "x$enable_mutex_debugging" = "xyes"]) + +AM_COND_IF([MUTEX_DEBUGGING], + AC_DEFINE_UNQUOTED([MUTEX_DEBUGGING], 1, [Enabling mutex debugging])) + # Not in older autoconf versions # AS_VAR_COPY(DEST, SOURCE) # ------------------------- diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c index 01ed04092..8be0ebf42 100644 --- a/src/lxc/cgroup.c +++ b/src/lxc/cgroup.c @@ -91,7 +91,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta() int saved_errno; errno = 0; - cgroup_use = lxc_global_config_value("cgroup.use"); + cgroup_use = default_cgroup_use(); if (!cgroup_use && errno != 0) return NULL; if (cgroup_use) { diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c index d403bcc16..3857ff06c 100644 --- a/src/lxc/lxclock.c +++ b/src/lxc/lxclock.c @@ -18,15 +18,15 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ -#include +#define _GNU_SOURCE #include "lxclock.h" #include #include #include #include #include -#define _GNU_SOURCE #include +#include #include #include #include @@ -38,7 +38,11 @@ lxc_log_define(lxc_lock, lxc); +#ifdef MUTEX_DEBUGGING +pthread_mutex_t thread_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP; +#else pthread_mutex_t thread_mutex = PTHREAD_MUTEX_INITIALIZER; +#endif static char *lxclock_name(const char *p, const char *n) { @@ -267,13 +271,20 @@ void process_lock(void) if ((ret = pthread_mutex_lock(&thread_mutex)) != 0) { ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret)); + dump_stacktrace(); exit(1); } } void process_unlock(void) { - pthread_mutex_unlock(&thread_mutex); + int ret; + + if ((ret = pthread_mutex_unlock(&thread_mutex)) != 0) { + ERROR("pthread_mutex_unlock returned:%d %s", ret, strerror(ret)); + dump_stacktrace(); + exit(1); + } } int container_mem_lock(struct lxc_container *c) diff --git a/src/lxc/start.c b/src/lxc/start.c index 1cadc0963..3b2ba8fbd 100644 --- a/src/lxc/start.c +++ b/src/lxc/start.c @@ -695,7 +695,7 @@ int lxc_spawn(struct lxc_handler *handler) * default value is available */ if (getuid() == 0) - cgroup_pattern = lxc_global_config_value("cgroup.pattern"); + cgroup_pattern = default_cgroup_pattern(); if (!cgroup_pattern) cgroup_pattern = "%n"; diff --git a/src/lxc/utils.c b/src/lxc/utils.c index 9e2e32614..590482e0b 100644 --- a/src/lxc/utils.c +++ b/src/lxc/utils.c @@ -21,7 +21,8 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ -#define _GNU_SOURCE +#include "config.h" + #include #include #include @@ -38,6 +39,8 @@ #include #include #include +#include +#include #ifndef HAVE_GETLINE #ifdef HAVE_FGETLN @@ -49,8 +52,61 @@ #include "log.h" #include "lxclock.h" +#define MAX_STACKDEPTH 25 + lxc_log_define(lxc_utils, lxc); + +#ifdef MUTEX_DEBUGGING +static pthread_mutex_t static_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP; + +inline void dump_stacktrace(void) +{ + void *array[MAX_STACKDEPTH]; + size_t size; + char **strings; + size_t i; + + size = backtrace(array, MAX_STACKDEPTH); + strings = backtrace_symbols(array, size); + + // Using fprintf here as our logging module is not thread safe + fprintf(stderr, "\tObtained %zd stack frames.\n", size); + + for (i = 0; i < size; i++) + fprintf(stderr, "\t\t%s\n", strings[i]); + + free (strings); +} +#else +static pthread_mutex_t static_mutex = PTHREAD_MUTEX_INITIALIZER; + +inline void dump_stacktrace(void) {;} +#endif + +/* Protects static const values inside the lxc_global_config_value funtion */ +static void static_lock(void) +{ + int ret; + + if ((ret = pthread_mutex_lock(&static_mutex)) != 0) { + ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret)); + dump_stacktrace(); + exit(1); + } +} + +static void static_unlock(void) +{ + int ret; + + if ((ret = pthread_mutex_unlock(&static_mutex)) != 0) { + ERROR("pthread_mutex_unlock returned:%d %s", ret, strerror(ret)); + dump_stacktrace(); + exit(1); + } +} + static int _recursive_rmdir_onedev(char *dirname, dev_t pdev) { struct dirent dirent, *direntp; @@ -252,8 +308,10 @@ const char *lxc_global_config_value(const char *option_name) { "cgroup.use", NULL }, { NULL, NULL }, }; + /* Protected by a mutex to eliminate conflicting load and store operations */ static const char *values[sizeof(options) / sizeof(options[0])] = { 0 }; const char *(*ptr)[2]; + const char *value; size_t i; char buf[1024], *p, *p2; FILE *fin = NULL; @@ -266,8 +324,14 @@ const char *lxc_global_config_value(const char *option_name) errno = EINVAL; return NULL; } - if (values[i]) - return values[i]; + + static_lock(); + if (values[i]) { + value = values[i]; + static_unlock(); + return value; + } + static_unlock(); process_lock(); fin = fopen_cloexec(LXC_GLOBAL_CONF, "r"); @@ -304,24 +368,31 @@ const char *lxc_global_config_value(const char *option_name) while (*p && (*p == ' ' || *p == '\t')) p++; if (!*p) continue; + static_lock(); values[i] = copy_global_config_value(p); + static_unlock(); goto out; } } /* could not find value, use default */ + static_lock(); values[i] = (*ptr)[1]; /* special case: if default value is NULL, * and there is no config, don't view that * as an error... */ if (!values[i]) errno = 0; + static_unlock(); out: process_lock(); if (fin) fclose(fin); process_unlock(); - return values[i]; + static_lock(); + value = values[i]; + static_unlock(); + return value; } const char *default_lvm_vg(void) @@ -338,11 +409,22 @@ const char *default_zfs_root(void) { return lxc_global_config_value("zfsroot"); } + const char *default_lxc_path(void) { return lxc_global_config_value("lxcpath"); } +const char *default_cgroup_use(void) +{ + return lxc_global_config_value("cgroup.use"); +} + +const char *default_cgroup_pattern(void) +{ + return lxc_global_config_value("cgroup.pattern"); +} + const char *get_rundir() { const char *rundir; diff --git a/src/lxc/utils.h b/src/lxc/utils.h index fc4676096..9c47560cf 100644 --- a/src/lxc/utils.h +++ b/src/lxc/utils.h @@ -49,7 +49,8 @@ extern const char *default_lxc_path(void); extern const char *default_zfs_root(void); extern const char *default_lvm_vg(void); extern const char *default_lvm_thin_pool(void); - +extern const char *default_cgroup_use(void); +extern const char *default_cgroup_pattern(void); /* Define getline() if missing from the C library */ #ifndef HAVE_GETLINE #ifdef HAVE_FGETLN @@ -239,4 +240,6 @@ extern size_t lxc_array_len(void **array); extern void **lxc_dup_array(void **array, lxc_dup_fn element_dup_fn, lxc_free_fn element_free_fn); extern void **lxc_append_null_to_array(void **array, size_t count); + +extern void dump_stacktrace(void); #endif