[PATCH v2 0/7] Some (not only) cpupool related fixes and improvements

[PATCH v2 0/7] Some (not only) cpupool related fixes and improvements

[Dario Faggioli]

Hi,

Here's the take two of this:

 http://www.gossamer-threads.com/lists/xen/devel/370516

I think I addressed all the comments from v1's review.

Sample output for the new `xl list' can be found here:

 http://pastebin.com/qSagPimL

Available as a git branch here:

 git://xenbits.xen.org/people/dariof/xen.git rel/cpupools/allow-ranges-v1
 http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/rel/cpupools/allow-ranges-v2

Thanks and Regards,
Dario

---
Dario Faggioli (7):
      xl: turn some int local variable into bool
      xl: add -c/--cpupool option to `xl list'
      libxl: introduce libxl_cpupool_cpu{add,remove}_cpumap()
      xl: enable using ranges of pCPUs when manipulating cpupools
      xl: enable using ranges of pCPUs when creating cpupools
      xl: make error reporting of cpupool subcommands consistent
      xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

 docs/man/xl.pod.1            |   29 +++++
 docs/man/xlcpupool.cfg.pod.5 |   22 ++++
 tools/libxl/libxl.c          |   56 +++++++++-
 tools/libxl/libxl.h          |   13 ++
 tools/libxl/xl_cmdimpl.c     |  225 ++++++++++++++++++++++--------------------
 tools/libxl/xl_cmdtable.c    |    1 
 6 files changed, 222 insertions(+), 124 deletions(-)

[PATCH v2 1/7] xl: turn some int local variable into bool

[Dario Faggioli]

more specifically, the ones used as argument presence
flags in `xl list'.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes from v1:
 * use true/false for actual parameters, as requested
   during review
---
 tools/libxl/xl_cmdimpl.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a27ac9e..3122fd5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3505,7 +3505,7 @@ static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
     }
 }
 
-static void list_domains(int verbose, int context, int claim, int numa,
+static void list_domains(bool verbose, bool context, bool claim, bool numa,
                          const libxl_dominfo *info, int nb_domain)
 {
     int i;
@@ -4489,10 +4489,11 @@ int main_reboot(int argc, char **argv)
 
 int main_list(int argc, char **argv)
 {
-    int opt, verbose = 0;
-    int context = 0;
-    int details = 0;
-    int numa = 0;
+    int opt;
+    bool verbose = false;
+    bool context = false;
+    bool details = false;
+    bool numa = false;
     static struct option opts[] = {
         {"long", 0, 0, 'l'},
         {"verbose", 0, 0, 'v'},
@@ -4508,16 +4509,16 @@ int main_list(int argc, char **argv)
 
     SWITCH_FOREACH_OPT(opt, "lvhZn", opts, "list", 0) {
     case 'l':
-        details = 1;
+        details = true;
         break;
     case 'v':
-        verbose = 1;
+        verbose = true;
         break;
     case 'Z':
-        context = 1;
+        context = true;
         break;
     case 'n':
-        numa = 1;
+        numa = true;
         break;
     }
 
@@ -4550,7 +4551,8 @@ int main_list(int argc, char **argv)
     if (details)
         list_domains_details(info, nb_domain);
     else
-        list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
+        list_domains(verbose, context, false /* claim */, numa,
+                     info, nb_domain);
 
     if (info_free)
         libxl_dominfo_list_free(info, nb_domain);
@@ -6600,8 +6602,8 @@ int main_claims(int argc, char **argv)
         return 1;
     }
 
-    list_domains(0 /* verbose */, 0 /* context */, 1 /* claim */,
-                 0 /* numa */, info, nb_domain);
+    list_domains(false /* verbose */, false /* context */, true /* claim */,
+                 false /* numa */, info, nb_domain);
 
     libxl_dominfo_list_free(info, nb_domain);
     return 0;

[PATCH v2 2/7] xl: add -c/--cpupool option to `xl list'

[Dario Faggioli]

which, if provided, makes the command print a column
with the name of the cpupool of the listed domain(s).

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes from v1:
* use true/false for actual parameters, as requested
  during review
---
 docs/man/xl.pod.1         |    4 ++++
 tools/libxl/xl_cmdimpl.c  |   19 +++++++++++++++----
 tools/libxl/xl_cmdtable.c |    1 +
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index b016272..5da9b15 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -304,6 +304,10 @@ Also prints the security labels.
 
 Also prints the domain UUIDs, the shutdown reason and security labels.
 
