[PATCH] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[PATCH] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Stefano Stabellini]

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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index de23fec..8381c3e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4767,10 +4767,12 @@ retry_transaction:
                 "%s/memory/videoram", dompath));
     videoram = videoram_s ? atoi(videoram_s) : 0;
 
-    if (enforce) {
-        memorykb = new_target_memkb;
-        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
-                LIBXL_MAXMEM_CONSTANT);
+    libxl_dominfo_init(&ptr);
+    xcinfo2xlinfo(ctx, &info, &ptr);
+
+    if (enforce && new_target_memkb > 0) {
+        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 "
@@ -4800,8 +4802,6 @@ retry_transaction:
         goto out;
     }
 
-    libxl_dominfo_init(&ptr);
-    xcinfo2xlinfo(ctx, &info, &ptr);
     uuid = libxl__uuid2string(gc, ptr.uuid);
     libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
             "%"PRIu32, new_target_memkb / 1024);

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

[Don Slutz]

On 11/25/14 12:54, 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.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index de23fec..8381c3e 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4767,10 +4767,12 @@ retry_transaction:
>                   "%s/memory/videoram", dompath));
>       videoram = videoram_s ? atoi(videoram_s) : 0;
>   
> -    if (enforce) {
> -        memorykb = new_target_memkb;
> -        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
> -                LIBXL_MAXMEM_CONSTANT);
> +    libxl_dominfo_init(&ptr);
> +    xcinfo2xlinfo(ctx, &info, &ptr);

This fills ptr with uninitialized data.  You need to call

xc_domain_getinfolist()

before this.  However calling xc_domain_getinfolist() here and retry
of the xenstore transaction, you will adjust this more then one time.

So I think that xc_domain_getinfolist() needs to be called before
the label retry_transaction.  However rc is a mess in this routine.

rc = 1 is the normal return (since rc = xc_domain_getinfolist() is
the last setting of rc).  So all the rest of "rc =" needs to be adjusted
someway.

    -Don Slutz


> +
> +    if (enforce && new_target_memkb > 0) {
> +        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 "
> @@ -4800,8 +4802,6 @@ retry_transaction:
>           goto out;
>       }
>   
> -    libxl_dominfo_init(&ptr);
> -    xcinfo2xlinfo(ctx, &info, &ptr);
>       uuid = libxl__uuid2string(gc, ptr.uuid);
>       libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
>               "%"PRIu32, new_target_memkb / 1024);
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Stefano Stabellini]

On Mon, 1 Dec 2014, Don Slutz wrote:
> On 11/25/14 12:54, 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index de23fec..8381c3e 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4767,10 +4767,12 @@ retry_transaction:
> >                   "%s/memory/videoram", dompath));
> >       videoram = videoram_s ? atoi(videoram_s) : 0;
> >   -    if (enforce) {
> > -        memorykb = new_target_memkb;
> > -        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
> > -                LIBXL_MAXMEM_CONSTANT);
> > +    libxl_dominfo_init(&ptr);
> > +    xcinfo2xlinfo(ctx, &info, &ptr);
> 
> This fills ptr with uninitialized data.  You need to call
> 
> xc_domain_getinfolist()
> 
> before this.  However calling xc_domain_getinfolist() here and retry
> of the xenstore transaction, you will adjust this more then one time.
> 
> So I think that xc_domain_getinfolist() needs to be called before
> the label retry_transaction.  However rc is a mess in this routine.
> 
> rc = 1 is the normal return (since rc = xc_domain_getinfolist() is
> the last setting of rc).  So all the rest of "rc =" needs to be adjusted
> someway.

Instead of calling xc_domain_getinfolist, I think it's best to call
libxl_domain_info. You are right that I need to call it before
retry_transaction.

Thanks for the review!



>    -Don Slutz
> 
> 
> > +
> > +    if (enforce && new_target_memkb > 0) {
> > +        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 "
> > @@ -4800,8 +4802,6 @@ retry_transaction:
> >           goto out;
> >       }
> >   -    libxl_dominfo_init(&ptr);
> > -    xcinfo2xlinfo(ctx, &info, &ptr);
> >       uuid = libxl__uuid2string(gc, ptr.uuid);
> >       libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
> >               "%"PRIu32, new_target_memkb / 1024);
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>