[lxc-devel] [PATCH] valgrind drd tool shows conflicting stores happening at lxc_global_config_value at src/lxc/utils.c (v2)
S.Çağlar Onur
caglar at 10ur.org
Fri Nov 1 20:16:10 UTC 2013
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 <caglar at 10ur.org>
---
configure.ac | 9 ++++++
src/lxc/cgroup.c | 2 +-
src/lxc/lxclock.c | 17 +++++++++--
src/lxc/start.c | 2 +-
src/lxc/utils.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
src/lxc/utils.h | 5 +++-
6 files changed, 115 insertions(+), 10 deletions(-)
diff --git a/configure.ac b/configure.ac
index 9fedf55..6004b35 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 01ed040..1e1e72a 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 d403bcc..3857ff0 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 <pthread.h>
+#define _GNU_SOURCE
#include "lxclock.h"
#include <malloc.h>
#include <stdio.h>
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>
-#define _GNU_SOURCE
#include <stdlib.h>
+#include <pthread.h>
#include <lxc/utils.h>
#include <lxc/log.h>
#include <lxc/lxccontainer.h>
@@ -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 1cadc09..58e1194 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 9e2e326..590482e 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 <errno.h>
#include <unistd.h>
#include <stdlib.h>
@@ -38,6 +39,8 @@
#include <sys/types.h>
#include <sys/wait.h>
#include <assert.h>
+#include <pthread.h>
+#include <execinfo.h>
#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 fc46760..9c47560 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
--
1.8.3.2
More information about the lxc-devel
mailing list