+=item B<-c>, <--cpupool>
+
+Also prints the cpupool the domain belong to.
+
 =item B<-n>, <--numa>
 
 Also prints the domain NUMA node affinity.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3122fd5..0481261 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3506,7 +3506,7 @@ static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
 }
 
 static void list_domains(bool verbose, bool context, bool claim, bool numa,
-                         const libxl_dominfo *info, int nb_domain)
+                         bool cpupool, const libxl_dominfo *info, int nb_domain)
 {
     int i;
     static const char shutdown_reason_letters[]= "-rscw";
@@ -3520,6 +3520,7 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
     if (verbose) printf("   UUID                            Reason-Code\tSecurity Label");
     if (context && !verbose) printf("   Security Label");
     if (claim) printf("  Claimed");
+    if (cpupool) printf("         Cpupool");
     if (numa) {
         if (libxl_node_bitmap_alloc(ctx, &nodemap, 0)) {
             fprintf(stderr, "libxl_node_bitmap_alloc_failed.\n");
@@ -3564,6 +3565,11 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
             printf(" %5lu", (unsigned long)info[i].outstanding_memkb / 1024);
         if (verbose || context)
             printf(" %16s", info[i].ssid_label ? : "-");
+        if (cpupool) {
+            char *poolname = libxl_cpupoolid_to_name(ctx, info[i].cpupool);
+            printf("%16s", poolname);
+            free(poolname);
+        }
         if (numa) {
             libxl_domain_get_nodeaffinity(ctx, info[i].domid, &nodemap);
 
@@ -4493,11 +4499,13 @@ int main_list(int argc, char **argv)
     bool verbose = false;
     bool context = false;
     bool details = false;
+    bool cpupool = false;
     bool numa = false;
     static struct option opts[] = {
         {"long", 0, 0, 'l'},
         {"verbose", 0, 0, 'v'},
         {"context", 0, 0, 'Z'},
+        {"cpupool", 0, 0, 'c'},
         {"numa", 0, 0, 'n'},
         COMMON_LONG_OPTS,
         {0, 0, 0, 0}
@@ -4507,7 +4515,7 @@ int main_list(int argc, char **argv)
     libxl_dominfo *info, *info_free=0;
     int nb_domain, rc;
 
-    SWITCH_FOREACH_OPT(opt, "lvhZn", opts, "list", 0) {
+    SWITCH_FOREACH_OPT(opt, "lvhZcn", opts, "list", 0) {
     case 'l':
         details = true;
         break;
@@ -4517,6 +4525,9 @@ int main_list(int argc, char **argv)
     case 'Z':
         context = true;
         break;
+    case 'c':
+        cpupool = true;
+        break;
     case 'n':
         numa = true;
         break;
@@ -4551,7 +4562,7 @@ int main_list(int argc, char **argv)
     if (details)
         list_domains_details(info, nb_domain);
     else
-        list_domains(verbose, context, false /* claim */, numa,
+        list_domains(verbose, context, false /* claim */, numa, cpupool,
                      info, nb_domain);
 
     if (info_free)
@@ -6603,7 +6614,7 @@ int main_claims(int argc, char **argv)
     }
 
     list_domains(false /* verbose */, false /* context */, true /* claim */,
-                 false /* numa */, info, nb_domain);
+                 false /* numa */, false /* cpupool */, info, nb_domain);
 
     libxl_dominfo_list_free(info, nb_domain);
     return 0;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 22ab63b..9284887 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -53,6 +53,7 @@ struct cmd_spec cmd_table[] = {
       "-l, --long              Output all VM details\n"
       "-v, --verbose           Prints out UUIDs and security context\n"
       "-Z, --context           Prints out security context\n"
+      "-c, --cpupool           Prints the cpupool the domain is in\n"
       "-n, --numa              Prints out NUMA node affinity"
     },
     { "destroy",

[PATCH v2 3/7] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Dario Faggioli]

To add (removes) to (from) a cpupool all the pCPUs corresponding
to the bits that are set in the passed bitmap.

This is convenient and useful in order to implement, in xl,
the possibility of specifying ranges of pCPUs to be added
(removed) to (from) a cpupool, in the relevant commands.

While there, convert libxl_cpupool_cpu{add,remove} to use the
appropriate logging macro (LOGE()).

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
Changes from v1:
* added LIBXL_HAVE_CPUPOOL_ADD_REM_CPUMAP, as requested
  during review;
* libxl_cpupool_cpu{add,remove} now conform more to the
  'goto out:' error handling style (although there is
  no need for any actual goto! :-D), as requested
  during review;
* libxl_cpupool_cpu{add,remove}_cpumap() now return
  0 or ERROR_FAIL, as requested during review.
---
 tools/libxl/libxl.c |   56 ++++++++++++++++++++++++++++++++++++++++++---------
 tools/libxl/libxl.h |   13 ++++++++++++
 2 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 94b4d59..ad69a8a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6358,15 +6358,33 @@ out:
 
 int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
-    int rc;
+    GC_INIT(ctx);
+    int rc = 0;
 
     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
-        return ERROR_FAIL;
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        rc = ERROR_FAIL;
     }
-    return 0;
+
+    GC_FREE;
+    return rc;
+}
+
+int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0, rc = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
+            ncpus++;
+    }
+
+    if (ncpus != libxl_bitmap_count_set(cpumap))
+        rc = ERROR_FAIL;
+
+    return rc;
 }
 
 int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
@@ -6403,15 +6421,33 @@ out:
 
 int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
-    int rc;
+    GC_INIT(ctx);
+    int rc = 0;
 
     rc = xc_cpupool_removecpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error removing cpu from cpupool");
-        return ERROR_FAIL;
+        LOGE(ERROR, "Error removing cpu %d from cpupool", cpu);
+        rc = ERROR_FAIL;
     }
-    return 0;
+
+    GC_FREE;
+    return rc;
+}
+
+int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                   const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0, rc = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuremove(ctx, poolid, c))
+            ncpus++;
+    }
+
+    if (ncpus != libxl_bitmap_count_set(cpumap))
+        rc = ERROR_FAIL;
+
+    return rc;
 }
 
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5eec092..bf0c39e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -83,6 +83,15 @@
  */
 #define LIBXL_HAVE_CPUPOOL_QUALIFIER_TO_CPUPOOLID 1
 
+/* LIBXL_HAVE_CPUPOOL_ADD_REM_CPUMAP
+ *
+ * If this is defined, libxl has two library functions called
+ * libxl_cpupool_cpuadd_cpumap and libxl_cpupool_cpuremove_cpumap,
+ * which allow to add to or remove from a cpupool all the cpus
+ * specified in a bitmap.
+ */
+#define LIBXL_HAVE_CPUPOOL_ADD_REM_CPUMAP 1
+
 /*
  * LIBXL_HAVE_FIRMWARE_PASSTHROUGH indicates the feature for
  * passing in SMBIOS and ACPI firmware to HVM guests is present
@@ -1468,8 +1477,12 @@ int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
 int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
 int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
 int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
+int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                const libxl_bitmap *cpumap);
 int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
+int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                   const libxl_bitmap *cpumap);
 int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
 int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);

[PATCH v2 4/7] xl: enable using ranges of pCPUs when manipulating cpupools

[Dario Faggioli]

in fact, right now, xl sub-commands 'cpupool-cpu-add' and
'cpupool-cpu-remove' only accept the specification of one
pCPU to be added or removed to/from a cpupool.

With this change, they can deal with ranges, like "4-8",
or "node:1,12-18,^14". The syntax is exactly the same one
that is supported by the 'vcpu-pin' subcommand, and
specifying just one pCPU still works, of course.

This make things more flexible, more consistent, and also
improves error handling, as the pCPU range parsing routine
already present in xl is more reliable than just a call
to atoi().

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
Changes from v1:
* reword the man page change, in line with what was
  requested during review.
---
 docs/man/xl.pod.1        |   25 +++++++++++--
 tools/libxl/xl_cmdimpl.c |   92 ++++++++++++++++++++--------------------------
 2 files changed, 60 insertions(+), 57 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 5da9b15..16783c8 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1147,13 +1147,30 @@ This is possible only if no domain is active in the cpu-pool.
 
 Renames a cpu-pool to I<newname>.
 
-=item B<cpupool-cpu-add> I<cpu-pool> I<cpu-nr|node:node-nr>
+=item B<cpupool-cpu-add> I<cpu-pool> I<cpus|node:nodes>
 
-Adds a cpu or all cpus of a numa node to a cpu-pool.
+Adds one or more CPUs or NUMA nodes to I<cpu-pool>. CPUs and NUMA
+nodes can be specified as single CPU/node IDs or as ranges.
 
-=item B<cpupool-cpu-remove> I<cpu-nr|node:node-nr>
+For example:
+
+ (a) xl cpupool-cpu-add mypool 4
+ (b) xl cpupool-cpu-add mypool 1,5,10-16,^13
+ (c) xl cpupool-cpu-add mypool node:0,nodes:2-3,^10-12,8
+
+means adding CPU 4 to mypool, in (a); adding CPUs 1,5,10,11,12,14,15
+and 16, in (b); and adding all the CPUs of NUMA nodes 0, 2 and 3,
+plus CPU 8, but keeping out CPUs 10,11,12, in (c).
+
+All the specified CPUs that can be added to the cpupool will be added
+to it. If some CPU can't (e.g., because they're already part of another
+cpupool), an error is reported about each one of them.
+
+=item B<cpupool-cpu-remove> I<cpus|node:nodes>
 
-Removes a cpu or all cpus of a numa node from a cpu-pool.
+Removes one or more CPUs or NUMA nodes from I<cpu-pool>. CPUs and NUMA
+nodes can be specified as single CPU/node IDs or as ranges, using the
+exact same syntax as in B<cpupool-cpu-add> above.
 
 =item B<cpupool-migrate> I<domain> I<cpu-pool>
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0481261..ba5b51e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -760,7 +760,7 @@ static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
  * single cpus or as eintire NUMA nodes) and turns it into the
  * corresponding libxl_bitmap (in cpumap).
  */
-static int vcpupin_parse(const char *cpu, libxl_bitmap *cpumap)
+static int cpurange_parse(const char *cpu, libxl_bitmap *cpumap)
 {
     char *ptr, *saveptr = NULL, *buf = strdup(cpu);
     int rc = 0;
@@ -872,7 +872,7 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
                 exit(1);
             }
 
-            if (vcpupin_parse(buf, &vcpu_affinity_array[j]))
+            if (cpurange_parse(buf, &vcpu_affinity_array[j]))
                 exit(1);
 
             j++;
@@ -889,7 +889,7 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
             exit(1);
         }
 
-        if (vcpupin_parse(buf, &vcpu_affinity_array[0]))
+        if (cpurange_parse(buf, &vcpu_affinity_array[0]))
             exit(1);
 
         for (i = 1; i < b_info->max_vcpus; i++) {
@@ -4964,7 +4964,7 @@ int main_vcpupin(int argc, char **argv)
      */
     if (!strcmp(hard_str, "-"))
         hard = NULL;
-    else if (vcpupin_parse(hard_str, hard))
+    else if (cpurange_parse(hard_str, hard))
         goto out;
     /*
      * Soft affinity is handled similarly. Only difference: we also want
@@ -4972,7 +4972,7 @@ int main_vcpupin(int argc, char **argv)
      */
     if (argc <= optind+3 || !strcmp(soft_str, "-"))
         soft = NULL;
-    else if (vcpupin_parse(soft_str, soft))
+    else if (cpurange_parse(soft_str, soft))
         goto out;
 
     if (dryrun_only) {
@@ -7310,44 +7310,37 @@ int main_cpupoolcpuadd(int argc, char **argv)
     int opt;
     const char *pool;
     uint32_t poolid;
-    int cpu;
-    int node;
-    int n;
+    libxl_bitmap cpumap;
+    int rc = 1;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-cpu-add", 2) {
         /* No options */
     }
 
-    pool = argv[optind++];
-    node = -1;
-    cpu = -1;
-    if (strncmp(argv[optind], "node:", 5) == 0) {
-        node = atoi(argv[optind] + 5);
-    } else {
-        cpu = atoi(argv[optind]);
+    libxl_bitmap_init(&cpumap);
+    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
+        fprintf(stderr, "Unable to allocate cpumap");
+        return 1;
     }
 
+    pool = argv[optind++];
+    if (cpurange_parse(argv[optind], &cpumap))
+        goto out;
+
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
-    }
-
-    if (cpu >= 0) {
-        return -libxl_cpupool_cpuadd(ctx, poolid, cpu);
+        goto out;
     }
 
-    if (libxl_cpupool_cpuadd_node(ctx, poolid, node, &n)) {
-        fprintf(stderr, "libxl_cpupool_cpuadd_node failed\n");
-        return -ERROR_FAIL;
-    }
+    if (libxl_cpupool_cpuadd_cpumap(ctx, poolid, &cpumap))
+        fprintf(stderr, "some cpus may not have been added to %s\n", pool);
 
