[PATCH 0/2] Revert changes to libxl_set_memory_target, unblock 4.6

[PATCH 0/2] Revert changes to libxl_set_memory_target, unblock 4.6

[Wei Liu]

A new memory model that allows QEMU to bump memory behind libxl's back was
merged a few months ago. We didn't fully understand the repercussions back
then. Now it breaks migration and becomes blocker of 4.6 release.

It's better to restore to original behaviour at this stage of the release
cycle, that would put us in a position no worse than before, so the release is
unblocked.

The said function is still racy after reverting these two patches. Making
domain memory state consistent requires a bit more work. Separate patch(es)
will be sent out to deal with that problem.

Wei.

Wei Liu (2):
  Revert "libxl: fix "xl mem-set" regression from 0c029c4da2"
  Revert "libxl_set_memory_target: retain the same maxmem offset on top
    of the current target"

 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_domain.c       |  2 +-
 tools/libxl/libxl.c           | 29 ++++++++++++++++-------------
 3 files changed, 18 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH 1/2] Revert "libxl: fix "xl mem-set" regression from 0c029c4da2"

[Wei Liu]

This reverts commit f5b43e95facdc17f925cb56a8963cd4531074034.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/include/xenctrl.h | 2 +-
 tools/libxc/xc_domain.c       | 2 +-
 tools/libxl/libxl.c           | 5 ++---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index d1d2ab3..db5d028 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1285,7 +1285,7 @@ int xc_getcpuinfo(xc_interface *xch, int max_cpus,
 
 int xc_domain_setmaxmem(xc_interface *xch,
                         uint32_t domid,
-                        uint64_t max_memkb);
+                        unsigned int max_memkb);
 
 int xc_domain_set_memmap_limit(xc_interface *xch,
                                uint32_t domid,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index ce51e69..5b1a52d 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -635,7 +635,7 @@ int xc_shadow_control(xc_interface *xch,
 
 int xc_domain_setmaxmem(xc_interface *xch,
                         uint32_t domid,
-                        uint64_t max_memkb)
+                        unsigned int max_memkb)
 {
     DECLARE_DOMCTL;
     domctl.cmd = XEN_DOMCTL_max_mem;
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d86ea62..35caf42 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4736,8 +4736,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
 {
     GC_INIT(ctx);
     int rc = 1, abort_transaction = 0;
-    uint64_t memorykb;
-    uint32_t videoram = 0;
+    uint32_t memorykb = 0, videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
     uint32_t current_max_memkb = 0;
     char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
@@ -4831,7 +4830,7 @@ retry_transaction:
         rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
         if (rc != 0) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                    "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed "
+                    "xc_domain_setmaxmem domid=%d memkb=%d failed "
                     "rc=%d\n", domid, memorykb, rc);
             abort_transaction = 1;
             goto out;
-- 
1.9.1

[PATCH 2/2] Revert "libxl_set_memory_target: retain the same maxmem offset on top of the current target"

[Wei Liu]

This reverts commit 0c029c4da2169159064568ef4fea862a5d2cd84a.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 35caf42..e3e63b5 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4741,16 +4741,11 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
     uint32_t current_max_memkb = 0;
     char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
     char *dompath = libxl__xs_get_dompath(gc, domid);
+    xc_domaininfo_t info;
     libxl_dominfo ptr;
     char *uuid;
     xs_transaction_t t;
 
-    libxl_dominfo_init(&ptr);
-    if (libxl_domain_info(ctx, &ptr, domid) < 0)
-        goto out_no_transaction;
-
-    uuid = libxl__uuid2string(gc, ptr.uuid);
-
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
 
@@ -4826,12 +4821,13 @@ retry_transaction:
     }
 
     if (enforce) {
-        memorykb = ptr.max_memkb - current_target_memkb + new_target_memkb;
-        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
+        memorykb = new_target_memkb + videoram;
+        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
+                LIBXL_MAXMEM_CONSTANT);
         if (rc != 0) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                     "xc_domain_setmaxmem domid=%d memkb=%d failed "
-                    "rc=%d\n", domid, memorykb, rc);
+                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc);
             abort_transaction = 1;
             goto out;
         }
