Re: [Xen-devel] [PATCH Remus v1 1/8] tools/libxc: adjust the memory allocation for migration

[Hongyang Yang <yanghy@xxxxxxxxxxxxxx>]

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 <yanghy@xxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel