[lxc-devel] [PATCH] remove static_lock()/static_unlock() and start to use thread local storage

[S.Çağlar Onur]

Uhh looks like git ate my commit description will resend in a minute

On Thu, Dec 19, 2013 at 12:05 AM, S.Çağlar Onur <caglar at 10ur.org> wrote:
> 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 at oOo:~] sudo gdb -p 16221
> (gdb) bt
>     lxcpath=lxcpath at entry=0x7f49200b2010 "/var/lib/lxc") at start.c:951
> (gdb)
>
> 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 at 10ur.org>
> ---
>  src/lxc/lxclock.c | 13 -------------
>  src/lxc/lxclock.h | 10 ----------
>  src/lxc/utils.c   | 16 +++-------------
>  3 files changed, 3 insertions(+), 36 deletions(-)
>
> diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
> index 64823d2..62e774f 100644
> --- 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 820e819..a02a032 100644
> --- 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 b605307..785f3e6 100644
> --- 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)
> --
> 1.8.3.2
>



-- 
S.Çağlar Onur <caglar at 10ur.org>