[PATCH v4] Fix xl vcpu-set to decrease an guest vCPU amoutn without complaints.

[PATCH v4] Fix xl vcpu-set to decrease an guest vCPU amoutn without complaints.

[Konrad Rzeszutek Wilk]

Hey Ian and Ian,

Since v3 [http://lists.xen.org/archives/html/xen-devel/2015-03/msg02822.html]
- Constify the libxl_dominfo and handle libxl_domain_info errors.
- Made libxl_domain_info use logging if domain is gone.
- Drop libxl: Add to libxl__domain_type a new return value (LIBXL_DOMAIN_TYPE_NOTFOUND)

In v2 [http://lists.xen.org/archives/html/xen-devel/2015-03/msg01787.html]:
 - Fixed up #1 "libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info" per your review.
 - Moved the check from vcpuset to libxl_set_vcpuonline to check if the set bits of the
   cpumap is greater than the maximum vCPU that the guest can have (and added printks).
 - Added LIBXL_DOMAIN_TYPE_NOTFOUND so that libxl_set_vcpuonline can report that the
   domain id is for an non-existent guest.
 - Added an #define in libxl.h to expose the new _NOTFOUND error value.

 git://xenbits.xen.org/people/konradwilk/xen.git vcpuset.v4

In short, these two patches were modified since v3 posting:

 [PATCH v4 2/7] libxl: In libxl_set_vcpuonline check for maximum
 [PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu count on

while the rest is the same.

Thank you for taking the time to review these patches!

 tools/libxl/libxl.c         | 73 ++++++++++++++++++++++++++++-----------------
 tools/libxl/libxl.h         |  9 +++++-
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    | 47 ++++++++++++++++++-----------
 4 files changed, 84 insertions(+), 46 deletions(-)

Konrad Rzeszutek Wilk (7):
      libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain
      libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.
      libxl/libxl_domain_info: Log if domain not found.
      libxl/vcpuset: Print error if libxl_set_vcpuonline returns ERROR_DOMAIN_NOTFOUND
      libxl/vcpuset: Return error value if failed.
      libxl/vcpuset: Remove useless limit on max_vcpus.
      libxl/vcpuset - allow to decrease vcpu count on overcommitted guests (v5)

[PATCH v4 1/7] libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain

[Konrad Rzeszutek Wilk]

And use that for all of its callers in the tree.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c         | 19 +++++++++++--------
 tools/libxl/libxl.h         |  9 ++++++++-
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    |  4 ++--
 4 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2a735b3..80c7ff6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -455,7 +455,7 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
     /* update /vm/<uuid>/name */
     rc = libxl_domain_info(ctx, &info, domid);
     if (rc)
-        goto x_fail;
+        goto x_rc;
 
     uuid = GCSPRINTF(LIBXL_UUID_FMT, LIBXL_UUID_BYTES(info.uuid));
     vm_name_path = GCSPRINTF("/vm/%s/name", uuid);
@@ -698,7 +698,7 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
         return ERROR_FAIL;
     }
-    if (ret==0 || xcinfo.domain != domid) return ERROR_INVAL;
+    if (ret==0 || xcinfo.domain != domid) return ERROR_DOMAIN_NOTFOUND;
 
     if (info_r)
         xcinfo2xlinfo(ctx, &xcinfo, info_r);
@@ -1577,7 +1577,7 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     switch(rc) {
     case 0:
         break;
-    case ERROR_INVAL:
+    case ERROR_DOMAIN_NOTFOUND:
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "non-existant domain %d", domid);
     default:
         goto out;
@@ -5448,14 +5448,16 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
     libxl_dominfo info;
     char *dompath;
     xs_transaction_t t;
-    int i, rc = ERROR_FAIL;
+    int i, rc;
 
     libxl_dominfo_init(&info);
 
-    if (libxl_domain_info(CTX, &info, domid) < 0) {
+    rc = libxl_domain_info(CTX, &info, domid);
+    if (rc < 0) {
         LOGE(ERROR, "getting domain info list");
         goto out;
     }
+    rc = ERROR_FAIL;
     if (!(dompath = libxl__xs_get_dompath(gc, domid)))
         goto out;
 
@@ -5479,14 +5481,15 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
                                      libxl_bitmap *cpumap)
 {
     libxl_dominfo info;
-    int i;
+    int i, rc;
 
     libxl_dominfo_init(&info);
 
-    if (libxl_domain_info(CTX, &info, domid) < 0) {
+    rc = libxl_domain_info(CTX, &info, domid);
+    if (rc < 0) {
         LOGE(ERROR, "getting domain info list");
         libxl_dominfo_dispose(&info);
-        return ERROR_FAIL;
+        return rc;
     }
     for (i = 0; i <= info.vcpu_max_id; i++) {
         if (libxl_bitmap_test(cpumap, i)) {
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bc75c5..1cf5699 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -179,6 +179,11 @@
 #define LIBXL_HAVE_BUILDINFO_HVM_MMIO_HOLE_MEMKB 1
 
 /*
+ * libxl_domain_info returns ERROR_DOMAIN_NOTFOUND if the domain
+ * is not present, instead of ERROR_INVAL.
+ */
+#define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
@@ -1104,7 +1109,9 @@ int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
  */
 int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
 
-/* May be called with info_r == NULL to check for domain's existance */
+/* May be called with info_r == NULL to check for domain's existence.
+ * Returns ERROR_DOMAIN_NOTFOUND if domain does not exist (used to return
+ * ERROR_INVAL for this scenario). */
 int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
                       uint32_t domid);
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0866433..117b61d 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -64,6 +64,7 @@ libxl_error = Enumeration("error", [
     (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"),
     (-19, "REMUS_DEVICE_NOT_SUPPORTED"),
     (-20, "VNUMA_CONFIG_INVALID"),
+    (-21, "DOMAIN_NOTFOUND"),
     ], value_namespace = "")
 
 libxl_domain_type = Enumeration("domain_type", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 394b55d..1c07ac6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4736,7 +4736,7 @@ int main_list(int argc, char **argv)
     } else if (optind == argc-1) {
         uint32_t domid = find_domain(argv[optind]);
         rc = libxl_domain_info(ctx, &info_buf, domid);
-        if (rc == ERROR_INVAL) {
+        if (rc == ERROR_DOMAIN_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
             return -rc;
@@ -5507,7 +5507,7 @@ int main_sharing(int argc, char **argv)
     } else if (optind == argc-1) {
         uint32_t domid = find_domain(argv[optind]);
         rc = libxl_domain_info(ctx, &info_buf, domid);
-        if (rc == ERROR_INVAL) {
+        if (rc == ERROR_DOMAIN_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
             return -rc;
-- 
2.1.0

[PATCH v4 2/7] libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

[Konrad Rzeszutek Wilk]

There is no sense in trying to online (or offline) CPUs when the size of
cpumap is greater than the maximum number of VCPUs the guest can go to.

As such fail the operation if the count of CPUs to online is greater
than what the guest started with. For the offline case we do not
check (as the bits are unset in the cpumap) and let it go through.

We coalesce some of the underlaying libxl_set_vcpuonline code
together to take advantage for the QMP and XenStore ways.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: constify the libxl_domain and handle libxl_domain_info error]
---
 tools/libxl/libxl.c | 70 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 80c7ff6..13a98fd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5442,28 +5442,34 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
     return 0;
 }
 
+static int libxl__check_max(libxl__gc *gc, const libxl_dominfo *info,
+                            libxl_bitmap *cpumap)
+{
+    int maxcpus = libxl_bitmap_count_set(cpumap);
+
+    if (maxcpus > info->vcpu_max_id + 1)
+    {
+        LOGE(ERROR, "Requested %d VCPUs, however maxcpus is %d!",
+             maxcpus, info->vcpu_max_id + 1);
+        return ERROR_FAIL;
+    }
+    return 0;
+}
+
 static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
-                                         libxl_bitmap *cpumap)
+                                         libxl_bitmap *cpumap,
+                                         const libxl_dominfo *info)
 {
-    libxl_dominfo info;
     char *dompath;
     xs_transaction_t t;
-    int i, rc;
-
-    libxl_dominfo_init(&info);
+    int i, rc = ERROR_FAIL;
 
-    rc = libxl_domain_info(CTX, &info, domid);
-    if (rc < 0) {
-        LOGE(ERROR, "getting domain info list");
-        goto out;
-    }
-    rc = ERROR_FAIL;
     if (!(dompath = libxl__xs_get_dompath(gc, domid)))
         goto out;
 
 retry_transaction:
     t = xs_transaction_start(CTX->xsh);
-    for (i = 0; i <= info.vcpu_max_id; i++)
+    for (i = 0; i <= info->vcpu_max_id; i++)
         libxl__xs_write(gc, t,
                        libxl__sprintf(gc, "%s/cpu/%u/availability", dompath, i),
                        "%s", libxl_bitmap_test(cpumap, i) ? "online" : "offline");
@@ -5473,25 +5479,16 @@ retry_transaction:
     } else
         rc = 0;
 out:
-    libxl_dominfo_dispose(&info);
     return rc;
 }
 
 static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
-                                     libxl_bitmap *cpumap)
+                                     libxl_bitmap *cpumap,
+                                     const libxl_dominfo *info)
 {
-    libxl_dominfo info;
-    int i, rc;
-
-    libxl_dominfo_init(&info);
+    int i;
 
-    rc = libxl_domain_info(CTX, &info, domid);
-    if (rc < 0) {
-        LOGE(ERROR, "getting domain info list");
-        libxl_dominfo_dispose(&info);
-        return rc;
-    }
-    for (i = 0; i <= info.vcpu_max_id; i++) {
+    for (i = 0; i <= info->vcpu_max_id; i++) {
         if (libxl_bitmap_test(cpumap, i)) {
             /* Return value is ignore because it does not tell anything useful
              * on the completion of the command.
@@ -5501,7 +5498,6 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
             libxl__qmp_cpu_add(gc, domid, i);
         }
     }
-    libxl_dominfo_dispose(&info);
     return 0;
 }
 
@@ -5509,25 +5505,41 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
 {
     GC_INIT(ctx);
     int rc;
+    libxl_dominfo info;
+
+    libxl_dominfo_init(&info);
+
+    rc = libxl_domain_info(CTX, &info, domid);
+    if (rc < 0) {
+        LOGE(ERROR, "getting domain info list");
+        goto out;
+    }
+
+    rc = libxl__check_max(gc, &info, cpumap);
+    if (rc)
+        goto out;
+
     switch (libxl__domain_type(gc, domid)) {
     case LIBXL_DOMAIN_TYPE_HVM:
         switch (libxl__device_model_version_running(gc, domid)) {
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
-            rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
+            rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
             break;
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap);
+            rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap, &info);
             break;
         default:
             rc = ERROR_INVAL;
         }
         break;
     case LIBXL_DOMAIN_TYPE_PV:
-        rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
+        rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
         break;
     default:
         rc = ERROR_INVAL;
     }
+out:
+    libxl_dominfo_dispose(&info);
     GC_FREE;
     return rc;
 }
-- 
2.1.0

Re: [PATCH v4 2/7] libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

[Ian Campbell]

On Mon, 2015-03-30 at 11:26 -0400, Konrad Rzeszutek Wilk wrote:
> There is no sense in trying to online (or offline) CPUs when the size of
> cpumap is greater than the maximum number of VCPUs the guest can go to.
> 
> As such fail the operation if the count of CPUs to online is greater
> than what the guest started with. For the offline case we do not
> check (as the bits are unset in the cpumap) and let it go through.
> 
> We coalesce some of the underlaying libxl_set_vcpuonline code

"underlying"

> together to take advantage for the QMP and XenStore ways.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> [v2: constify the libxl_domain and handle libxl_domain_info error]
> ---
>  tools/libxl/libxl.c | 70 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 80c7ff6..13a98fd 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5442,28 +5442,34 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
>      return 0;
>  }
>  
> +static int libxl__check_max(libxl__gc *gc, const libxl_dominfo *info,

The name is so generic (max what?) and the implementation so simple I'd
suggest to just inline the check at the one call site.

Otherwise looks ok, thanks.

Re: [PATCH v4 2/7] libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

[Konrad Rzeszutek Wilk]

On Tue, Mar 31, 2015 at 04:57:51PM +0100, Ian Campbell wrote:
> On Mon, 2015-03-30 at 11:26 -0400, Konrad Rzeszutek Wilk wrote:
> > There is no sense in trying to online (or offline) CPUs when the size of
> > cpumap is greater than the maximum number of VCPUs the guest can go to.
> > 
> > As such fail the operation if the count of CPUs to online is greater
> > than what the guest started with. For the offline case we do not
> > check (as the bits are unset in the cpumap) and let it go through.
> > 
> > We coalesce some of the underlaying libxl_set_vcpuonline code
> 
> "underlying"

Whoa! Just to make sure I understand why I used the incorrect word.

I used an verb while I should have used an adjective - which
'underlying' is, right?

> 
> > together to take advantage for the QMP and XenStore ways.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > [v2: constify the libxl_domain and handle libxl_domain_info error]
> > ---
> >  tools/libxl/libxl.c | 70 +++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 41 insertions(+), 29 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 80c7ff6..13a98fd 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5442,28 +5442,34 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
> >      return 0;
> >  }
> >  
> > +static int libxl__check_max(libxl__gc *gc, const libxl_dominfo *info,
> 
> The name is so generic (max what?) and the implementation so simple I'd
> suggest to just inline the check at the one call site.
> 
> Otherwise looks ok, thanks.
> 
> 

Re: [PATCH v4 2/7] libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

[Ian Campbell]

On Fri, 2015-04-03 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
> > "underlying"
> 
> Whoa! Just to make sure I understand why I used the incorrect word.
> 
> I used an verb while I should have used an adjective - which
> 'underlying' is, right?

Err, I haven't done English grammar since the mid 90s (and it was out of
fashion in our education system at the time...), I'm just coasting on
being allowed to speak my native tongue around here...

To lay is indeed a verb. (It also has other forms, e.g. you can be a lay
preacher or take in the lay of the land, lets skip all that!).

"lying" in the sense of underlying is some form (present participle?) of
"To lie" as in to lie down. i.e. underlying means to lie beneath
something.

Ian.

[PATCH v4 3/7] libxl/libxl_domain_info: Log if domain not found.

[Konrad Rzeszutek Wilk]

If we cannot find the domain - log an error (and still
continue returning an error).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 13a98fd..22f0f0c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -698,8 +698,10 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
         return ERROR_FAIL;
     }
-    if (ret==0 || xcinfo.domain != domid) return ERROR_DOMAIN_NOTFOUND;
-
+    if (ret==0 || xcinfo.domain != domid) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Domain %d not found!", domid);
+        return ERROR_DOMAIN_NOTFOUND;
+    }
     if (info_r)
         xcinfo2xlinfo(ctx, &xcinfo, info_r);
     return 0;
-- 
2.1.0

Re: [PATCH v4 3/7] libxl/libxl_domain_info: Log if domain not found.

[Ian Campbell]

On Mon, 2015-03-30 at 11:26 -0400, Konrad Rzeszutek Wilk wrote:
> If we cannot find the domain - log an error (and still
> continue returning an error).
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v4 4/7] libxl/vcpuset: Print error if libxl_set_vcpuonline returns ERROR_DOMAIN_NOTFOUND

[Konrad Rzeszutek Wilk]

Instead of just printing an generic error.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1c07ac6..0ccf257 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5223,6 +5223,7 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     char *endptr;
     unsigned int max_vcpus, i;
     libxl_bitmap cpumap;
+    int rc;
 
     libxl_bitmap_init(&cpumap);
     max_vcpus = strtoul(nr_vcpus, &endptr, 10);
@@ -5253,8 +5254,12 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     for (i = 0; i < max_vcpus; i++)
         libxl_bitmap_set(&cpumap, i);
 
-    if (libxl_set_vcpuonline(ctx, domid, &cpumap) < 0)
-        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d\n", domid, max_vcpus);
+    rc = libxl_set_vcpuonline(ctx, domid, &cpumap);
+    if (rc == ERROR_DOMAIN_NOTFOUND)
+        fprintf(stderr, "Domain %u does not exist.\n", domid);
+    else if (rc)
+        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d," \
+                " rc: %d\n", domid, max_vcpus, rc);
 
     libxl_bitmap_dispose(&cpumap);
 }
-- 
2.1.0

[PATCH v4 5/7] libxl/vcpuset: Return error value if failed.

[Konrad Rzeszutek Wilk]

The function does not return any values at all. Convert the
internal libxl errors (ERROR_FAIL, ..., etc) to 1.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0ccf257..0a104ce 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5218,7 +5218,7 @@ int main_vcpupin(int argc, char **argv)
     return rc;
 }
 
-static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
+static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
 {
     char *endptr;
     unsigned int max_vcpus, i;
@@ -5229,7 +5229,7 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     max_vcpus = strtoul(nr_vcpus, &endptr, 10);
     if (nr_vcpus == endptr) {
         fprintf(stderr, "Error: Invalid argument.\n");
-        return;
+        return 1;
     }
 
     /*
@@ -5242,14 +5242,15 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
             fprintf(stderr, "You are overcommmitting! You have %d physical " \
                     " CPUs and want %d vCPUs! Aborting, use --ignore-host to " \
                     " continue\n", host_cpu, max_vcpus);
-            return;
+            return 1;
         }
         /* NB: This also limits how many are set in the bitmap */
         max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
     }
-    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
-        fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
-        return;
+    rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
+    if (rc) {
+        fprintf(stderr, "libxl_cpu_bitmap_alloc failed, rc: %d\n", rc);
+        return 1;
     }
     for (i = 0; i < max_vcpus; i++)
         libxl_bitmap_set(&cpumap, i);
@@ -5262,6 +5263,7 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
                 " rc: %d\n", domid, max_vcpus, rc);
 
     libxl_bitmap_dispose(&cpumap);
+    return rc ? 1 : 0;
 }
 
 int main_vcpuset(int argc, char **argv)
@@ -5280,8 +5282,7 @@ int main_vcpuset(int argc, char **argv)
         break;
     }
 
-    vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
-    return 0;
+    return vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
 }
 
 static void output_xeninfo(void)
-- 
2.1.0

[PATCH v4 6/7] libxl/vcpuset: Remove useless limit on max_vcpus.

[Konrad Rzeszutek Wilk]

The check is superflous. If the 'max_vcpus' (argument
value) is greater than  pCPU and --ignore-host has not
been supplied we would print an warning and return
and not call this code.

If the --ignore-host parameter had been used we would
never end up in this condition and enforce 'max_vcpus'.

The only time it would be invoked is if max_vcpus < host_cpu
in which case it would set max_vcpus to max_vcpus.

In short - it is dead code.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0a104ce..b121d75 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5244,8 +5244,6 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
                     " continue\n", host_cpu, max_vcpus);
             return 1;
         }
-        /* NB: This also limits how many are set in the bitmap */
-        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
-- 
2.1.0

[PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v5)

[Konrad Rzeszutek Wilk]

We have a check to warn the user if they are overcommitting.
But the check only checks the hosts CPU amount and does
not take into account the case when the user is trying to fix
the overcommit. That is - they want to limit the amount of
online VCPUs.

This fix allows the user to offline vCPUs without any
warnings when they are running an overcommitted guest.

Also fix the extra space in the message.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
[v2: Remove crud code as spotted by Boris]
[v3: Moved crud code removal to another patch]
[v4: Remove the fprintf as it is done in libxl_domain_info]
[v5: Fix memory leak]
---
 tools/libxl/xl_cmdimpl.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b121d75..648ca08 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5238,12 +5238,21 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
      */
     if (check_host) {
         unsigned int host_cpu = libxl_get_max_cpus(ctx);
-        if (max_vcpus > host_cpu) {
-            fprintf(stderr, "You are overcommmitting! You have %d physical " \
-                    " CPUs and want %d vCPUs! Aborting, use --ignore-host to " \
-                    " continue\n", host_cpu, max_vcpus);
+        libxl_dominfo dominfo;
+
+        rc = libxl_domain_info(ctx, &dominfo, domid);
+        if (rc)
             return 1;
+
+        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
+            fprintf(stderr, "You are overcommmitting! You have %d physical" \
+                    " CPUs and want %d vCPUs! Aborting, use --ignore-host to" \
+                    " continue\n", host_cpu, max_vcpus);
+            rc = 1;
         }
+        libxl_dominfo_dispose(&dominfo);
+        if (rc)
+            return 1;
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
-- 
2.1.0

Re: [PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v5)

[Ian Campbell]

On Mon, 2015-03-30 at 11:26 -0400, Konrad Rzeszutek Wilk wrote:
> We have a check to warn the user if they are overcommitting.
> But the check only checks the hosts CPU amount and does
> not take into account the case when the user is trying to fix
> the overcommit. That is - they want to limit the amount of
> online VCPUs.
> 
> This fix allows the user to offline vCPUs without any
> warnings when they are running an overcommitted guest.
> 
> Also fix the extra space in the message.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v5)

[Xu, Quan]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Konrad Rzeszutek Wilk
> Sent: Monday, March 30, 2015 11:27 PM
> To: ian.campbell@citrix.com; ian.jackson@eu.citrix.com; xen-devel@lists.xen.org;
> wei.liu2@citrix.com
> Cc: Konrad Rzeszutek Wilk
> Subject: [Xen-devel] [PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu count
> on overcommitted guests (v5)
> 
> We have a check to warn the user if they are overcommitting.
> But the check only checks the hosts CPU amount and does not take into account
> the case when the user is trying to fix the overcommit. That is - they want to limit
> the amount of online VCPUs.
> 
> This fix allows the user to offline vCPUs without any warnings when they are
> running an overcommitted guest.
> 
> Also fix the extra space in the message.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> [v2: Remove crud code as spotted by Boris]
> [v3: Moved crud code removal to another patch]
> [v4: Remove the fprintf as it is done in libxl_domain_info]
> [v5: Fix memory leak]
> ---
>  tools/libxl/xl_cmdimpl.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index
> b121d75..648ca08 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5238,12 +5238,21 @@ static int vcpuset(uint32_t domid, const char*
> nr_vcpus, int check_host)
>       */
>      if (check_host) {
>          unsigned int host_cpu = libxl_get_max_cpus(ctx);
> -        if (max_vcpus > host_cpu) {
> -            fprintf(stderr, "You are overcommmitting! You have %d physical " \
> -                    " CPUs and want %d vCPUs! Aborting, use --ignore-host
> to " \
> -                    " continue\n", host_cpu, max_vcpus);
> +        libxl_dominfo dominfo;
> +
> +        rc = libxl_domain_info(ctx, &dominfo, domid);
> +        if (rc)
>              return 1;
> +
> +        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {

Konrad,
	  I have a question. Does xen support vcpu-set to reset vcpu-count larger than current number of active VCPUs ??

       If the vcpu-count is larger than the current number of active VCPUs, sometimes Xen hypervisor doesn't run correctly with Call Trace form series output.
       For example, When I run vcpu-set to set Domain-0 vcpu count from 32 to 20( domain0_max_vcpu is 32), then I try to run vcpu-set to
       Vcpu-set Domain-0 from 20 to 32 again(20 is current number of active VCPUs ). sometimes, it is wrong form series output as below:

Output >> 
[  830.220998] Call Trace:
[  830.221020]  [<ffffffff8110fbca>] ? rcu_irq_exit+0x7a/0xe0
[  830.221032]  [<ffffffff8105f010>] ? xen_safe_halt+0x10/0x20
[  830.221043]  [<ffffffff81074abc>] ? default_idle+0x2c/0xd0
[  830.221051]  [<ffffffff81073eea>] ? arch_cpu_idle+0xa/0x10
[  830.221064]  [<ffffffff810fd2a5>] ? cpu_startup_entry+0x315/0x470
[  830.221074]  [<ffffffff81065ed5>] ? cpu_bringup_and_idle+0x25/0x40
[  830.221082] rcu_sched kthread starved for 63005 jiffies!
Output<<

BTW, I have tried to set domain-0 as http://wiki.xen.org/wiki/Paravirt_Linux_CPU_Hotplug


Thanks
Quan Xu



> +            fprintf(stderr, "You are overcommmitting! You have %d physical" \
> +                    " CPUs and want %d vCPUs! Aborting, use --ignore-host
> to" \
> +                    " continue\n", host_cpu, max_vcpus);
> +            rc = 1;
>          }
> +        libxl_dominfo_dispose(&dominfo);
> +        if (rc)
> +            return 1;
>      }
>      rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
>      if (rc) {
> --
> 2.1.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v5)

[Konrad Rzeszutek Wilk]

On July 1, 2015 7:42:33 AM EDT, "Xu, Quan" <quan.xu@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org
>> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Konrad
>Rzeszutek Wilk
>> Sent: Monday, March 30, 2015 11:27 PM
>> To: ian.campbell@citrix.com; ian.jackson@eu.citrix.com;
>xen-devel@lists.xen.org;
>> wei.liu2@citrix.com
>> Cc: Konrad Rzeszutek Wilk
>> Subject: [Xen-devel] [PATCH v4 7/7] libxl/vcpu-set - allow to
>decrease vcpu count
>> on overcommitted guests (v5)
>> 
>> We have a check to warn the user if they are overcommitting.
>> But the check only checks the hosts CPU amount and does not take into
>account
>> the case when the user is trying to fix the overcommit. That is -
>they want to limit
>> the amount of online VCPUs.
>> 
>> This fix allows the user to offline vCPUs without any warnings when
>they are
>> running an overcommitted guest.
>> 
>> Also fix the extra space in the message.
>> 
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> 
>> ---
>> [v2: Remove crud code as spotted by Boris]
>> [v3: Moved crud code removal to another patch]
>> [v4: Remove the fprintf as it is done in libxl_domain_info]
>> [v5: Fix memory leak]
>> ---
>>  tools/libxl/xl_cmdimpl.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>index
>> b121d75..648ca08 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -5238,12 +5238,21 @@ static int vcpuset(uint32_t domid, const
>char*
>> nr_vcpus, int check_host)
>>       */
>>      if (check_host) {
>>          unsigned int host_cpu = libxl_get_max_cpus(ctx);
>> -        if (max_vcpus > host_cpu) {
>> -            fprintf(stderr, "You are overcommmitting! You have %d
>physical " \
>> -                    " CPUs and want %d vCPUs! Aborting, use
>--ignore-host
>> to " \
>> -                    " continue\n", host_cpu, max_vcpus);
>> +        libxl_dominfo dominfo;
>> +
>> +        rc = libxl_domain_info(ctx, &dominfo, domid);
>> +        if (rc)
>>              return 1;
>> +
>> +        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu)
>{
>
>Konrad,
>	  I have a question. Does xen support vcpu-set to reset vcpu-count
>larger than current number of active VCPUs ??

Yes.
>
>If the vcpu-count is larger than the current number of active VCPUs,
>sometimes Xen hypervisor doesn't run correctly with Call Trace form
>series output.
>For example, When I run vcpu-set to set Domain-0 vcpu count from 32 to
>20( domain0_max_vcpu is 32), then I try to run vcpu-set to
>Vcpu-set Domain-0 from 20 to 32 again(20 is current number of active
>VCPUs ). sometimes, it is wrong form series output as below:
>


Did you use the latest kernel?please start another thread with this error and details such as kernel version, etc. Please CC David Vrabel and Boris Ostrovsky.


>Output >> 
>[  830.220998] Call Trace:
>[  830.221020]  [<ffffffff8110fbca>] ? rcu_irq_exit+0x7a/0xe0
>[  830.221032]  [<ffffffff8105f010>] ? xen_safe_halt+0x10/0x20
>[  830.221043]  [<ffffffff81074abc>] ? default_idle+0x2c/0xd0
>[  830.221051]  [<ffffffff81073eea>] ? arch_cpu_idle+0xa/0x10
>[  830.221064]  [<ffffffff810fd2a5>] ? cpu_startup_entry+0x315/0x470
>[  830.221074]  [<ffffffff81065ed5>] ? cpu_bringup_and_idle+0x25/0x40
>[  830.221082] rcu_sched kthread starved for 63005 jiffies!
>Output<<
>
>BTW, I have tried to set domain-0 as
>http://wiki.xen.org/wiki/Paravirt_Linux_CPU_Hotplug
>
>
>Thanks
>Quan Xu
>
>
>
>> +            fprintf(stderr, "You are overcommmitting! You have %d
>physical" \
>> +                    " CPUs and want %d vCPUs! Aborting, use
>--ignore-host
>> to" \
>> +                    " continue\n", host_cpu, max_vcpus);
>> +            rc = 1;
>>          }
>> +        libxl_dominfo_dispose(&dominfo);
>> +        if (rc)
>> +            return 1;
>>      }
>>      rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
>>      if (rc) {
>> --
>> 2.1.0
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v5)

[Xu, Quan]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Wednesday, July 01, 2015 8:03 PM
> To: Xu, Quan; ian.campbell@citrix.com; ian.jackson@eu.citrix.com;
> xen-devel@lists.xen.org; wei.liu2@citrix.com
> Subject: RE: [Xen-devel] [PATCH v4 7/7] libxl/vcpu-set - allow to decrease vcpu
> count on overcommitted guests (v5)
> 
> 
> 
> On July 1, 2015 7:42:33 AM EDT, "Xu, Quan" <quan.xu@intel.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: xen-devel-bounces@lists.xen.org
> >> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Konrad
> >Rzeszutek Wilk
> >> Sent: Monday, March 30, 2015 11:27 PM
> >> To: ian.campbell@citrix.com; ian.jackson@eu.citrix.com;
> >xen-devel@lists.xen.org;
> >> wei.liu2@citrix.com
> >> Cc: Konrad Rzeszutek Wilk
> >> Subject: [Xen-devel] [PATCH v4 7/7] libxl/vcpu-set - allow to
> >decrease vcpu count
> >> on overcommitted guests (v5)
> >>
> >> We have a check to warn the user if they are overcommitting.
> >> But the check only checks the hosts CPU amount and does not take into
> >account
> >> the case when the user is trying to fix the overcommit. That is -
> >they want to limit
> >> the amount of online VCPUs.
> >>
> >> This fix allows the user to offline vCPUs without any warnings when
> >they are
> >> running an overcommitted guest.
> >>
> >> Also fix the extra space in the message.
> >>
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>
> >> ---
> >> [v2: Remove crud code as spotted by Boris]
> >> [v3: Moved crud code removal to another patch]
> >> [v4: Remove the fprintf as it is done in libxl_domain_info]
> >> [v5: Fix memory leak]
> >> ---
> >>  tools/libxl/xl_cmdimpl.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >index
> >> b121d75..648ca08 100644
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -5238,12 +5238,21 @@ static int vcpuset(uint32_t domid, const
> >char*
> >> nr_vcpus, int check_host)
> >>       */
> >>      if (check_host) {
> >>          unsigned int host_cpu = libxl_get_max_cpus(ctx);
> >> -        if (max_vcpus > host_cpu) {
> >> -            fprintf(stderr, "You are overcommmitting! You have %d
> >physical " \
> >> -                    " CPUs and want %d vCPUs! Aborting, use
> >--ignore-host
> >> to " \
> >> -                    " continue\n", host_cpu, max_vcpus);
> >> +        libxl_dominfo dominfo;
> >> +
> >> +        rc = libxl_domain_info(ctx, &dominfo, domid);
> >> +        if (rc)
> >>              return 1;
> >> +
> >> +        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu)
> >{
> >
> >Konrad,
> >	  I have a question. Does xen support vcpu-set to reset vcpu-count
> >larger than current number of active VCPUs ??
> 
> Yes.
> >

Thanks! This maybe still a bug.



> >If the vcpu-count is larger than the current number of active VCPUs,
> >sometimes Xen hypervisor doesn't run correctly with Call Trace form
> >series output.
> >For example, When I run vcpu-set to set Domain-0 vcpu count from 32 to
> >20( domain0_max_vcpu is 32), then I try to run vcpu-set to Vcpu-set
> >Domain-0 from 20 to 32 again(20 is current number of active VCPUs ).
> >sometimes, it is wrong form series output as below:
> >
> 
> 
> Did you use the latest kernel?please start another thread with this error and
> details such as kernel version, etc. Please CC David Vrabel and Boris Ostrovsky.
> 
> 


Not the last.
I use ubutu 14.04 kernel as domain0 kernel.
Which kernel version does it work? I can try it.

Another question for http://wiki.xen.org/wiki/Paravirt_Linux_CPU_Hotplug : 

   Content of `cpu-online.rules`:
   SUBSYSTEM=="cpu",ACTION=="add",RUN+="/bin/sh -c '[ ! -e /sys$devpath/online ] || echo 1 > /sys$devpath/online'"



Should I replace "/sys$devpath/online " with "/sys/devices/system/cpu/online" ??


Thanks
Quan





> >Output >>
> >[  830.220998] Call Trace:
> >[  830.221020]  [<ffffffff8110fbca>] ? rcu_irq_exit+0x7a/0xe0 [
> >830.221032]  [<ffffffff8105f010>] ? xen_safe_halt+0x10/0x20 [
> >830.221043]  [<ffffffff81074abc>] ? default_idle+0x2c/0xd0 [
> >830.221051]  [<ffffffff81073eea>] ? arch_cpu_idle+0xa/0x10 [
> >830.221064]  [<ffffffff810fd2a5>] ? cpu_startup_entry+0x315/0x470 [
> >830.221074]  [<ffffffff81065ed5>] ? cpu_bringup_and_idle+0x25/0x40 [
> >830.221082] rcu_sched kthread starved for 63005 jiffies!
> >Output<<
> >
> >BTW, I have tried to set domain-0 as
> >http://wiki.xen.org/wiki/Paravirt_Linux_CPU_Hotplug
> >
> >
> >Thanks
> >Quan Xu
> >
> >
> >
> >> +            fprintf(stderr, "You are overcommmitting! You have %d
> >physical" \
> >> +                    " CPUs and want %d vCPUs! Aborting, use
> >--ignore-host
> >> to" \
> >> +                    " continue\n", host_cpu, max_vcpus);
> >> +            rc = 1;
> >>          }
> >> +        libxl_dominfo_dispose(&dominfo);
> >> +        if (rc)
> >> +            return 1;
> >>      }
> >>      rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
> >>      if (rc) {
> >> --
> >> 2.1.0
> >>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> http://lists.xen.org/xen-devel
> 
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.