Re: [Xen-devel] [PATCH v2] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 28 Jan 2015, Ian Campbell wrote:
> On Mon, 2015-01-26 at 17:03 +0000, Stefano Stabellini wrote:
> > In libxl_set_memory_target when setting the new maxmem, retain the same
> > offset on top of the current target. The offset includes memory
> > allocated by QEMU for rom files.
> 
> Did we apply that patch for 4.5? (should this be backported?)

No, we didn't. We decided to wait for the new dev cycle.


> How is this change expected to interact with relative vs. absolute mode?

This change works well with both.


> Does docs/misc/libxl_memory.txt not need an update to account for this
> change in behaviour?

No, because this patch doesn't change the memory layout: it only makes
sure that it stays the same when libxl_set_memory_target is called
after the guest has booted.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > 
> > ---
> > 
> > Changes in v2:
> > - remove LIBXL_MAXMEM_CONSTANT from LIBXL__LOG_ERRNO.
> 
> And from the setmaxmem call too from the looks of it, can the reason for
> that be explained in the commit log please.

I am not really removing LIBXL_MAXMEM_CONSTANT for maxmem: by setting
the new maxmem as a relative change to the current one,
LIBXL_MAXMEM_CONSTANT has already been included.


> > ---
> >  tools/libxl/libxl.c |   14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index cd6f42c..04062dd 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4717,6 +4717,9 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t 
> > domid,
> >      char *uuid;
> >      xs_transaction_t t;
> >  
> > +    if (libxl_domain_info(ctx, &ptr, domid) < 0)
> > +        goto out_no_transaction;
> > +
> >  retry_transaction:
> >      t = xs_transaction_start(ctx->xsh);
> >  
> > @@ -4791,14 +4794,13 @@ retry_transaction:
> >          goto out;
> >      }
> >  
> > -    if (enforce) {
> > -        memorykb = new_target_memkb + videoram;
> > -        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
> > -                LIBXL_MAXMEM_CONSTANT);
> > +    if (enforce && new_target_memkb > 0) {
> 
> How does this change in the condition relate to the change here?

This is just one more correctness fix. I should note into the commit
message.


> > +        memorykb = ptr.max_memkb - current_target_memkb + new_target_memkb;
> > +        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
> >          if (rc != 0) {
> >              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> >                      "xc_domain_setmaxmem domid=%d memkb=%d failed "
> > -                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, 
> > rc);
> > +                    "rc=%d\n", domid, memorykb, rc);
> >              abort_transaction = 1;
> >              goto out;
> >          }
> > @@ -4823,8 +4825,6 @@ retry_transaction:
> >          goto out;
> >      }
> >  
> > -    libxl_dominfo_init(&ptr);
> > -    xcinfo2xlinfo(ctx, &info, &ptr);
> 
> >      uuid = libxl__uuid2string(gc, ptr.uuid);
> 
> I think you should move this and the dispose just outside the context up
> to the libxl_domain_info call, to keep them more obviously together.

OK


> >      libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
> >              "%"PRIu32, new_target_memkb / 1024);
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel