On 05/07/2015 05:48 PM, Andrew Cooper wrote:
On 07/05/15 07:37, Yang Hongyang wrote:
Move the memory allocation before the concrete live/nolive save
in order to avoid the free/alloc memory loop when using Remus.
Signed-off-by: Yang Hongyang <yan...@cn.fujitsu.com>
---
tools/libxc/xc_sr_save.c | 53 +++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 32 deletions(-)
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 5d9c267..7fed668 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -3,6 +3,8 @@
#include "xc_sr_common.h"
+DECLARE_HYPERCALL_BUFFER(unsigned long, to_send);
This unfortunately causes an issue when concurrent calls to
xc_domain_save() in the same process. While this is a highly
ill-advised action, I did try to avoid breaking it.
Please move this declaration into the ctx.save union.
I know the best way is to put this into ctx.save union, but I haven't
found a method to put it in, the DECLARE_HYPERCALL_BUFFER macro can not
be used there, should I just define a unsigned long var at ctx.save
union, and use other macro(what macro?) define at save()?
+
/*
* Writes an Image header and Domain header into the stream.
*/
@@ -475,25 +477,11 @@ static int update_progress_string(struct xc_sr_context
*ctx,
static int send_domain_memory_live(struct xc_sr_context *ctx)
{
xc_interface *xch = ctx->xch;
- DECLARE_HYPERCALL_BUFFER(unsigned long, to_send);
xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size };
char *progress_str = NULL;
unsigned x;
int rc = -1;
- to_send = xc_hypercall_buffer_alloc_pages(
- xch, to_send, NRPAGES(bitmap_size(ctx->save.p2m_size)));
-
- ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE *
- sizeof(*ctx->save.batch_pfns));
- ctx->save.deferred_pages = calloc(1, bitmap_size(ctx->save.p2m_size));
-
- if ( !ctx->save.batch_pfns || !to_send || !ctx->save.deferred_pages )
- {
- ERROR("Unable to allocate memory for to_{send,fix}/batch bitmaps");
- goto out;
- }
-
rc = enable_logdirty(ctx);
if ( rc )
goto out;
@@ -593,10 +581,6 @@ static int send_domain_memory_live(struct xc_sr_context
*ctx)
out:
xc_set_progress_prefix(xch, NULL);
free(progress_str);
- xc_hypercall_buffer_free_pages(xch, to_send,
- NRPAGES(bitmap_size(ctx->save.p2m_size)));
- free(ctx->save.deferred_pages);
- free(ctx->save.batch_pfns);
return rc;
}
@@ -609,17 +593,6 @@ static int send_domain_memory_nonlive(struct xc_sr_context
*ctx)
xc_interface *xch = ctx->xch;
int rc = -1;
- ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE *
- sizeof(*ctx->save.batch_pfns));
- ctx->save.deferred_pages = calloc(1, bitmap_size(ctx->save.p2m_size));
-
- if ( !ctx->save.batch_pfns || !ctx->save.deferred_pages )
- {
- PERROR("Failed to allocate memory for nonlive tracking structures");
- errno = ENOMEM;
- goto err;
- }
-
rc = suspend_domain(ctx);
if ( rc )
goto err;
@@ -631,9 +604,6 @@ static int send_domain_memory_nonlive(struct xc_sr_context
*ctx)
goto err;
err:
- free(ctx->save.deferred_pages);
- free(ctx->save.batch_pfns);
-
return rc;
}
@@ -645,6 +615,20 @@ static int save(struct xc_sr_context *ctx, uint16_t
guest_type)
xc_interface *xch = ctx->xch;
int rc, saved_rc = 0, saved_errno = 0;
+ to_send = xc_hypercall_buffer_alloc_pages(
+ xch, to_send,
NRPAGES(bitmap_size(ctx->save.p2m_size)));
This allocation only needs to be made if ctx->save.live is set. For
plain suspend and resume, logdirty handling is not required.
+ ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE *
+ sizeof(*ctx->save.batch_pfns));
+ ctx->save.deferred_pages = calloc(1, bitmap_size(ctx->save.p2m_size));
+
+ if ( !ctx->save.batch_pfns || !to_send || !ctx->save.deferred_pages )
+ {
+ ERROR("Unable to allocate memory for to_{send,fix}/batch bitmaps");
+ rc = -1;
+ errno = ENOMEM;
+ goto err;
+ }
+
Instead of complicating save() like this, could you introduce two new
static functions setup() and cleanup() which subsume the
ctx->save.ops.{setup,cleanup}() calls and also do these allocations and
free.
Sure, will do this in next version.
I think the SHADOW_OP_OFF hypercall can also be moved into the cleanup()
function.
~Andrew
IPRINTF("Saving domain %d, type %s",
ctx->domid, dhdr_type_to_str(guest_type));
@@ -704,6 +688,11 @@ static int save(struct xc_sr_context *ctx, uint16_t
guest_type)
if ( rc )
PERROR("Failed to clean up");
+ xc_hypercall_buffer_free_pages(xch, to_send,
+ NRPAGES(bitmap_size(ctx->save.p2m_size)));
+ free(ctx->save.deferred_pages);
+ free(ctx->save.batch_pfns);
+
if ( saved_rc )
{
rc = saved_rc;
.
--
Thanks,
Yang.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