-    if (n > 0) {
-        return 0;
-    }
+    rc = 0;
 
-    fprintf(stderr, "no free cpu found\n");
-    return -ERROR_FAIL;
+out:
+    libxl_bitmap_dispose(&cpumap);
+    return rc;
 }
 
 int main_cpupoolcpuremove(int argc, char **argv)
@@ -7355,44 +7348,37 @@ int main_cpupoolcpuremove(int argc, char **argv)
     int opt;
     const char *pool;
     uint32_t poolid;
-    int cpu;
-    int node;
-    int n;
+    libxl_bitmap cpumap;
+    int rc = 1;
+
+    libxl_bitmap_init(&cpumap);
+    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
+        fprintf(stderr, "Unable to allocate cpumap");
+        return 1;
+    }
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-cpu-remove", 2) {
         /* No options */
     }
 
     pool = argv[optind++];
-    node = -1;
-    cpu = -1;
-    if (strncmp(argv[optind], "node:", 5) == 0) {
-        node = atoi(argv[optind] + 5);
-    } else {
-        cpu = atoi(argv[optind]);
-    }
+    if (cpurange_parse(argv[optind], &cpumap))
+        goto out;
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
-    }
-
-    if (cpu >= 0) {
-        return -libxl_cpupool_cpuremove(ctx, poolid, cpu);
+        goto out;
     }
 
-    if (libxl_cpupool_cpuremove_node(ctx, poolid, node, &n)) {
-        fprintf(stderr, "libxl_cpupool_cpuremove_node failed\n");
-        return -ERROR_FAIL;
-    }
+    if (libxl_cpupool_cpuremove_cpumap(ctx, poolid, &cpumap))
+        fprintf(stderr, "some cpus may not have been removed from %s\n", pool);
 
-    if (n == 0) {
-        fprintf(stderr, "no cpu of node found in cpupool\n");
-        return -ERROR_FAIL;
-    }
+    rc = 0;
 
-    return 0;
+out:
+    libxl_bitmap_dispose(&cpumap);
+    return rc;
 }
 
 int main_cpupoolmigrate(int argc, char **argv)

[PATCH v2 5/7] xl: enable using ranges of pCPUs when creating cpupools

[Dario Faggioli]

instead of just list of single pCPUs or NUMA node IDs, as
it happens right now.

On the other hand, after this change, strings containing
pCPUs and NUMA node ranges is supported. The syntax is the
same one supported by the "cpus" and "cpus_soft" config
switch, i.e., "4-8" or "node:1,12-18,^14".

This make things more flexible, more consistent, and also
improves error handling, as the pCPU range parsing routine
already present in xl is more reliable than just a call
to atoi().

While there, remove a redundant error check in the legacy syntax
handling (libxl_bitmap_test() already checks the index being
within the size of the bitmap).

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
 docs/man/xlcpupool.cfg.pod.5 |   22 +++++++++++++++++++---
 tools/libxl/xl_cmdimpl.c     |   17 ++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
index bb15cbe..2ff8ee8 100644
--- a/docs/man/xlcpupool.cfg.pod.5
+++ b/docs/man/xlcpupool.cfg.pod.5
@@ -93,10 +93,26 @@ Specifies the cpus of the NUMA-nodes given in C<NODES> (an integer or
 a list of integers) to be member of the cpupool. The free cpus in the
 specified nodes are allocated in the new cpupool.
 
-=item B<cpus="CPUS">
+=item B<cpus="CPU-LIST">
 
-The specified C<CPUS> are allocated in the new cpupool. All cpus must
-be free. Must not be specified together with B<nodes>.
+Specifies the cpus that will be member of the cpupool. All the specified
+cpus must be free, or creation will fail. C<CPU-LIST> may be specified
+as follows:
+
+=over 4
+
+=item ["2", "3", "5"]
+
+means that cpus 2,3,5 will be member of the cpupool.
+
+=item "0-3,5,^1"
+
+means that cpus 0,2,3 and 5 will be member of the cpupool. A "node:" or
+"nodes:" modifier can be used. E.g., "0,node:1,nodes:2-3,^10-13" means
+that pcpus 0, plus all the cpus of NUMA nodes 1,2,3 with the exception
+of cpus 10,11,12,13 will be memeber of the cpupool.
+
+=back
 
 If neither B<nodes> nor B<cpus> are specified only the first free cpu
 found will be allocated in the new cpupool.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ba5b51e..b2d80f4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7148,18 +7148,29 @@ int main_cpupoolcreate(int argc, char **argv)
             fprintf(stderr, "no free cpu found\n");
             goto out_cfg;
         }
