[PATCH] libxc: reset completed flag in restore_ctx

[PATCH] libxc: reset completed flag in restore_ctx

[Jim Fehlig]

# HG changeset patch
# User Jim Fehlig <jfehlig@novell.com>
# Date 1306191873 21600
# Node ID f94242f20cdaee81d28f68df38d5a98f8fd9947d
# Parent  fb517cc27adef3a7ad548e7397e02e1414132ead
libxc: reset completed flag in restore_ctx

Long running libxl/libxc apps such as libvirt segfault when
attempting multiple restores.  The completed flag in restore_ctx
structure is set at completion of first restore.  Subsequent
restores do not load any pages and result in the segfault.

Reset completed flag at start of restore.

    Signed-off-by: Jim Fehlig <jfehlig@novell.com>

diff -r fb517cc27ade -r f94242f20cda tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c	Fri May 20 18:20:09 2011 +0100
+++ b/tools/libxc/xc_domain_restore.c	Mon May 23 17:04:33 2011 -0600
@@ -1146,6 +1146,7 @@ int xc_domain_restore(xc_interface *xch,
 
     /* For info only */
     ctx->nr_pfns = 0;
+    ctx->completed = 0;
 
     if ( superpages )
         return 1;

Re: [PATCH] libxc: reset completed flag in restore_ctx

[Ian Campbell]

On Tue, 2011-05-24 at 00:05 +0100, Jim Fehlig wrote:
> # HG changeset patch
> # User Jim Fehlig <jfehlig@novell.com>
> # Date 1306191873 21600
> # Node ID f94242f20cdaee81d28f68df38d5a98f8fd9947d
> # Parent  fb517cc27adef3a7ad548e7397e02e1414132ead
> libxc: reset completed flag in restore_ctx
> 
> Long running libxl/libxc apps such as libvirt segfault when
> attempting multiple restores.  The completed flag in restore_ctx
> structure is set at completion of first restore.  Subsequent
> restores do not load any pages and result in the segfault.
> 
> Reset completed flag at start of restore.

Seems reasonable. However, we have:
    static struct restore_ctx _ctx = {
        .live_p2m = NULL,
        .p2m = NULL,
    };
    static struct restore_ctx *ctx = &_ctx;
    [...]
    /* For info only */
    ctx->nr_pfns = 0;

i.e. we initialise the different subsets of the fields in two separate
places. Perhaps we should just add a memset?

BTW, those static variables are pretty disgusting and are going to cause
trouble for users which deal with >1 domain at a time.

It's not clear that there is any reason for it to be a static variable
anyway. It comes from 20545:cc7d66ba0dad which says: "pass the
restore_context through function and allocate the context on the restore
function stack." but, presumably by mistake, retains the static
modifiers from the global variable. The same is true of the save side.

So how about this instead (untested but seemingly sane...):

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1306224654 -3600
# Node ID 13599c2c82d8bdbfc73611fed9a1bc2aebfa275c
# Parent  9cf8d38260606de37826b76334028114feff6131
libxc: xc_domain_{save,restore}: remove static variables

20544:ad9d75d74bd5 and 20545:cc7d66ba0dad seemingly intended to change these
global static variables into stack variables but didn't remove the static
qualifier.

Also zero the entire struct once with memset rather than clearing fields
piecemeal in two different places.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 9cf8d3826060 -r 13599c2c82d8 tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c	Tue May 24 09:02:05 2011 +0100
+++ b/tools/libxc/xc_domain_restore.c	Tue May 24 09:10:54 2011 +0100
@@ -1133,23 +1133,19 @@ int xc_domain_restore(xc_interface *xch,
 
     int orig_io_fd_flags;
 
-    static struct restore_ctx _ctx = {
-        .live_p2m = NULL,
-        .p2m = NULL,
-    };
-    static struct restore_ctx *ctx = &_ctx;
+    struct restore_ctx _ctx;
+    struct restore_ctx *ctx = &_ctx;
     struct domain_info_context *dinfo = &ctx->dinfo;
 
     pagebuf_init(&pagebuf);
     memset(&tailbuf, 0, sizeof(tailbuf));
     tailbuf.ishvm = hvm;
 
-    /* For info only */
-    ctx->nr_pfns = 0;
-
     if ( superpages )
         return 1;
 
+    memset(ctx, 0, sizeof(*ctx));
+
     ctxt = xc_hypercall_buffer_alloc(xch, ctxt, sizeof(*ctxt));
 
     if ( ctxt == NULL )
diff -r 9cf8d3826060 -r 13599c2c82d8 tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c	Tue May 24 09:02:05 2011 +0100
+++ b/tools/libxc/xc_domain_save.c	Tue May 24 09:10:54 2011 +0100
@@ -958,11 +958,8 @@ int xc_domain_save(xc_interface *xch, in
     unsigned long mfn;
 
     struct outbuf ob;
-    static struct save_ctx _ctx = {
-        .live_p2m = NULL,
-        .live_m2p = NULL,
-    };
-    static struct save_ctx *ctx = &_ctx;
+    struct save_ctx _ctx;
+    struct save_ctx *ctx = &_ctx;
     struct domain_info_context *dinfo = &ctx->dinfo;
 
     int completed = 0;
@@ -976,6 +973,8 @@ int xc_domain_save(xc_interface *xch, in
 
     outbuf_init(xch, &ob, OUTBUF_SIZE);
 
+    memset(ctx, 0, sizeof(*ctx));
+
     /* If no explicit control parameters given, use defaults */
     max_iters  = max_iters  ? : DEF_MAX_ITERS;
     max_factor = max_factor ? : DEF_MAX_FACTOR;

Re: [PATCH] libxc: reset completed flag in restore_ctx

[Jim Fehlig]

Ian Campbell wrote:
> On Tue, 2011-05-24 at 00:05 +0100, Jim Fehlig wrote:
>   
>> # HG changeset patch
>> # User Jim Fehlig <jfehlig@novell.com>
>> # Date 1306191873 21600
>> # Node ID f94242f20cdaee81d28f68df38d5a98f8fd9947d
>> # Parent  fb517cc27adef3a7ad548e7397e02e1414132ead
>> libxc: reset completed flag in restore_ctx
>>
>> Long running libxl/libxc apps such as libvirt segfault when
>> attempting multiple restores.  The completed flag in restore_ctx
>> structure is set at completion of first restore.  Subsequent
>> restores do not load any pages and result in the segfault.
>>
>> Reset completed flag at start of restore.
>>     
>
> Seems reasonable. However, we have:
>     static struct restore_ctx _ctx = {
>         .live_p2m = NULL,
>         .p2m = NULL,
>     };
>     static struct restore_ctx *ctx = &_ctx;
>     [...]
>     /* For info only */
>     ctx->nr_pfns = 0;
>
> i.e. we initialise the different subsets of the fields in two separate
> places. Perhaps we should just add a memset?
>
> BTW, those static variables are pretty disgusting and are going to cause
> trouble for users which deal with >1 domain at a time.
>   

Heh, I was thinking about this on my way to the office today ...

> It's not clear that there is any reason for it to be a static variable
> anyway. It comes from 20545:cc7d66ba0dad which says: "pass the
> restore_context through function and allocate the context on the restore
> function stack." but, presumably by mistake, retains the static
> modifiers from the global variable. The same is true of the save side.
>
> So how about this instead (untested but seemingly sane...):
>   

Yes, it looks sane to me and has now been tested.  I did several
save/restore of both pv and hvm domains through libvirt libxl driver.

    Tested-by: Jim Fehlig <jfehlig@novell.com>

Regards,
Jim

> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1306224654 -3600
> # Node ID 13599c2c82d8bdbfc73611fed9a1bc2aebfa275c
> # Parent  9cf8d38260606de37826b76334028114feff6131
> libxc: xc_domain_{save,restore}: remove static variables
>
> 20544:ad9d75d74bd5 and 20545:cc7d66ba0dad seemingly intended to change these
> global static variables into stack variables but didn't remove the static
> qualifier.
>
> Also zero the entire struct once with memset rather than clearing fields
> piecemeal in two different places.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> diff -r 9cf8d3826060 -r 13599c2c82d8 tools/libxc/xc_domain_restore.c
> --- a/tools/libxc/xc_domain_restore.c	Tue May 24 09:02:05 2011 +0100
> +++ b/tools/libxc/xc_domain_restore.c	Tue May 24 09:10:54 2011 +0100
> @@ -1133,23 +1133,19 @@ int xc_domain_restore(xc_interface *xch,
>  
>      int orig_io_fd_flags;
>  
> -    static struct restore_ctx _ctx = {
> -        .live_p2m = NULL,
> -        .p2m = NULL,
> -    };
> -    static struct restore_ctx *ctx = &_ctx;
> +    struct restore_ctx _ctx;
> +    struct restore_ctx *ctx = &_ctx;
>      struct domain_info_context *dinfo = &ctx->dinfo;
>  
>      pagebuf_init(&pagebuf);
>      memset(&tailbuf, 0, sizeof(tailbuf));
>      tailbuf.ishvm = hvm;
>  
> -    /* For info only */
> -    ctx->nr_pfns = 0;
> -
>      if ( superpages )
>          return 1;
>  
> +    memset(ctx, 0, sizeof(*ctx));
> +
>      ctxt = xc_hypercall_buffer_alloc(xch, ctxt, sizeof(*ctxt));
>  
>      if ( ctxt == NULL )
> diff -r 9cf8d3826060 -r 13599c2c82d8 tools/libxc/xc_domain_save.c
> --- a/tools/libxc/xc_domain_save.c	Tue May 24 09:02:05 2011 +0100
> +++ b/tools/libxc/xc_domain_save.c	Tue May 24 09:10:54 2011 +0100
> @@ -958,11 +958,8 @@ int xc_domain_save(xc_interface *xch, in
>      unsigned long mfn;
>  
>      struct outbuf ob;
> -    static struct save_ctx _ctx = {
> -        .live_p2m = NULL,
> -        .live_m2p = NULL,
> -    };
> -    static struct save_ctx *ctx = &_ctx;
> +    struct save_ctx _ctx;
> +    struct save_ctx *ctx = &_ctx;
>      struct domain_info_context *dinfo = &ctx->dinfo;
>  
>      int completed = 0;
> @@ -976,6 +973,8 @@ int xc_domain_save(xc_interface *xch, in
>  
>      outbuf_init(xch, &ob, OUTBUF_SIZE);
>  
> +    memset(ctx, 0, sizeof(*ctx));
> +
>      /* If no explicit control parameters given, use defaults */
>      max_iters  = max_iters  ? : DEF_MAX_ITERS;
>      max_factor = max_factor ? : DEF_MAX_FACTOR;
>
>
>   

Re: [PATCH] libxc: reset completed flag in restore_ctx

[Ian Jackson]

Jim Fehlig writes ("[Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx"):
> libxc: reset completed flag in restore_ctx

This has been dealt with by Ian's series removing static variables, I
think.  Thanks.

Ian.

Re: [PATCH] libxc: reset completed flag in restore_ctx

[Jim Fehlig]

Ian Jackson wrote:
> Jim Fehlig writes ("[Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx"):
>   
>> libxc: reset completed flag in restore_ctx
>>     
>
> This has been dealt with by Ian's series removing static variables, I
> think.  Thanks.
>   

Yep, I tested Ian's patch and it is definitely a better approach than
this one.

Thanks,
Jim