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

[PATCH v2] 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:
- remove LIBXL_MAXMEM_CONSTANT from LIBXL__LOG_ERRNO.
---
 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) {
+        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);
     libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
             "%"PRIu32, new_target_memkb / 1024);
-- 
1.7.10.4

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

[Ian Campbell]

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

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

[Stefano Stabellini]

On Wed, 28 Jan 2015, Ian Campbell wrote:
> 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?)

No, we didn't. We decided to wait for the new dev cycle.


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

This change works well with both.


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

No, because this patch doesn't change the memory layout: it only makes
sure that it stays the same when libxl_set_memory_target is called
after the guest has booted.


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

I am not really removing LIBXL_MAXMEM_CONSTANT for maxmem: by setting
the new maxmem as a relative change to the current one,
LIBXL_MAXMEM_CONSTANT has already been included.


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

This is just one more correctness fix. I should note into the commit
message.


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

OK


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

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

[Ian Campbell]

On Wed, 2015-01-28 at 14:35 +0000, Stefano Stabellini wrote:
> On Wed, 28 Jan 2015, Ian Campbell wrote:
> > 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?)
> 
> No, we didn't. We decided to wait for the new dev cycle.

OK, so "The offset includes..." should really be "In the future the
offset will include..."?

> > How is this change expected to interact with relative vs. absolute mode?
> 
> This change works well with both.

What I meant was what are the semantics of relative mode, it seems like
that should require now change to the function?

> > Does docs/misc/libxl_memory.txt not need an update to account for this
> > change in behaviour?
> 
> No, because this patch doesn't change the memory layout: it only makes
> sure that it stays the same when libxl_set_memory_target is called
> after the guest has booted.

OK, perhaps the ROM file accounting patch needs that change then?

> > > 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.
> 
> I am not really removing LIBXL_MAXMEM_CONSTANT for maxmem: by setting
> the new maxmem as a relative change to the current one,
> LIBXL_MAXMEM_CONSTANT has already been included.

Ah right. please allude to that in the commit log.
> > > +    if (enforce && new_target_memkb > 0) {
> > 
> > How does this change in the condition relate to the change here?
> 
> This is just one more correctness fix. I should note into the commit
> message.

Please.

Although, if someone asks to set RAM to 0 and enforce -- should we not
do so? Or error, or something other than silently nothing.

Ian.

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

[Stefano Stabellini]

On Wed, 28 Jan 2015, Ian Campbell wrote:
> On Wed, 2015-01-28 at 14:35 +0000, Stefano Stabellini wrote:
> > On Wed, 28 Jan 2015, Ian Campbell wrote:
> > > 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?)
> > 
> > No, we didn't. We decided to wait for the new dev cycle.
> 
> OK, so "The offset includes..." should really be "In the future the
> offset will include..."?

Yes, I'll change it.


> > > How is this change expected to interact with relative vs. absolute mode?
> > 
> > This change works well with both.
> 
> What I meant was what are the semantics of relative mode, it seems like
> that should require now change to the function?

Relative mode changes the memory target relative to the current target.
This patch leaves this behavior unchanged.

If enforce=1 is passed to the function, libxl_set_memory_target is
supposed to enforce the new limit setting maxmem accordingly. This
behavior is also unchanged, but the maxmem calculation is more
accurate.


> > > Does docs/misc/libxl_memory.txt not need an update to account for this
> > > change in behaviour?
> > 
> > No, because this patch doesn't change the memory layout: it only makes
> > sure that it stays the same when libxl_set_memory_target is called
> > after the guest has booted.
> 
> OK, perhaps the ROM file accounting patch needs that change then?

Yes, please don't apply the ROM file accounting patch for the moment.


> > > > 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.
> > 
> > I am not really removing LIBXL_MAXMEM_CONSTANT for maxmem: by setting
> > the new maxmem as a relative change to the current one,
> > LIBXL_MAXMEM_CONSTANT has already been included.
> 
> Ah right. please allude to that in the commit log.

OK


> > > > +    if (enforce && new_target_memkb > 0) {
> > > 
> > > How does this change in the condition relate to the change here?
> > 
> > This is just one more correctness fix. I should note into the commit
> > message.
> 
> Please.
> 
> Although, if someone asks to set RAM to 0 and enforce -- should we not
> do so? Or error, or something other than silently nothing.

I think you are right. I'll change it to log an error and return.