-    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 0)) {
+    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 1)) {
         n_cpus = 0;
         while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
             i = atoi(buf);
-            if ((i < 0) || (i >= freemap.size * 8) ||
-                !libxl_bitmap_test(&freemap, i)) {
+            if ((i < 0) || !libxl_bitmap_test(&freemap, i)) {
                 fprintf(stderr, "cpu %d illegal or not free\n", i);
                 goto out_cfg;
             }
             libxl_bitmap_set(&cpumap, i);
             n_cpus++;
         }
+    } else if (!xlu_cfg_get_string(config, "cpus", &buf, 0)) {
+        if (cpurange_parse(buf, &cpumap))
+            goto out_cfg;
+
+        n_cpus = 0;
+        libxl_for_each_set_bit(i, cpumap) {
+            if (!libxl_bitmap_test(&freemap, i)) {
+                fprintf(stderr, "cpu %d illegal or not free\n", i);
+                goto out_cfg;
+            }
+            n_cpus++;
+        }
     } else
         n_cpus = 0;

[PATCH v2 6/7] xl: make error reporting of cpupool subcommands consistent

[Dario Faggioli]

with the rest of the file, where we return 1 on 0, rather
than using libxl error codes.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   69 ++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b2d80f4..8587cc2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7019,7 +7019,7 @@ int main_cpupoolcreate(int argc, char **argv)
     libxl_bitmap cpumap;
     libxl_uuid uuid;
     libxl_cputopology *topology;
-    int rc = -ERROR_FAIL;
+    int rc = 1;
 
     SWITCH_FOREACH_OPT(opt, "hnf:", opts, "cpupool-create", 0) {
     case 'f':
@@ -7213,7 +7213,6 @@ int main_cpupoollist(int argc, char **argv)
     int n_pools, p, c, n;
     uint32_t poolid;
     char *name;
-    int ret = 0;
 
     SWITCH_FOREACH_OPT(opt, "hc", opts, "cpupool-list", 0) {
     case 'c':
@@ -7225,14 +7224,14 @@ int main_cpupoollist(int argc, char **argv)
         pool = argv[optind];
         if (libxl_name_to_cpupoolid(ctx, pool, &poolid)) {
             fprintf(stderr, "Pool \'%s\' does not exist\n", pool);
-            return -ERROR_FAIL;
+            return 1;
         }
     }
 
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {
         fprintf(stderr, "error getting cpupool info\n");
-        return -ERROR_NOMEM;
+        return 1;
     }
 
     printf("%-19s", "Name");
@@ -7242,7 +7241,7 @@ int main_cpupoollist(int argc, char **argv)
         printf("CPUs   Sched     Active   Domain count\n");
 
     for (p = 0; p < n_pools; p++) {
-        if (!ret && (!pool || (poolinfo[p].poolid == poolid))) {
+        if (!pool || (poolinfo[p].poolid == poolid)) {
             name = poolinfo[p].pool_name;
             printf("%-19s", name);
             n = 0;
@@ -7263,7 +7262,7 @@ int main_cpupoollist(int argc, char **argv)
 
     libxl_cpupoolinfo_list_free(poolinfo, n_pools);
 
-    return ret;
+    return 0;
 }
 
 int main_cpupooldestroy(int argc, char **argv)
@@ -7280,11 +7279,14 @@ int main_cpupooldestroy(int argc, char **argv)
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
-        fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown cpupool '%s'\n", pool);
+        return 1;
     }
 
-    return -libxl_cpupool_destroy(ctx, poolid);
+    if (libxl_cpupool_destroy(ctx, poolid))
+        return 1;
+
+    return 0;
 }
 
 int main_cpupoolrename(int argc, char **argv)
@@ -7302,14 +7304,14 @@ int main_cpupoolrename(int argc, char **argv)
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
-        fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown cpupool '%s'\n", pool);
+        return 1;
     }
 
     new_name = argv[optind];
 
     if (libxl_cpupool_rename(ctx, new_name, poolid)) {
-        fprintf(stderr, "Can't rename cpupool '%s'.\n", pool);
+        fprintf(stderr, "Can't rename cpupool '%s'\n", pool);
         return 1;
     }
 
