Re: [Xen-devel] [PATCH 15/15] libxl: New event generation API

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 12 Dec 2011, Ian Jackson wrote:
> > > It's probably not so important for memory allocation cleanup but it
> > > would be a semantically useful change for the callbacks if we could
> > > arrange for them to happen only on final exit of the library -- exactly
> > > because of the difficulty of reasoning about things otherwise. Alas I
> > > can't think of a good way to do this since the ctx can be shared, thread
> > > local storage would be one or libxl__gc_owner could return a special
> > > "nested ctx" instead of the real outer ctx, but, ick.
> > 
> > I think that using TLS to increase/decrease a nesting counter would OK
> > in this case. We could have a libxl__enter and a libxl__exit function
> > that take care it this.
> > 
> > Modifying libxl__gc_owner could lead to errors because it doesn't handle
> > the case in which an externally visible function calls another
> > externally visible function: libxl__gc_owner wouldn't even be called in
> > this case.
> 
> I suggested to Ian the moral equivalent of:
> 
> #define CTX_LOCK \
>      <previous implementation> \
>      ctx->recursive_lock_counter++
> 
> #define GC_INIT \
>      CTX_LOCK; \
>      assert(ctx->recursive_lock_counter==1); \
>      CTX_UNLOCK
> 
> but I don't think this is really very nice.
> 
> 
> I think the answer is to write a comment saying that it is forbidden
> to allocate a new gc with the ctx locked.
 
Why not handle the case correctly in the first place?
It is certainly better than an assert or a comment, don't you think?
Ideally the code should be easy enough to understand without a comment.
This is what I have in mind:


diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cd613f7..94bdbba 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -63,6 +63,12 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
      * only as an initialiser, not as an expression. */
     memcpy(&ctx->lock, &mutex_value, sizeof(ctx->lock));
 
+    if(pthread_key_create(&ctx->tls_key, NULL) < 0) {
+        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
+            "cannot create tls key");
+        return ERROR_FAIL;
+    }
+
     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.c b/tools/libxl/libxl_internal.c
index 34edaf3..eef3815 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -19,6 +19,7 @@
 #include <stdarg.h>
 #include <string.h>
 
+#include <pthread.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
@@ -65,17 +66,41 @@ int libxl__ptr_add(libxl__gc *gc, void *ptr)
     return 0;
 }
 
+libxl__gc *libxl__init_gc(libxl_ctx *ctx)
+{
+    libxl__gc *gc = (libxl__gc *) pthread_getspecific(ctx->tls_key);
+    if (gc == NULL) {
+        gc = (libxl__gc *) malloc(sizeof(libxl__gc));
+        if (gc == NULL)
+            return NULL;
+        gc->alloc_maxsize = 0;
+        gc->alloc_ptrs = 0;
+        gc->owner = ctx;
+        gc->nested = 1;
+        pthread_setspecific(ctx->tls_key, gc);
+    } else {
+        gc->nested++;
+    }
+    return gc;
+}
+
 void libxl__free_all(libxl__gc *gc)
 {
     void *ptr;
     int i;
 
+    gc->nested--;
+    if (gc->nested > 0)
+        return;
+
     for (i = 0; i < gc->alloc_maxsize; i++) {
         ptr = gc->alloc_ptrs[i];
         gc->alloc_ptrs[i] = NULL;
         free(ptr);
     }
     free(gc->alloc_ptrs);
+    pthread_setspecific(CTX->tls_key, NULL);
+    free(gc);
 }
 
 void *libxl__zalloc(libxl__gc *gc, int bytes)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b479c49..50e6b33 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -91,6 +91,8 @@ struct libxl__ctx {
     xc_interface *xch;
     struct xs_handle *xsh;
 
+    pthread_key_t tls_key;
+
     pthread_mutex_t lock; /* protects data structures hanging off the ctx */
       /* Always use CTX_LOCK and CTX_UNLOCK to manipulate this.
        *
@@ -138,9 +140,9 @@ typedef struct {
     int alloc_maxsize;
     void **alloc_ptrs;
     libxl_ctx *owner;
+    int nested;
 } libxl__gc;
 
-#define LIBXL_INIT_GC(ctx) (libxl__gc){ .alloc_maxsize = 0, .alloc_ptrs = 0, 
.owner = ctx }
 static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
 {
     return gc->owner;
@@ -670,7 +672,8 @@ libxl__device_model_version_running(libxl__gc *gc, uint32_t 
domid);
  * as a local variable.
  */
 
-#define GC_INIT(ctx)  libxl__gc gc[1] = { LIBXL_INIT_GC(ctx) }
+_hidden libxl__gc *libxl__init_gc(struct libxl__ctx *ctx);
+#define GC_INIT(ctx)  libxl__gc *gc = libxl__init_gc(ctx); if (gc == NULL) 
return ERROR_NOMEM;
 #define GC_FREE       libxl__free_all(gc)
 #define CTX           libxl__gc_owner(gc)
 
---

I like this approach because it uses malloc for gc allocation so it can
be reused for your following patches that need a gc that lives out of
the function. This way we don't have two special cases but a single
approach that works everywhere and another comment that probably doesn't
need to be read anymore.
I also think that it makes the code easier to read and understand.

However in case you would like to see how would this patch look, keeping
the gc allocation on the stack, I have appended a version of it that
uses alloca instead of malloc.



P.S.
please note that the inline patch as it is doesn't compile because some libxl
function don't return an error but they return a pointer, something that
should be changed anyway.

Attachment: 2
Description: Text document

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