remove static_lock()/static_unlock() and start to use thread local storage (v2)

While testing https://github.com/lxc/lxc/pull/106, I found that concurrent starts
are hanging time to time. I then reproduced the same problem in master and got following;

 [caglar@oOo:~] sudo gdb -p 16221
 (gdb) bt
 #0  __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
 #1  0x00007f495526515c in _L_lock_982 () from /lib/x86_64-linux-gnu/libpthread.so.0
 #2  0x00007f4955264fab in __GI___pthread_mutex_lock (mutex=0x7f49556d4600 <static_mutex>) at pthread_mutex_lock.c:64
 #3  0x00007f49554b27a6 in lock_mutex (l=l@entry=0x7f49556d4600 <static_mutex>) at lxclock.c:78
 #4  0x00007f49554b2dac in static_lock () at lxclock.c:330
 #5  0x00007f4955498f71 in lxc_global_config_value (option_name=option_name@entry=0x7f49554c02cf "cgroup.use") at utils.c:273
 #6  0x00007f495549926c in default_cgroup_use () at utils.c:366
 #7  0x00007f49554953bd in lxc_cgroup_load_meta () at cgroup.c:94
 #8  0x00007f495548debc in lxc_spawn (handler=handler@entry=0x7f49200af300) at start.c:783
 #9  0x00007f495548e7a7 in __lxc_start (name=name@entry=0x7f49200b48a0 "lxc-test-concurrent-4", conf=conf@entry=0x7f49200b2030, ops=ops@entry=0x7f49556d3900 <start_ops>, data=data@entry=0x7f495487db90,
    lxcpath=lxcpath@entry=0x7f49200b2010 "/var/lib/lxc") at start.c:951
 #10 0x00007f495548eb9c in lxc_start (name=0x7f49200b48a0 "lxc-test-concurrent-4", argv=argv@entry=0x7f495487dbe0, conf=conf@entry=0x7f49200b2030, lxcpath=0x7f49200b2010 "/var/lib/lxc") at start.c:1048
 #11 0x00007f49554b68f1 in lxcapi_start (c=0x7f49200b1dd0, useinit=<optimized out>, argv=0x7f495487dbe0) at lxccontainer.c:648
 #12 0x0000000000401317 in do_function (arguments=0x1aa80b0) at concurrent.c:94
 #13 0x0000000000401499 in concurrent (arguments=<optimized out>) at concurrent.c:130
 #14 0x00007f4955262f6e in start_thread (arg=0x7f495487e700) at pthread_create.c:311
 #15 0x00007f4954f8d9cd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:113

It looks like both parent and child end up with locked mutex thus deadlocks.

I ended up placing values in the thread local storage pool, instead of doing "unlock the lock in the child" dance

Signed-off-by: S.Çağlar Onur <caglar@10ur.org>
Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>

diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
index 64823d2a58edbcdfe4ade6e1b3b782e41bfb38a8..62e774ff9a429149b1f86ae208f94f3dd6e002db 100644 (file)
--- a/src/lxc/lxclock.c
+++ b/src/lxc/lxclock.c
@@ -44,7 +44,6 @@ lxc_log_define(lxc_lock, lxc);
 
 #ifdef MUTEX_DEBUGGING
 pthread_mutex_t thread_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
-pthread_mutex_t static_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
 
 inline void dump_stacktrace(void)
 {
@@ -66,7 +65,6 @@ inline void dump_stacktrace(void)
 }
 #else
 pthread_mutex_t thread_mutex = PTHREAD_MUTEX_INITIALIZER;
-pthread_mutex_t static_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 inline void dump_stacktrace(void) {;}
 #endif
@@ -324,17 +322,6 @@ void process_unlock(void)
        unlock_mutex(&thread_mutex);
 }
 
-/* Protects static const values inside the lxc_global_config_value funtion */
-void static_lock(void)
-{
-       lock_mutex(&static_mutex);
-}
-
-void static_unlock(void)
-{
-       unlock_mutex(&static_mutex);
-}
-
 int container_mem_lock(struct lxc_container *c)
 {
        return lxclock(c->privlock, 0);

diff --git a/src/lxc/lxclock.h b/src/lxc/lxclock.h
index 820e819acc9187a7b30518fe59c2684fe8c37452..a02a0320c6da973ffb897a1792d99505a5ef03d3 100644 (file)
--- a/src/lxc/lxclock.h
+++ b/src/lxc/lxclock.h
@@ -123,16 +123,6 @@ extern void process_lock(void);
  */
 extern void process_unlock(void);
 
-/*!
- * \brief Lock global data.
- */
-extern void static_lock(void);
-
-/*!
- * \brief Unlock global data.
- */
-extern void static_unlock(void);
-
 struct lxc_container;
 
 /*!

diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index b6053075884c42bf717aac955b36960baf40108d..785f3e6b072bb3e3e0e0248fb01f610b7ccb933a 100644 (file)
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -253,8 +253,8 @@ 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 };
+       /* placed in the thread local storage pool */
+       static __thread const char *values[sizeof(options) / sizeof(options[0])] = { 0 };
        const char *(*ptr)[2];
        const char *value;
        size_t i;
@@ -270,13 +270,10 @@ const char *lxc_global_config_value(const char *option_name)
                return NULL;
        }
 
-       static_lock();
        if (values[i]) {
                value = values[i];
-               static_unlock();
                return value;
        }
-       static_unlock();
 
        process_lock();
        fin = fopen_cloexec(LXC_GLOBAL_CONF, "r");
@@ -313,21 +310,17 @@ 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();
@@ -335,10 +328,7 @@ out:
                fclose(fin);
        process_unlock();
 
-       static_lock();
-       value = values[i];
-       static_unlock();
-       return value;
+       return values[i];
 }
 
 const char *default_lvm_vg(void)