@@ -7409,22 +7411,25 @@ int main_cpupoolmigrate(int argc, char **argv)
 
     if (libxl_domain_qualifier_to_domid(ctx, dom, &domid) ||
         !libxl_domid_to_name(ctx, domid)) {
-        fprintf(stderr, "unknown domain \'%s\'\n", dom);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown domain '%s'\n", dom);
+        return 1;
     }
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
-        fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown cpupool '%s'\n", pool);
+        return 1;
     }
 
-    return -libxl_cpupool_movedomain(ctx, poolid, domid);
+    if (libxl_cpupool_movedomain(ctx, poolid, domid))
+        return 1;
+
+    return 0;
 }
 
 int main_cpupoolnumasplit(int argc, char **argv)
 {
-    int ret;
+    int rc;
     int opt;
     int p;
     int c;
@@ -7445,13 +7450,13 @@ int main_cpupoolnumasplit(int argc, char **argv)
         /* No options */
     }
 
-    ret = 0;
+    rc = 1;
 
     libxl_bitmap_init(&cpumap);
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {
         fprintf(stderr, "error getting cpupool info\n");
-        return -ERROR_NOMEM;
+        return 1;
     }
     poolid = poolinfo[0].poolid;
     sched = poolinfo[0].sched;
@@ -7460,19 +7465,19 @@ int main_cpupoolnumasplit(int argc, char **argv)
     }
     if (n_pools > 1) {
         fprintf(stderr, "splitting not possible, already cpupools in use\n");
-        return -ERROR_FAIL;
+        return 1;
     }
 
     topology = libxl_get_cpu_topology(ctx, &n_cpus);
     if (topology == NULL) {
         fprintf(stderr, "libxl_get_topologyinfo failed\n");
-        return -ERROR_FAIL;
+        return 1;
     }
 
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
         fprintf(stderr, "Failed to allocate cpumap\n");
         libxl_cputopology_list_free(topology, n_cpus);
