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

[PATCH v2 for-4.6] 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>

---

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;
+        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 v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Don Slutz]

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

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

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

[Stefano Stabellini]

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

 
> > @@ -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 v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Don Slutz]

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;
>>> +        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);

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

[Stefano Stabellini]

On Tue, 2 Dec 2014, 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;
> > > > +        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.

Yep, you are right, I'll fix.




> >   
> > > > @@ -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 v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Don Slutz]

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

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

[Stefano Stabellini]

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.


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

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

[Don Slutz]

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

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

[Stefano Stabellini]

On Wed, 3 Dec 2014, Don Slutz wrote:
> 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.

Thanks, I'll do.