@@ -4850,9 +4846,18 @@ retry_transaction:
 
     libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
                 dompath), "%"PRIu32, new_target_memkb);
+    rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
+    if (rc != 1 || info.domain != domid) {
+        abort_transaction = 1;
+        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);
+    libxl_dominfo_dispose(&ptr);
 
 out:
     if (!xs_transaction_end(ctx->xsh, t, abort_transaction)
@@ -4861,7 +4866,6 @@ out:
             goto retry_transaction;
 
 out_no_transaction:
-    libxl_dominfo_dispose(&ptr);
     GC_FREE;
     return rc;
 }
-- 
1.9.1

Re: [PATCH 0/2] Revert changes to libxl_set_memory_target, unblock 4.6

[Wei Liu]

CC Jan, author of one of the patches.

On Tue, Jun 23, 2015 at 03:16:22PM +0100, Wei Liu wrote:
> A new memory model that allows QEMU to bump memory behind libxl's back was
> merged a few months ago. We didn't fully understand the repercussions back
> then. Now it breaks migration and becomes blocker of 4.6 release.
> 
> It's better to restore to original behaviour at this stage of the release
> cycle, that would put us in a position no worse than before, so the release is
> unblocked.
> 
> The said function is still racy after reverting these two patches. Making
> domain memory state consistent requires a bit more work. Separate patch(es)
> will be sent out to deal with that problem.
> 
> Wei.
> 
> Wei Liu (2):
>   Revert "libxl: fix "xl mem-set" regression from 0c029c4da2"
>   Revert "libxl_set_memory_target: retain the same maxmem offset on top
>     of the current target"
> 
>  tools/libxc/include/xenctrl.h |  2 +-
>  tools/libxc/xc_domain.c       |  2 +-
>  tools/libxl/libxl.c           | 29 ++++++++++++++++-------------
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> -- 
> 1.9.1

Re: [PATCH 1/2] Revert "libxl: fix "xl mem-set" regression from 0c029c4da2"

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 03:16:23PM +0100, Wei Liu wrote:
> This reverts commit f5b43e95facdc17f925cb56a8963cd4531074034.

Could you include some detail of why the revert is needed?