-        return -ERROR_FAIL;
+        return 1;
     }
 
     /* Reset Pool-0 to 1st node: first add cpus, then remove cpus to avoid
@@ -7481,12 +7486,11 @@ int main_cpupoolnumasplit(int argc, char **argv)
     node = topology[0].node;
     if (libxl_cpupool_cpuadd_node(ctx, 0, node, &n)) {
         fprintf(stderr, "error on adding cpu to Pool 0\n");
-        return -ERROR_FAIL;
+        return 1;
     }
 
     snprintf(name, 15, "Pool-node%d", node);
-    ret = -libxl_cpupool_rename(ctx, name, 0);
-    if (ret) {
+    if (libxl_cpupool_rename(ctx, name, 0)) {
         fprintf(stderr, "error on renaming Pool 0\n");
         goto out;
     }
@@ -7525,8 +7529,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
         }
 
         node = topology[c].node;
-        ret = -libxl_cpupool_cpuremove_node(ctx, 0, node, &n);
-        if (ret) {
+        if (libxl_cpupool_cpuremove_node(ctx, 0, node, &n)) {
             fprintf(stderr, "error on removing cpu from Pool 0\n");
             goto out;
         }
@@ -7534,14 +7537,12 @@ int main_cpupoolnumasplit(int argc, char **argv)
         snprintf(name, 15, "Pool-node%d", node);
         libxl_uuid_generate(&uuid);
         poolid = 0;
-        ret = -libxl_cpupool_create(ctx, name, sched, cpumap, &uuid, &poolid);
-        if (ret) {
+        if (libxl_cpupool_create(ctx, name, sched, cpumap, &uuid, &poolid)) {
             fprintf(stderr, "error on creating cpupool\n");
             goto out;
         }
 
-        ret = -libxl_cpupool_cpuadd_node(ctx, poolid, node, &n);
-        if (ret) {
+        if (libxl_cpupool_cpuadd_node(ctx, poolid, node, &n)) {
             fprintf(stderr, "error on adding cpus to cpupool\n");
             goto out;
         }
@@ -7553,11 +7554,13 @@ int main_cpupoolnumasplit(int argc, char **argv)
         }
     }
 
+    rc = 0;
+
 out:
     libxl_cputopology_list_free(topology, n_cpus);
     libxl_bitmap_dispose(&cpumap);
 
-    return ret;
+    return rc;
 }
 
 int main_getenforce(int argc, char **argv)

[PATCH v2 7/7] xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

[Dario Faggioli]

instead of manually freeing the elements of the list, which
is exactly the purpose of the said function.

Trade also a couple of 'return'-s with 'goto out'-s, which
is more in line with libxl usage paradigm.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes from v1:
* grammar fixes in the changelog, as requested during
  review.
---
 tools/libxl/xl_cmdimpl.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8587cc2..efb6024 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7460,9 +7460,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
     }
     poolid = poolinfo[0].poolid;
     sched = poolinfo[0].sched;
-    for (p = 0; p < n_pools; p++) {
-        libxl_cpupoolinfo_dispose(poolinfo + p);
-    }
+    libxl_cpupoolinfo_list_free(poolinfo, n_pools);
+
     if (n_pools > 1) {
         fprintf(stderr, "splitting not possible, already cpupools in use\n");
         return 1;
@@ -7476,8 +7475,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
 
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
         fprintf(stderr, "Failed to allocate cpumap\n");
-        libxl_cputopology_list_free(topology, n_cpus);
-        return 1;
+        goto out;
     }
 
     /* Reset Pool-0 to 1st node: first add cpus, then remove cpus to avoid
@@ -7486,7 +7484,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
     node = topology[0].node;
     if (libxl_cpupool_cpuadd_node(ctx, 0, node, &n)) {
         fprintf(stderr, "error on adding cpu to Pool 0\n");
-        return 1;
+        goto out;
     }
 
     snprintf(name, 15, "Pool-node%d", node);

Re: [PATCH v2 3/7] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Wei Liu]

On Fri, Mar 13, 2015 at 12:09:24PM +0100, Dario Faggioli wrote:
> To add (removes) to (from) a cpupool all the pCPUs corresponding
> to the bits that are set in the passed bitmap.
> 
> This is convenient and useful in order to implement, in xl,
> the possibility of specifying ranges of pCPUs to be added
> (removed) to (from) a cpupool, in the relevant commands.
> 
> While there, convert libxl_cpupool_cpu{add,remove} to use the
> appropriate logging macro (LOGE()).
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH v2 4/7] xl: enable using ranges of pCPUs when manipulating cpupools

[Wei Liu]

On Fri, Mar 13, 2015 at 12:09:32PM +0100, Dario Faggioli wrote:
> in fact, right now, xl sub-commands 'cpupool-cpu-add' and
> 'cpupool-cpu-remove' only accept the specification of one
> pCPU to be added or removed to/from a cpupool.
> 
> With this change, they can deal with ranges, like "4-8",
> or "node:1,12-18,^14". The syntax is exactly the same one
> that is supported by the 'vcpu-pin' subcommand, and
> specifying just one pCPU still works, of course.
> 
> This make things more flexible, more consistent, and also
> improves error handling, as the pCPU range parsing routine
> already present in xl is more reliable than just a call
> to atoi().
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH v2 5/7] xl: enable using ranges of pCPUs when creating cpupools

[Wei Liu]

On Fri, Mar 13, 2015 at 12:09:41PM +0100, Dario Faggioli wrote:
> instead of just list of single pCPUs or NUMA node IDs, as
> it happens right now.
> 
> On the other hand, after this change, strings containing
> pCPUs and NUMA node ranges is supported. The syntax is the
> same one supported by the "cpus" and "cpus_soft" config
> switch, i.e., "4-8" or "node:1,12-18,^14".
> 
> This make things more flexible, more consistent, and also
> improves error handling, as the pCPU range parsing routine
> already present in xl is more reliable than just a call
> to atoi().
> 
> While there, remove a redundant error check in the legacy syntax
> handling (libxl_bitmap_test() already checks the index being
> within the size of the bitmap).
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH v2 0/7] Some (not only) cpupool related fixes and improvements

[Ian Campbell]

On Fri, 2015-03-13 at 12:08 +0100, Dario Faggioli wrote:
> Dario Faggioli (7):
>       xl: turn some int local variable into bool
>       xl: add -c/--cpupool option to `xl list'
>       libxl: introduce libxl_cpupool_cpu{add,remove}_cpumap()
>       xl: enable using ranges of pCPUs when manipulating cpupools
>       xl: enable using ranges of pCPUs when creating cpupools
>       xl: make error reporting of cpupool subcommands consistent
>       xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

Applied with Wei's acks.

Ian.