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

[PATCH v6] 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. In the future the offset will
include memory allocated by QEMU for rom files.

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

---

Changes in v6:
- remove now useless xc_domaininfo_t info and xc_domain_getinfolist call.

Changes in v5:
- call libxl_dominfo_init;
- move libxl_dominfo_dispose call before returning to the caller;

Changes in v4:
- remove new_target_memkb <= 0 check.

Changes in v3:
- move call to libxl__uuid2string and libxl_dominfo_dispose earlier;
- error out if new_target_memkb <= 0.

Changes in v2:
- remove LIBXL_MAXMEM_CONSTANT from LIBXL__LOG_ERRNO.
---
 tools/libxl/libxl.c |   23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b9a1941..b9d083a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4715,11 +4715,15 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
     uint32_t current_max_memkb = 0;
     char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
     char *dompath = libxl__xs_get_dompath(gc, domid);
-    xc_domaininfo_t info;
     libxl_dominfo ptr;
     char *uuid;
     xs_transaction_t t;
 
+    if (libxl_domain_info(ctx, &ptr, domid) < 0)
+        goto out_no_transaction;
+
+    uuid = libxl__uuid2string(gc, ptr.uuid);
+
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
 
@@ -4795,13 +4799,12 @@ retry_transaction:
     }
 
     if (enforce) {
-        memorykb = new_target_memkb + videoram;
-        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
-                LIBXL_MAXMEM_CONSTANT);
+        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;
         }
@@ -4820,18 +4823,9 @@ retry_transaction:
 
     libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
                 dompath), "%"PRIu32, new_target_memkb);
-    rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
-    if (rc != 1 || info.domain != domid) {
-        abort_transaction = 1;
-        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);
-    libxl_dominfo_dispose(&ptr);
 
 out:
     if (!xs_transaction_end(ctx->xsh, t, abort_transaction)
@@ -4840,6 +4834,7 @@ out:
             goto retry_transaction;
 
 out_no_transaction:
+    libxl_dominfo_dispose(&ptr);
     GC_FREE;
     return rc;
 }
-- 
1.7.10.4

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

[Wei Liu]

On Fri, Feb 27, 2015 at 12:45:47PM +0000, Stefano Stabellini wrote:
> In libxl_set_memory_target when setting the new maxmem, retain the same
> offset on top of the current target. In the future the offset will
> include memory allocated by QEMU for rom files.
> 

Sorry for not having mentioned this earlier.

I think the behaviour of this API is changed now (for good reason).

Ian and Ian, do want need a #define in libxl.h to mark such change?

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v6:
> - remove now useless xc_domaininfo_t info and xc_domain_getinfolist call.
> 
> Changes in v5:
> - call libxl_dominfo_init;
> - move libxl_dominfo_dispose call before returning to the caller;
> 
> Changes in v4:
> - remove new_target_memkb <= 0 check.
> 
> Changes in v3:
> - move call to libxl__uuid2string and libxl_dominfo_dispose earlier;
> - error out if new_target_memkb <= 0.
> 
> Changes in v2:
> - remove LIBXL_MAXMEM_CONSTANT from LIBXL__LOG_ERRNO.
> ---
>  tools/libxl/libxl.c |   23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b9a1941..b9d083a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4715,11 +4715,15 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
>      uint32_t current_max_memkb = 0;
>      char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
>      char *dompath = libxl__xs_get_dompath(gc, domid);
> -    xc_domaininfo_t info;
>      libxl_dominfo ptr;
>      char *uuid;
>      xs_transaction_t t;

You missed libxl_domain_info_init.

>  
> +    if (libxl_domain_info(ctx, &ptr, domid) < 0)
> +        goto out_no_transaction;
> +
> +    uuid = libxl__uuid2string(gc, ptr.uuid);
> +

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

[Ian Campbell]

On Mon, 2015-03-02 at 11:36 +0000, Wei Liu wrote:
> On Fri, Feb 27, 2015 at 12:45:47PM +0000, Stefano Stabellini wrote:
> > In libxl_set_memory_target when setting the new maxmem, retain the same
> > offset on top of the current target. In the future the offset will
> > include memory allocated by QEMU for rom files.
> > 
> 
> Sorry for not having mentioned this earlier.
> 
> I think the behaviour of this API is changed now (for good reason).
> 
> Ian and Ian, do want need a #define in libxl.h to mark such change?

I'm not sure, but I don't think that changes to the internal
calculations regarding overheads count as a libxl ABI change.

By analogy we don't break ABI/API if Xen happens to need an extra page
of memory to deal with a guest.

Other than Wei's comment about initialising the dominfo this looks good
to me.

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

[Wei Liu]

On Mon, Mar 02, 2015 at 02:05:18PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-02 at 11:36 +0000, Wei Liu wrote:
> > On Fri, Feb 27, 2015 at 12:45:47PM +0000, Stefano Stabellini wrote:
> > > In libxl_set_memory_target when setting the new maxmem, retain the same
> > > offset on top of the current target. In the future the offset will
> > > include memory allocated by QEMU for rom files.
> > > 
> > 
> > Sorry for not having mentioned this earlier.
> > 
> > I think the behaviour of this API is changed now (for good reason).
> > 
> > Ian and Ian, do want need a #define in libxl.h to mark such change?
> 
> I'm not sure, but I don't think that changes to the internal
> calculations regarding overheads count as a libxl ABI change.
> 
> By analogy we don't break ABI/API if Xen happens to need an extra page
> of memory to deal with a guest.
> 

Makes sense.

> Other than Wei's comment about initialising the dominfo this looks good
> to me.
> 
>