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] libxc: reset completed flag in restore_ctx

To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx
From: Jim Fehlig <jfehlig@xxxxxxxxxx>
Date: Tue, 24 May 2011 09:12:47 -0600
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Vincent Hanquez <Vincent.Hanquez@xxxxxxxxxxxxx>
Delivery-date: Tue, 24 May 2011 08:13:53 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1306224788.20576.109.camel@xxxxxxxxxxxxxxxxxxxxxx>
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>
References: <f94242f20cdaee81d28f.1306191954@xxxxxxxxxxxxxxxxxxxxxxxxx> <1306224788.20576.109.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird 2.0.0.18 (X11/20081112)
Ian Campbell wrote:
> On Tue, 2011-05-24 at 00:05 +0100, Jim Fehlig wrote:
>   
>> # HG changeset patch
>> # User Jim Fehlig <jfehlig@xxxxxxxxxx>
>> # 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@xxxxxxxxxx>

Regards,
Jim

> # HG changeset patch
> # User Ian Campbell <ian.campbell@xxxxxxxxxx>
> # 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@xxxxxxxxxx>
>
> 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;
>
>
>   

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