Jim Fehlig
2011-May-23 23:05 UTC
[Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx
# 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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-May-24 08:13 UTC
Re: [Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx
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; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jim Fehlig
2011-May-24 15:12 UTC
Re: [Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx
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; > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-May-24 17:33 UTC
Re: [Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jim Fehlig
2011-May-24 17:47 UTC
Re: [Xen-devel] [PATCH] libxc: reset completed flag in restore_ctx
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel