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/03/14 12:31, Stefano Stabellini wrote:
> On Tue, 2 Dec 2014, Don Slutz wrote:
>> 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;
> Thank you very much for testing and the suggestion!
>
> I think that the right fix for this is to remove videoram from
> new_target_memkb earlier and only when the new target is absolute,
> otherwise we risk removing videoram twice (in case the new target is
> relative). I wonder why we didn't notice this before.

Sounds like a good idea.  No clue, I have been looking real close at 
this stuff.
and that my be why I tripped over it.

>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d5d5204..4803cc4 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4744,13 +4744,17 @@ retry_transaction:
>           goto out;
>       }
>   
> +    videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc,
> +                "%s/memory/videoram", dompath));
> +    videoram = videoram_s ? atoi(videoram_s) : 0;
> +
>       if (relative) {
>           if (target_memkb < 0 && abs(target_memkb) > current_target_memkb)
>               new_target_memkb = 0;
>           else
>               new_target_memkb = current_target_memkb + target_memkb;
>       } else
> -        new_target_memkb = target_memkb;
> +        new_target_memkb = target_memkb - videoram;
>       if (new_target_memkb > memorykb) {
>           LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>                   "memory_dynamic_max must be less than or equal to"
> @@ -4766,9 +4770,6 @@ retry_transaction:
>           abort_transaction = 1;
>           goto out;
>       }
> -    videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc,
> -                "%s/memory/videoram", dompath));
> -    videoram = videoram_s ? atoi(videoram_s) : 0;
>   
>       if (enforce && new_target_memkb > 0) {
>           memorykb = ptr.max_memkb - current_target_memkb + new_target_memkb;
> @@ -4782,7 +4783,6 @@ retry_transaction:
>           }
>       }
>   
> -    new_target_memkb -= videoram;
>       rc = xc_domain_set_pod_target(ctx->xch, domid,
>               new_target_memkb / 4, NULL, NULL, NULL);
>       if (rc != 0) {

This does look like a bugfix for just the videoram issue.  Not sure why
you made this v2 since I do not see the original change.

Anyway if you want to post this change for 4.5 (?) I would be happy to 
review it.
     -Don Slutz