Re: [Xen-devel] [PATCH RFC v2 09/13] libxl: introduce lock in libxl_ctx

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2011-10-28 at 19:37 +0100, Ian Jackson wrote:
> This lock will be used to protect data structures which will be hung
> off the libxl_ctx in subsequent patches.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl.c          |    3 +++
>  tools/libxl/libxl_internal.h |   16 ++++++++++++++++
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 5d448af..a3c9807 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -40,6 +40,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version, 
> xentoollog_logger * lg)
>  {
>      libxl_ctx *ctx;
>      struct stat stat_buf;
> +    const pthread_mutex_t mutex_value = 
> PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
>  
>      if (version != LIBXL_VERSION)
>          return ERROR_VERSION;
> @@ -53,6 +54,8 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version, 
> xentoollog_logger * lg)
>      memset(ctx, 0, sizeof(libxl_ctx));
>      ctx->lg = lg;
>  
> +    memcpy(&ctx->lock, &mutex_value, sizeof(ctx->lock));

Is this subtly different to
       ctx->lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
in some way I'm missing?

> +
>      if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon 
> running?\n"
>                       "failed to stat %s", XENSTORE_PID_FILE);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6d9da2c..79a9de4 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -23,6 +23,7 @@
>  #include <stdarg.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <pthread.h>
>  
>  #include <xs.h>
>  #include <xenctrl.h>
> @@ -95,6 +96,9 @@ struct libxl__ctx {
>      xc_interface *xch;
>      struct xs_handle *xsh;
>  
> +    pthread_mutex_t lock; /* protects data structures hanging off the ctx */
> +      /* always use MUTEX_LOCK and MUTEX_UNLOCK to manipulate this */

MUTEX is something of an implementation detail (although I admit we are
unlikely to use anything else), perhaps CTX_(UN)LOCK?

Perhaps give the variable a less obvious name to help discourage direct
use?

> +
>      /* for callers who reap children willy-nilly; caller must only
>       * set this after libxl_init and before any other call - or
>       * may leave them untouched */
> @@ -577,6 +581,18 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc 
> *gc, const char *s);
>  #define CTX           libxl__gc_owner(gc)
>  
> 
> +#define MUTEX_LOCK do {                                 \
> +        int mutex_r = pthread_mutex_lock(&CTX->lock);   \
> +        assert(!mutex_r);                               \

This assert is to catch EINVAL ("the mutex has not been properly
initialized") rather than EDEADLK ("the mutex is already locked by the
calling thread") since we asked for a non-error-checking recursive lock?

Since it is OK to take this lock recursively then it might be as well to
say so explicitly?

This is the first lock in libxl so I guess there isn't much of a locking
hierarchy yet. Are there any particular considerations which a caller
must make wrt its own locking?

> +    } while(0)
> +
> +#define MUTEX_UNLOCK do {                               \
> +        int mutex_r = pthread_mutex_unlock(&CTX->lock); \
> +        assert(!mutex_r);                               \
> +    } while(0)
> +        
> +
> +
>  /*
>   * Inserts "elm_new" into the sorted list "head".
>   *



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel