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

[Ian Campbell <Ian.Campbell@citrix.com>]

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?)

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

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

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> 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.

> ---
>  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?

> +        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.

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