Thanks.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/include/xenctrl.h | 2 +-
>  tools/libxc/xc_domain.c       | 2 +-
>  tools/libxl/libxl.c           | 5 ++---
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index d1d2ab3..db5d028 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1285,7 +1285,7 @@ int xc_getcpuinfo(xc_interface *xch, int max_cpus,
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        uint64_t max_memkb);
> +                        unsigned int max_memkb);
>  
>  int xc_domain_set_memmap_limit(xc_interface *xch,
>                                 uint32_t domid,
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index ce51e69..5b1a52d 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -635,7 +635,7 @@ int xc_shadow_control(xc_interface *xch,
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        uint64_t max_memkb)
> +                        unsigned int max_memkb)
>  {
>      DECLARE_DOMCTL;
>      domctl.cmd = XEN_DOMCTL_max_mem;
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d86ea62..35caf42 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4736,8 +4736,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
>  {
>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
> -    uint64_t memorykb;
> -    uint32_t videoram = 0;
> +    uint32_t memorykb = 0, videoram = 0;
>      uint32_t current_target_memkb = 0, new_target_memkb = 0;
>      uint32_t current_max_memkb = 0;
>      char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
> @@ -4831,7 +4830,7 @@ retry_transaction:
>          rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
>          if (rc != 0) {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> -                    "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed "
> +                    "xc_domain_setmaxmem domid=%d memkb=%d failed "
>                      "rc=%d\n", domid, memorykb, rc);
>              abort_transaction = 1;
>              goto out;
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/2] Revert "libxl: fix "xl mem-set" regression from 0c029c4da2"

[Jan Beulich]

>>> On 23.06.15 at 16:16, <wei.liu2@citrix.com> wrote:
> This reverts commit f5b43e95facdc17f925cb56a8963cd4531074034.

Even if the patch having introduced the regression this fixed is
being reverted, it's not clear to me why this change needs to be
reverted too - it seems correct to me to use 64-bit types for the
calculations. Or are you planning to do a more comprehensive
adjustments to all of the types later on?

Jan

> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1285,7 +1285,7 @@ int xc_getcpuinfo(xc_interface *xch, int max_cpus,
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        uint64_t max_memkb);
> +                        unsigned int max_memkb);
>  
>  int xc_domain_set_memmap_limit(xc_interface *xch,
>                                 uint32_t domid,
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index ce51e69..5b1a52d 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -635,7 +635,7 @@ int xc_shadow_control(xc_interface *xch,
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        uint64_t max_memkb)
> +                        unsigned int max_memkb)
>  {
>      DECLARE_DOMCTL;
>      domctl.cmd = XEN_DOMCTL_max_mem;
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d86ea62..35caf42 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4736,8 +4736,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t 
> domid,
>  {
>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
> -    uint64_t memorykb;
> -    uint32_t videoram = 0;
> +    uint32_t memorykb = 0, videoram = 0;
>      uint32_t current_target_memkb = 0, new_target_memkb = 0;
>      uint32_t current_max_memkb = 0;
>      char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
> @@ -4831,7 +4830,7 @@ retry_transaction:
>          rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
>          if (rc != 0) {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> -                    "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed "
> +                    "xc_domain_setmaxmem domid=%d memkb=%d failed "
>                      "rc=%d\n", domid, memorykb, rc);
>              abort_transaction = 1;
>              goto out;
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH 1/2] Revert "libxl: fix "xl mem-set" regression from 0c029c4da2"

[Wei Liu]

On Tue, Jun 23, 2015 at 03:45:14PM +0100, Jan Beulich wrote:
> >>> On 23.06.15 at 16:16, <wei.liu2@citrix.com> wrote:
> > This reverts commit f5b43e95facdc17f925cb56a8963cd4531074034.
> 
> Even if the patch having introduced the regression this fixed is
> being reverted, it's not clear to me why this change needs to be
> reverted too - it seems correct to me to use 64-bit types for the
> calculations. Or are you planning to do a more comprehensive
> adjustments to all of the types later on?
> 

I was thinking more about giving Ian (who is looking at making things
consistent) a clean state to start with. He may end up touching those
types.

This patch itself looks correct to me.

Ian, I think I will send a patch to revert the only offending commit and
fix up conflicts. Are you OK with that.

Wei.

Re: [PATCH 1/2] Revert "libxl: fix "xl mem-set" regression from 0c029c4da2"

[Ian Campbell]

On Tue, 2015-06-23 at 15:52 +0100, Wei Liu wrote:
> On Tue, Jun 23, 2015 at 03:45:14PM +0100, Jan Beulich wrote:
> > >>> On 23.06.15 at 16:16, <wei.liu2@citrix.com> wrote:
> > > This reverts commit f5b43e95facdc17f925cb56a8963cd4531074034.
> > 
> > Even if the patch having introduced the regression this fixed is
> > being reverted, it's not clear to me why this change needs to be
> > reverted too - it seems correct to me to use 64-bit types for the
> > calculations. Or are you planning to do a more comprehensive
> > adjustments to all of the types later on?
> > 
> 
> I was thinking more about giving Ian (who is looking at making things
> consistent) a clean state to start with. He may end up touching those
> types.
> 
> This patch itself looks correct to me.
> 
> Ian, I think I will send a patch to revert the only offending commit and
> fix up conflicts. Are you OK with that.

Yes, it's the right thing to do, I didn't correctly remember what Jan's
fixup was doing when I thought we should revert it.

I've just sent out a locking patch fix, which may not now apply due to
this change. I'll rebase if that turns out to be needed, I think it'll
just be some lite-contextual differences.

Ian.