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

[Don Slutz <dslutz@verizon.com>]

On 12/02/14 09:59, Don Slutz wrote:
> On 12/02/14 09:26, Stefano Stabellini wrote:
>> On Tue, 2 Dec 2014, Don Slutz wrote:
>>> On 12/02/14 06:53, 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>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - call libxl_domain_info instead of libxl_dominfo_init;
>>>> - call libxl_domain_info before retry_transaction.
>>>>
>>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>>>> index de23fec..569a32a 100644
>>>> --- a/tools/libxl/libxl.c
>>>> +++ b/tools/libxl/libxl.c
>>>> @@ -4694,6 +4694,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);
>>>>    @@ -4767,10 +4770,9 @@ 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);
>>>> +    if (enforce && new_target_memkb > 0) {
>>>> +        memorykb = ptr.max_memkb - current_target_memkb + 
>>>> new_target_memkb;

My testing shows that this should be:

         memorykb = ptr.max_memkb - (current_target_memkb + videoram) +
             new_target_memkb;

As far as I can tell the reason for this is that memory/target (aka
current_target_memkb) was set based on:

     new_target_memkb -= videoram;

    -Don Slutz
>>>> +        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 "
>>> You need to remove LIBXL_MAXMEM_CONSTANT here also.
>> I don't think so: LIBXL_MAXMEM_CONSTANT is supposed to be a safety
>> buffer and we should keep it as is across libxl_set_memory_target calls.
>> Arguably LIBXL_MAXMEM_CONSTANT could be removed entirely with the
>> proposed QEMU changes but that is a separate matter.
>>
>
> I was talking about the line:
>
>                     "rc=%d\n", domid, memorykb + 
> LIBXL_MAXMEM_CONSTANT, rc);
>
> (which is missing from the diff but is part of the LIBXL__LOG_ERRNO 
> call).  The
> error message no longer matches what  xc_domain_setmaxmem() was called
> with.
>
>    -Don Slutz
>
>>>> @@ -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);
>