WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH RFC v2 09/13] libxl: introduce lock in libxl_ctx
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Mon, 31 Oct 2011 10:03:02 +0000
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 31 Oct 2011 03:06:26 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1319827031-15395-10-git-send-email-ian.jackson@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1319827031-15395-1-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-2-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-3-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-4-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-5-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-6-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-7-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-8-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-9-git-send-email-ian.jackson@xxxxxxxxxxxxx> <1319827031-15395-10-git-send-email-ian.jackson@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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

<Prev in Thread] Current Thread [Next in Thread>