Some (not only) cpupool related fixes and improvements

PATCH Superseded Dario Faggioli 4 participants 40 messages Mar 6, 2015 – Mar 11, 2015

lore

Share & tools

lore

Dario Faggioli

author

2015-03-06 17:20 UTC #0 lore raw

cover Some (not only) cpupool related fixes and improvements

Hi everyone,

The main goal of this series is making it possible to specify ranges of pCPUs when manipulating (creating, adding/removing pCPUs) cpupools. Something like this:

xl cpupool-cpu-remove Pool-node0 6-10

while, right now, only single pCPU IDs can be specified, which means that, to achieve the above, we need to do this:

xl cpupool-cpu-remove Pool-node0 6
xl cpupool-cpu-remove Pool-node0 7
xl cpupool-cpu-remove Pool-node0 8
xl cpupool-cpu-remove Pool-node0 9
xl cpupool-cpu-remove Pool-node0 10

This is done in patches 5/9, 6/9 and 7/9.

The series also add a new parameter to xl list', '-c' or '--cpupool', which shows in which cpupool the domain(s) lives. Such information was not easy to find in any other way. One can use xl sched-<schedule_of_cpupool_x>' and have, as a side effect, something similar, but that's rather impractical.

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

http://pastebin.com/qSagPimL

This happens in patch 3/9 and 4/9.

While there, the series includes some other, somewhat related, fixes of various kind.

This is 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-v1

Thanks and Regards,
Dario

---
Dario Faggioli (9):

  1/9  docs: RTDS is a valid alternative as a scheduler for a cpupool
  2/9  docs: fix `xl list' manpage entry
  3/9  xl: turn some int local variable into bool
  4/9  xl: add -c/--cpupool option to `xl list'
  5/9  libxl: introduce libxl_cpupool_cpu{add,remove}_cpumap()
  6/9  xl: enable using ranges of pCPUs when manipulating cpupools
  7/9  xl: enable using ranges of pCPUs when creating cpupools
  8/9  xl: make error reporting of cpupool subcommands consistent
  9/9  xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

 docs/man/xl.pod.1            |   33 +++++-
 docs/man/xlcpupool.cfg.pod.5 |   26 ++++-
 tools/examples/cpupool       |    2 
 tools/libxl/libxl.c          |   36 ++++++-
 tools/libxl/libxl.h          |    4 +
 tools/libxl/xl_cmdimpl.c     |  225 ++++++++++++++++++++++--------------------
 tools/libxl/xl_cmdtable.c    |    1 
 7 files changed, 209 insertions(+), 118 deletions(-)

Dario Faggioli

author

2015-03-06 17:20 UTC #1 lore raw

PATCH 1/9 docs: RTDS is a valid alternative as a scheduler for a cpupool

so update the relevant manpage and the example file.

Signed-off-by Dario Faggioli <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

Cc Meng Xu <[email protected]>

---
 docs/man/xlcpupool.cfg.pod.5 |    4 ++++
 tools/examples/cpupool       |    2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

docs/man/xlcpupool.cfg.pod.5 +4 -0

index e32ce17..bb15cbe 100644
@@ -74,6 +74,10 @@ the credit scheduler
 
 the credit2 scheduler
 
+=item B<rtds>
+
+the RTDS scheduler
+
 =item B<sedf>
 
 the SEDF scheduler

tools/examples/cpupool +1 -1

index 01e62c8..73368e6 100644
@@ -9,7 +9,7 @@
 # the name of the new cpupool
 name = "Example-Cpupool"
 
-# the scheduler to use: valid are e.g. credit, sedf, credit2
+# the scheduler to use: valid are e.g. credit, credit2, rtds and sedf
 sched = "credit"
 
 # list of cpus to use

Meng Xu

2015-03-06 19:16 UTC #2 lore raw

Re: [PATCH 1/9] docs: RTDS is a valid alternative as a scheduler for a cpupool

2015-03-06 12:20 GMT-05:00 Dario Faggioli <[email protected]>:

Quoted 36 lines

so update the relevant manpage and the example file.
Signed-off-by Dario Faggioli <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Meng Xu <[email protected]>
---
 docs/man/xlcpupool.cfg.pod.5 |    4 ++++
 tools/examples/cpupool       |    2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)
docs/man/xlcpupool.cfg.pod.5 +4 -0
index e32ce17..bb15cbe 100644
@@ -74,6 +74,10 @@ the credit scheduler
 
 the credit2 scheduler
 
+=item B<rtds>
+
+the RTDS scheduler
+
 =item B<sedf>
 
 the SEDF scheduler
tools/examples/cpupool +0 -0
index 01e62c8..73368e6 100644

tools/examples/cpupool line 15

Quoted 8 lines

tools/examples/cpupool +1 -1

@@ -9,7 +9,7 @@
 # the name of the new cpupool
 name = "Example-Cpupool"
 
-# the scheduler to use: valid are e.g. credit, sedf, credit2
+# the scheduler to use: valid are e.g. credit, credit2, rtds and sedf
 sched = "credit"
 
 # list of cpus to use

Reviewed-by: Meng Xu <[email protected]>

Thank you very much!

Best,

Meng

-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Wei Liu

Acked-by on v1 1/9

2015-03-09 10:20 UTC #3 lore raw

Re: [PATCH 1/9] docs: RTDS is a valid alternative as a scheduler for a cpupool

In reply to Dario Faggioli

Quoted 9 lines · Dario Faggioli

so update the relevant manpage and the example file.
Signed-off-by Dario Faggioli <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Meng Xu <[email protected]>

Acked-by Wei Liu <[email protected]>

Quoted 33 lines

---
 docs/man/xlcpupool.cfg.pod.5 |    4 ++++
 tools/examples/cpupool       |    2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

docs/man/xlcpupool.cfg.pod.5 +4 -0

index e32ce17..bb15cbe 100644
@@ -74,6 +74,10 @@ the credit scheduler
 
 the credit2 scheduler
 
+=item B<rtds>
+
+the RTDS scheduler
+
 =item B<sedf>
 
 the SEDF scheduler

tools/examples/cpupool +1 -1

index 01e62c8..73368e6 100644
@@ -9,7 +9,7 @@
 # the name of the new cpupool
 name = "Example-Cpupool"
 
-# the scheduler to use: valid are e.g. credit, sedf, credit2
+# the scheduler to use: valid are e.g. credit, credit2, rtds and sedf
 sched = "credit"
 
 # list of cpus to use

Dario Faggioli

author

2015-03-06 17:21 UTC #4 lore raw

PATCH 2/9 docs: fix `xl list' manpage entry

In reply to Dario Faggioli

as it was not covering the '-n' option, which is present since d743a223 ("xl: add node-affinity to the output of xl list").

Signed-off-by Dario Faggioli <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

---
 docs/man/xl.pod.1 |    4 ++++
 1 file changed, 4 insertions(+)

docs/man/xl.pod.1 +4 -0

index cd80ffc..861ae21 100644
@@ -304,6 +304,10 @@ Also prints the security labels.
 
 Also prints the domain UUIDs, the shutdown reason and security labels.
 
+=item B<-n>, <--numa>
+
+Also prints the domain NUMA node affinity.
+
 =back
 
 B<EXAMPLE>

Wei Liu

Acked-by on v1 2/9

2015-03-09 10:21 UTC #5 lore raw

Re: [PATCH 2/9] docs: fix `xl list' manpage entry

Quoted 9 lines · Dario Faggioli

as it was not covering the '-n' option, which is present since d743a223 ("xl: add node-affinity to the output of xl list").
Signed-off-by Dario Faggioli <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>

Acked-by Wei Liu <[email protected]>

Quoted 19 lines

---
 docs/man/xl.pod.1 |    4 ++++
 1 file changed, 4 insertions(+)

docs/man/xl.pod.1 +4 -0

index cd80ffc..861ae21 100644
@@ -304,6 +304,10 @@ Also prints the security labels.
 
 Also prints the domain UUIDs, the shutdown reason and security labels.
 
+=item B<-n>, <--numa>
+
+Also prints the domain NUMA node affinity.
+
 =back
 
 B<EXAMPLE>

Dario Faggioli

author

2015-03-06 17:21 UTC #6 lore raw

PATCH 3/9 xl: turn some int local variable into bool

In reply to Dario Faggioli

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

Signed-off-by Dario Faggioli <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

---
 tools/libxl/xl_cmdimpl.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

tools/libxl/xl_cmdimpl.c +10 -9

index 5c40e84..69469ff 100644
@@ -3520,7 +3520,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;

@@ -4504,10 +4504,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'},

@@ -4523,16 +4524,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;
     }

Wei Liu

Acked-by on v1 3/9

2015-03-09 10:24 UTC #7 lore raw

Re: [PATCH 3/9] xl: turn some int local variable into bool

Quoted 8 lines · Dario Faggioli

more specifically, the ones used as argument presence flags in `xl list'.
Signed-off-by Dario Faggioli <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>

Acked-by Wei Liu <[email protected]>

Dario Faggioli

author

2015-03-06 17:21 UTC #8 lore raw

PATCH 4/9 xl: add -c/--cpupool option to `xl list'

In reply to 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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

---
 docs/man/xl.pod.1         |    4 ++++
 tools/libxl/xl_cmdimpl.c  |   20 ++++++++++++++++----
 tools/libxl/xl_cmdtable.c |    1 +
 3 files changed, 21 insertions(+), 4 deletions(-)

docs/man/xl.pod.1 +4 -0

index 861ae21..c330016 100644
@@ -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.

tools/libxl/xl_cmdimpl.c +16 -4

index 69469ff..42b3954 100644
@@ -3521,7 +3521,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";

@@ -3535,6 +3535,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");

@@ -3579,6 +3580,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);

@@ -4508,11 +4514,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}

@@ -4522,7 +4530,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;

@@ -4532,6 +4540,9 @@ int main_list(int argc, char **argv)
     case 'Z':
         context = true;
         break;
+    case 'c':
+        cpupool = true;
+        break;
     case 'n':
         numa = true;
         break;

@@ -4566,7 +4577,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, cpupool,
+                     info, nb_domain);
 
     if (info_free)
         libxl_dominfo_list_free(info, nb_domain);

@@ -6617,7 +6629,7 @@ int main_claims(int argc, char **argv)
     }
 
     list_domains(0 /* verbose */, 0 /* context */, 1 /* claim */,
-                 0 /* numa */, info, nb_domain);
+                 0 /* numa */, 0 /* cpupool */, info, nb_domain);
 
     libxl_dominfo_list_free(info, nb_domain);
     return 0;

tools/libxl/xl_cmdtable.c +1 -0

index 22ab63b..9284887 100644
@@ -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",

Wei Liu

Acked-by on v1 4/9

2015-03-09 10:27 UTC #9 lore raw

Re: [PATCH 4/9] xl: add -c/--cpupool option to `xl list'

Quoted 9 lines · 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 <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>
Cc Juergen Gross <[email protected]>

Acked-by Wei Liu <[email protected]>

Only one nit. See below.

tools/libxl/xl_cmdimpl.c int main_claims(int argc, char **argv) · line 6632

Quoted 12 lines

tools/libxl/xl_cmdimpl.c +3 -2

-        list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
+        list_domains(verbose, context, false /* claim */, numa, cpupool,
+                     info, nb_domain);
 
     if (info_free)
         libxl_dominfo_list_free(info, nb_domain);

@@ -6617,7 +6629,7 @@ int main_claims(int argc, char **argv)
     }
 
     list_domains(0 /* verbose */, 0 /* context */, 1 /* claim */,
-                 0 /* numa */, info, nb_domain);
+                 0 /* numa */, 0 /* cpupool */, info, nb_domain);

In previous patch, you changed all types to bool, but you forgot to use "true" / "false" here.

Wei.

Quoted 21 lines

tools/libxl/xl_cmdimpl.c +0 -0

     libxl_dominfo_list_free(info, nb_domain);
     return 0;

tools/libxl/xl_cmdtable.c +1 -0

index 22ab63b..9284887 100644
@@ -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",

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Dario Faggioli

author

2015-03-06 17:21 UTC #10 lore raw

PATCH 5/9 libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

In reply to 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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

---
 tools/libxl/libxl.c |   36 ++++++++++++++++++++++++++++++++----
 tools/libxl/libxl.h |    4 ++++
 2 files changed, 36 insertions(+), 4 deletions(-)

tools/libxl/libxl.c +32 -4

index 088786e..e06fe32 100644
@@ -6343,17 +6343,31 @@ out:
 
 int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
+    GC_INIT(ctx);
     int rc;
 
     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;
     return 0;
 }
 
+int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
+            ncpus++;
+    }
+    return ncpus;
+}
+
 int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
 {
     int rc = 0;

@@ -6388,17 +6402,31 @@ out:
 
 int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
+    GC_INIT(ctx);
     int rc;
 
     rc = xc_cpupool_removecpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error removing cpu from cpupool");
+        LOGE(ERROR, "Error removing cpu %d from cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;
     return 0;
 }
 
+int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                   const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuremove(ctx, poolid, c))
+            ncpus++;
+    }
+    return ncpus;
+}
+
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
 {
     int ret = 0;

tools/libxl/libxl.h +4 -0

index 6bbc52d..7661999 100644
@@ -1456,8 +1456,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);

Wei Liu

2015-03-09 10:39 UTC #11 lore raw

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

Quoted 31 lines · 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 <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>
Cc Juergen Gross <[email protected]>
---
 tools/libxl/libxl.c |   36 ++++++++++++++++++++++++++++++++----
 tools/libxl/libxl.h |    4 ++++
 2 files changed, 36 insertions(+), 4 deletions(-)
tools/libxl/libxl.c +1 -0
index 088786e..e06fe32 100644
@@ -6343,17 +6343,31 @@ out:
 
 int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
+    GC_INIT(ctx);
     int rc;

tools/libxl/libxl.c out: · line 6355

Quoted 10 lines

tools/libxl/libxl.c +3 -2

 
     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;

Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.

Quoted 4 lines

tools/libxl/libxl.c +1 -0

     return 0;
 }
 
+int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,

tools/libxl/libxl.c out: · line 6369

Quoted 10 lines

tools/libxl/libxl.c +10 -0

+                                const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
+            ncpus++;
+    }
+    return ncpus;
+}

I think returning a libxl error code on error and 0 on success is better. At least this stay in line with libxl_cpupool_cpuadd_node.

Quoted 45 lines

tools/libxl/libxl.c +17 -2

+
 int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
 {
     int rc = 0;

@@ -6388,17 +6402,31 @@ out:
 
 int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
+    GC_INIT(ctx);
     int rc;
 
     rc = xc_cpupool_removecpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error removing cpu from cpupool");
+        LOGE(ERROR, "Error removing cpu %d from cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;
     return 0;
 }
 
+int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                   const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuremove(ctx, poolid, c))
+            ncpus++;
+    }
+    return ncpus;
+}
+
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
 {
     int ret = 0;

tools/libxl/libxl.h +0 -0

index 6bbc52d..7661999 100644
@@ -1456,8 +1456,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);

tools/libxl/libxl.h int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid); · line 1467

Quoted 10 lines

tools/libxl/libxl.h +4 -0

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

Missing #define LIBXl_HAVE_$FOO.

Wei.

Dario Faggioli

author

2015-03-09 11:31 UTC #12 lore raw

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

Quoted 11 lines · Wei Liu

Quoted 9 lines · Dario Faggioli

tools/libxl/libxl.c +1 -0

@@ -6343,17 +6343,31 @@ out:
 
 int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
+    GC_INIT(ctx);
     int rc;

Quoted 12 lines

tools/libxl/libxl.c out: · line 6355

Quoted 9 lines

tools/libxl/libxl.c +3 -2

     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;

Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.

Ok.

Quoted 18 lines

Quoted 4 lines
tools/libxl/libxl.c +1 -0
     return 0;
 }
 
+int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
tools/libxl/libxl.c out: · line 6369
Quoted 10 lines
tools/libxl/libxl.c +10 -0
+                                const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
+            ncpus++;
+    }
+    return ncpus;
+}
I think returning a libxl error code on error and 0 on success is better. At least this stay in line with libxl_cpupool_cpuadd_node.

Will do.

Quoted 9 lines

tools/libxl/libxl.h +2 -0

@@ -1456,8 +1456,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);

Quoted 9 lines

tools/libxl/libxl.h int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);

Quoted 6 lines

tools/libxl/libxl.h +2 -0

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

Missing #define LIBXl_HAVE_$FOO.

So, do we need to do this every time we add a new function, even if not changing any existing one, not adding fields to any data structure, etc.?

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Wei Liu

2015-03-09 11:47 UTC #13 lore raw

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

On Mon, Mar 09, 2015 at 11:31:09AM +0000, Dario Faggioli wrote:
[...]

Quoted 18 lines

Quoted 9 lines

tools/libxl/libxl.h +2 -0

@@ -1456,8 +1456,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);

Quoted 9 lines

tools/libxl/libxl.h int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);

Quoted 6 lines

tools/libxl/libxl.h +2 -0

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

Missing #define LIBXl_HAVE_$FOO.

Quoted 4 lines

So, do we need to do this every time we add a new function, even if not changing any existing one, not adding fields to any data structure, etc.?

IMHO this needs one.

Consider a new client which uses this API compiles against old library. The same reason why we add LIBXL_HAVE_$FOO for any structure changes.

Wei.

Quoted 2 lines

Regards,
Dario

Ian Campbell

2015-03-11 16:42 UTC #14 lore raw

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

In reply to Wei Liu

Quoted 11 lines · Wei Liu

     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);

tools/libxl/libxl.c out: · line 6355

Quoted 8 lines

     if (rc) {

tools/libxl/libxl.c +3 -2

-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;

Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.

TBH I think using LIBXL__LOG_ERRNOVAL in functions where the GC is otherwise unneeded in the entire call-chain, like here, is a reasonable exception to the usual rule.

Ian.

Dario Faggioli

author

2015-03-11 16:50 UTC #15 lore raw

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

Quoted 11 lines · Ian Campbell

Quoted 10 lines · Wei Liu

     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);

Quoted 8 lines

     if (rc) {

tools/libxl/libxl.c +3 -2

-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;

Quoted 6 lines

Quoted 1 line
Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.
TBH I think using LIBXL__LOG_ERRNOVAL in functions where the GC is otherwise unneeded in the entire call-chain, like here, is a reasonable exception to the usual rule.

And I distinctly remember someone of you tools people saying, about another patch, that "using the new macro is well worth adding a GC_INIT/FREE" :-D

Of course, I'm fine with leaving this alone... just let me know what you prefer. :-)

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2015-03-11 16:57 UTC #16 lore raw

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

Quoted 14 lines · Dario Faggioli

Quoted 13 lines · Ian Campbell

Quoted 11 lines · Wei Liu

     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);

tools/libxl/libxl.c out: · line 6355

Quoted 8 lines

     if (rc) {

tools/libxl/libxl.c +3 -2

-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;

Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.

Quoted 7 lines

Quoted 4 lines
TBH I think using LIBXL__LOG_ERRNOVAL in functions where the GC is otherwise unneeded in the entire call-chain, like here, is a reasonable exception to the usual rule.
And I distinctly remember someone of you tools people saying, about another patch, that "using the new macro is well worth adding a GC_INIT/FREE" :-D

Could have even be me on another day ;-)

Quoted 2 lines

Of course, I'm fine with leaving this alone... just let me know what you prefer. :-)

Either use GC+goto style error paths or leave it alone, either is fine by me.

Ian.

Dario Faggioli

author

2015-03-06 17:21 UTC #17 lore raw

PATCH 6/9 xl: enable using ranges of pCPUs when manipulating cpupools

In reply to 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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

---
 docs/man/xl.pod.1        |   25 ++++++++++--
 tools/libxl/xl_cmdimpl.c |   94 ++++++++++++++++++++--------------------------
 2 files changed, 62 insertions(+), 57 deletions(-)

docs/man/xl.pod.1 +21 -4

index c330016..ef0d763 100644
@@ -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 I<cpus> or full NUMA I<nodes> to I<cpu-pool>. pCPUs and NUMA nodes
+can be specified as single pCPU/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 pCPU 4 to mypool, in (a); adding pCPUs 1,5,10,11,12,14,15
+and 16, in (b); and adding all the pCPUs of NUMA nodes 0, 2 and 3,
+plus pCPU 8, but keeping out pCPUs 10,11,12, in (c).
+
+All the specified pCPUs that can be added to the cpupool will be added
+to it. If some pCPU 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 I<cpus> or full NUMA I<nodes> from I<cpu-pool>. pCPUs and NUMA
+nodes can be specified as single pCPU/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>

tools/libxl/xl_cmdimpl.c +41 ~~-53~~

164 lines not on this page · raw message

Wei Liu

2015-03-09 10:51 UTC #18 lore raw

Re: [PATCH 6/9] xl: enable using ranges of pCPUs when manipulating cpupools

Quoted 30 lines · 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 <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>
Cc Juergen Gross <[email protected]>
---
 docs/man/xl.pod.1        |   25 ++++++++++--
 tools/libxl/xl_cmdimpl.c |   94 ++++++++++++++++++++--------------------------
 2 files changed, 62 insertions(+), 57 deletions(-)
docs/man/xl.pod.1 +0 -0
index c330016..ef0d763 100644

docs/man/xl.pod.1 This is possible only if no domain is active in the cpu-pool. · line 1152

Quoted 8 lines

docs/man/xl.pod.1 +2 -2

@@ -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 I<cpus> or full NUMA I<nodes> to I<cpu-pool>. pCPUs and NUMA nodes

                   ^^^^

You can also specify some cpus inside a NUMA node.

Quoted 11 lines

docs/man/xl.pod.1 +9 -1

+can be specified as single pCPU/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 pCPU 4 to mypool, in (a); adding pCPUs 1,5,10,11,12,14,15
+and 16, in (b); and adding all the pCPUs of NUMA nodes 0, 2 and 3,

docs/man/xl.pod.1 This is possible only if no domain is active in the cpu-pool. · line 1171

Quoted 10 lines

docs/man/xl.pod.1 +8 -1

+plus pCPU 8, but keeping out pCPUs 10,11,12, in (c).
+
+All the specified pCPUs that can be added to the cpupool will be added
+to it. If some pCPU 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 I<cpus> or full NUMA I<nodes> from I<cpu-pool>. pCPUs and NUMA

Ditto.

docs/man/xl.pod.1 This is possible only if no domain is active in the cpu-pool. · line 1176

Quoted 5 lines

docs/man/xl.pod.1 +2 -0

+nodes can be specified as single pCPU/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>

The changes look good to me.

Wei.

Dario Faggioli

author

2015-03-09 11:37 UTC #19 lore raw

Re: [PATCH 6/9] xl: enable using ranges of pCPUs when manipulating cpupools

Quoted 15 lines · Wei Liu

Quoted 13 lines · Dario Faggioli

---
 docs/man/xl.pod.1        |   25 ++++++++++--
 tools/libxl/xl_cmdimpl.c |   94 ++++++++++++++++++++--------------------------
 2 files changed, 62 insertions(+), 57 deletions(-)

docs/man/xl.pod.1 +0 -0

index c330016..ef0d763 100644
@@ -1147,13 +1147,30 @@ This is possible only if no domain is active in the cpu-pool.
 
 Renames a cpu-pool to I<newname>.

Quoted 8 lines

docs/man/xl.pod.1 This is possible only if no domain is active in the cpu-pool. · line 1152
Quoted 5 lines
docs/man/xl.pod.1 +2 -2
-=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 I<cpus> or full NUMA I<nodes> to I<cpu-pool>. pCPUs and NUMA nodes
                   ^^^^
You can also specify some cpus inside a NUMA node.

Well, if you use "node:1,2", that means _all_ the cpus of NUMA node 1 and 2. Then, of course, you can add only some of then by just saying "0,1,8,9", or something like "node:1,^2-7", so yes, of course you can specify only some of the cpus of a NUMA node, but that was meant at describing what "node:" does.

That being said, I'll look for a better wording, or just use

"Adds pCPUs and/or NUMA nodes to I<cpu-pool>"

and leave it to the more detailed explanation and to the examples below.

Quoted 17 lines

Quoted 7 lines

docs/man/xl.pod.1 +5 -1

+can be specified as single pCPU/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

docs/man/xl.pod.1 This is possible only if no domain is active in the cpu-pool. · line 1168

Quoted 10 lines

docs/man/xl.pod.1 +10 -0

+ (c) xl cpupool-cpu-add mypool node:0,nodes:2-3,^10-12,8
+
+means adding pCPU 4 to mypool, in (a); adding pCPUs 1,5,10,11,12,14,15
+and 16, in (b); and adding all the pCPUs of NUMA nodes 0, 2 and 3,
+plus pCPU 8, but keeping out pCPUs 10,11,12, in (c).
+
+All the specified pCPUs that can be added to the cpupool will be added
+to it. If some pCPU can't (e.g., because they're already part of another
+cpupool), an error is reported about each one of them.
+

Thanks and Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Dario Faggioli

author

2015-03-06 17:21 UTC #20 lore raw

PATCH 7/9 xl: enable using ranges of pCPUs when creating cpupools

In reply to 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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

---
 docs/man/xlcpupool.cfg.pod.5 |   22 +++++++++++++++++++---
 tools/libxl/xl_cmdimpl.c     |   17 ++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)

docs/man/xlcpupool.cfg.pod.5 +19 -3

index bb15cbe..2ff8ee8 100644
@@ -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.

tools/libxl/xl_cmdimpl.c +14 -3

index c748ba0..e6d1234 100644
@@ -7163,18 +7163,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;

Wei Liu

2015-03-09 10:58 UTC #21 lore raw

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

Quoted 41 lines · 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 <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>
Cc Juergen Gross <[email protected]>
---
 docs/man/xlcpupool.cfg.pod.5 |   22 +++++++++++++++++++---
 tools/libxl/xl_cmdimpl.c     |   17 ++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)
docs/man/xlcpupool.cfg.pod.5 +1 -3
index bb15cbe..2ff8ee8 100644
@@ -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>.

docs/man/xlcpupool.cfg.pod.5 Specifies the cpus of the NUMA-nodes given in C<NODES> (an integer or · line 107

Quoted 10 lines

docs/man/xlcpupool.cfg.pod.5 +10 -0

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

I suppose this is the old syntax?

I'm asking because I want to be sure we still support the old syntax. I think we still support the old syntax from the look of the changes below but I'd better get confirmation from you.

Quoted 25 lines

docs/man/xlcpupool.cfg.pod.5 +8 -0

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

tools/libxl/xl_cmdimpl.c +1 -2

index c748ba0..e6d1234 100644
@@ -7163,18 +7163,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) ||

tools/libxl/xl_cmdimpl.c int main_cpupoolcreate(int argc, char **argv) · line 7178

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +3 -1

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

Perhaps print something to say the cpu range specified is invalid?

Wei.

Quoted 13 lines

tools/libxl/xl_cmdimpl.c +10 -0

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

Dario Faggioli

author

2015-03-09 11:18 UTC #22 lore raw

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

Quoted 18 lines · Wei Liu

Quoted 16 lines · Dario Faggioli

---
 docs/man/xlcpupool.cfg.pod.5 |   22 +++++++++++++++++++---
 tools/libxl/xl_cmdimpl.c     |   17 ++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)

docs/man/xlcpupool.cfg.pod.5 +1 -1

index bb15cbe..2ff8ee8 100644
@@ -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">

Quoted 15 lines

docs/man/xlcpupool.cfg.pod.5 Specifies the cpus of the NUMA-nodes given in C<NODES> (an integer or · line 107
Quoted 12 lines
docs/man/xlcpupool.cfg.pod.5 +10 -2
-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.
+
I suppose this is the old syntax?

It is.

Quoted 4 lines

I'm asking because I want to be sure we still support the old syntax. I think we still support the old syntax from the look of the changes below but I'd better get confirmation from you.

I also think we should, and, in fact, we do. :-)

Quoted 29 lines

docs/man/xlcpupool.cfg.pod.5 +8 -0

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

tools/libxl/xl_cmdimpl.c +2 -3

index c748ba0..e6d1234 100644
@@ -7163,18 +7163,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;

Quoted 9 lines

tools/libxl/xl_cmdimpl.c int main_cpupoolcreate(int argc, char **argv) · line 7178
Quoted 6 lines
tools/libxl/xl_cmdimpl.c +2 -0
             }
             libxl_bitmap_set(&cpumap, i);
             n_cpus++;
         }
+    } else if (!xlu_cfg_get_string(config, "cpus", &buf, 0)) {
+        if (cpurange_parse(buf, &cpumap))
Perhaps print something to say the cpu range specified is invalid?

It happens already inside cpurange_parse() (well, to be precise, it's inside update_cpumap_range() ), which is in a better position for printing the most meaningful error messages.

If you check any other call site of cpurange_parse(), none prints anything on failure, exactly for that reason.

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Wei Liu

2015-03-09 11:20 UTC #23 lore raw

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

On Mon, Mar 09, 2015 at 11:18:57AM +0000, Dario Faggioli wrote:
[...]

Quoted 4 lines

Quoted 4 lines
Quoted 1 line
+
I suppose this is the old syntax?

Quoted 8 lines

It is.
Quoted 4 lines
I'm asking because I want to be sure we still support the old syntax. I think we still support the old syntax from the look of the changes below but I'd better get confirmation from you.
I also think we should, and, in fact, we do. :-)

Thanks for confirming.

Quoted 38 lines

Quoted 29 lines

+=item "0-3,5,^1"

docs/man/xlcpupool.cfg.pod.5 +7 -0

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

tools/libxl/xl_cmdimpl.c +2 -3

index c748ba0..e6d1234 100644
@@ -7163,18 +7163,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;

Quoted 9 lines

tools/libxl/xl_cmdimpl.c int main_cpupoolcreate(int argc, char **argv) · line 7178
Quoted 6 lines
tools/libxl/xl_cmdimpl.c +2 -0
             }
             libxl_bitmap_set(&cpumap, i);
             n_cpus++;
         }
+    } else if (!xlu_cfg_get_string(config, "cpus", &buf, 0)) {
+        if (cpurange_parse(buf, &cpumap))
Perhaps print something to say the cpu range specified is invalid?

Quoted 6 lines

It happens already inside cpurange_parse() (well, to be precise, it's inside update_cpumap_range() ), which is in a better position for printing the most meaningful error messages.
If you check any other call site of cpurange_parse(), none prints anything on failure, exactly for that reason.

OK then.

Wei.

Quoted 3 lines

Regards,
Dario

Dario Faggioli

author

2015-03-06 17:21 UTC #24 lore raw

PATCH 8/9 xl: make error reporting of cpupool subcommands consistent

In reply to 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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

---
 tools/libxl/xl_cmdimpl.c |   69 ++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

tools/libxl/xl_cmdimpl.c +36 ~~-33~~

index e6d1234..14b5a3d 100644
@@ -7034,7 +7034,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':

@@ -7228,7 +7228,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':

@@ -7240,14 +7239,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");

@@ -7257,7 +7256,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;

@@ -7278,7 +7277,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)

@@ -7295,11 +7294,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)

@@ -7317,14 +7319,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;
     }

@@ -7426,22 +7428,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;

@@ -7462,13 +7467,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;

@@ -7477,19 +7482,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

@@ -7498,12 +7503,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;
     }

@@ -7542,8 +7546,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;
         }

@@ -7551,14 +7554,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;
         }

@@ -7570,11 +7571,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)

Wei Liu

2015-03-09 11:01 UTC #25 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 3 lines · Dario Faggioli

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

While being consistent is good I'm not very sure if we should go for 0/1 rather than libxl error codes. I vaguely remember at some point we discussed we should make xl exit code better but I don't remember the exact details.

Ian and Ian, what do you think?

Wei.

Dario Faggioli

author

2015-03-09 11:23 UTC #26 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 8 lines · Wei Liu

Quoted 3 lines · Dario Faggioli
with the rest of the file, where we return 1 on 0, rather than using libxl error codes.
While being consistent is good I'm not very sure if we should go for 0/1 rather than libxl error codes.

If there is a plan to switch everything in xl to use libxl error code, then just let me know, ad I'll drop this patch.

As far as I could see, there is __A__ __LOT__ of 'return 0/1' (or 'exit(1)'), and only these few libxl error code usage, so it's really inconsistent! :-)

Quoted 4 lines

I vaguely remember at some point we
discussed we should make xl exit code better but I don't remember the exact details.

As I said, just let me know.

Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2015-03-11 14:52 UTC #27 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

In reply to Wei Liu

Quoted 11 lines · Wei Liu

Quoted 3 lines · Dario Faggioli
with the rest of the file, where we return 1 on 0, rather than using libxl error codes.
While being consistent is good I'm not very sure if we should go for 0/1 rather than libxl error codes. I vaguely remember at some point we discussed we should make xl exit code better but I don't remember the exact details.
Ian and Ian, what do you think?

TBH I'm not sure what exit() called with a negative number even results in.

I think having more consistent exist codes from xl would be nice, but I don't think the libxl error codes are the ones to use, since they don't really map semantically onto what I would expect a CLI tool to fail with (I'm not sure what I would expect though, something a bit higher level on a command specific basis probably).

Wei Liu

2015-03-11 15:04 UTC #28 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 13 lines · Ian Campbell

Quoted 11 lines · Wei Liu
Quoted 3 lines · Dario Faggioli
with the rest of the file, where we return 1 on 0, rather than using libxl error codes.
While being consistent is good I'm not very sure if we should go for 0/1 rather than libxl error codes. I vaguely remember at some point we discussed we should make xl exit code better but I don't remember the exact details.
Ian and Ian, what do you think?

Quoted 9 lines

TBH I'm not sure what exit() called with a negative number even results in.
I think having more consistent exist codes from xl would be nice, but I don't think the libxl error codes are the ones to use, since they don't really map semantically onto what I would expect a CLI tool to fail with (I'm not sure what I would expect though, something a bit higher level on a command specific basis probably).

I agree that libxl error codes are not the ones to use.

Since we haven't explicitly defined any return value in xl manpage, I think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). They are more appropriate then 0 and 1.

Wei.

Dario Faggioli

author

2015-03-11 15:13 UTC #29 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 9 lines · Wei Liu

Quoted 6 lines · Ian Campbell
I think having more consistent exist codes from xl would be nice, but I don't think the libxl error codes are the ones to use, since they don't really map semantically onto what I would expect a CLI tool to fail with (I'm not sure what I would expect though, something a bit higher level on a command specific basis probably).

Quoted 5 lines

I agree that libxl error codes are not the ones to use.
Since we haven't explicitly defined any return value in xl manpage, I think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3).

I like the idea.

Quoted 3 lines

They are
more appropriate then 0 and 1.

Indeed. However, as far as this patch is concerned, what should I do?

a) drop it, and leave libxl error code in place, until we convert everything to EXIT_SUCCESS/FAILURE
b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later patch(es)
c) turn libxl error codes into EXIT_SUCCESS/FAILURE at least for this functions, since I'm touching them, the rest will come with later patch(es)
?

My opinion, I don't like c), so I'd go for either a) or b), with a slight preference for b).

Let me know...

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Wei Liu

2015-03-11 15:23 UTC #30 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 31 lines · Dario Faggioli

Quoted 9 lines · Wei Liu
Quoted 6 lines · Ian Campbell
I think having more consistent exist codes from xl would be nice, but I don't think the libxl error codes are the ones to use, since they don't really map semantically onto what I would expect a CLI tool to fail with (I'm not sure what I would expect though, something a bit higher level on a command specific basis probably).
Quoted 5 lines
I agree that libxl error codes are not the ones to use.
Since we haven't explicitly defined any return value in xl manpage, I think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3).
I like the idea.
Quoted 3 lines
They are
more appropriate then 0 and 1.
Indeed. However, as far as this patch is concerned, what should I do?
a) drop it, and leave libxl error code in place, until we convert everything to EXIT_SUCCESS/FAILURE
b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later patch(es)
c) turn libxl error codes into EXIT_SUCCESS/FAILURE at least for this functions, since I'm touching them, the rest will come with later patch(es)
?

Quoted 3 lines

My opinion, I don't like c), so I'd go for either a) or b), with a slight preference for b).

I think b) is good.

Quoted 4 lines

Let me know...
Regards,
Dario

Dario Faggioli

author

2015-03-11 16:22 UTC #31 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 16 lines · Wei Liu

Quoted 14 lines · Dario Faggioli
Quoted 3 lines · Wei Liu
Since we haven't explicitly defined any return value in xl manpage, I think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3).
I like the idea.
Quoted 3 lines
They are
more appropriate then 0 and 1.
Indeed. However, as far as this patch is concerned, what should I do?
b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later patch(es)

Quoted 8 lines

Quoted 5 lines
?
My opinion, I don't like c), so I'd go for either a) or b), with a slight preference for b).
I think b) is good.

Right. It was the only comment you made on this patch... does that means I can stick your Acked/Reviewed -by tag to it when sending v2?

Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Wei Liu

Acked-by

2015-03-11 16:24 UTC #32 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 19 lines · Dario Faggioli

Quoted 18 lines · Wei Liu
Quoted 17 lines · Dario Faggioli
Quoted 3 lines · Wei Liu
Since we haven't explicitly defined any return value in xl manpage, I think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3).
I like the idea.
Quoted 3 lines
They are
more appropriate then 0 and 1.
Indeed. However, as far as this patch is concerned, what should I do?
b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later patch(es)
?

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
My opinion, I don't like c), so I'd go for either a) or b), with a slight preference for b).
I think b) is good.
Right. It was the only comment you made on this patch... does that means I can stick your Acked/Reviewed -by tag to it when sending v2?

Yes.

Acked-by Wei Liu <[email protected]>

Quoted 1 line

Dario

Dario Faggioli

author

2015-03-11 16:28 UTC #33 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

Quoted 5 lines · Wei Liu

Quoted 4 lines · Dario Faggioli
Quoted 2 lines · Wei Liu
I think b) is good.

Quoted 8 lines

Quoted 3 lines
Right. It was the only comment you made on this patch... does that means I can stick your Acked/Reviewed -by tag to it when sending v2?
Yes.
Acked-by Wei Liu <[email protected]>

Thanks! :-)

Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2015-03-11 15:14 UTC #34 lore raw

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

In reply to Wei Liu

Quoted 24 lines · Wei Liu

Quoted 13 lines · Ian Campbell
Quoted 11 lines · Wei Liu
Quoted 3 lines · Dario Faggioli
with the rest of the file, where we return 1 on 0, rather than using libxl error codes.
While being consistent is good I'm not very sure if we should go for 0/1 rather than libxl error codes. I vaguely remember at some point we discussed we should make xl exit code better but I don't remember the exact details.
Ian and Ian, what do you think?
Quoted 9 lines
TBH I'm not sure what exit() called with a negative number even results in.
I think having more consistent exist codes from xl would be nice, but I don't think the libxl error codes are the ones to use, since they don't really map semantically onto what I would expect a CLI tool to fail with (I'm not sure what I would expect though, something a bit higher level on a command specific basis probably).

Quoted 5 lines

I agree that libxl error codes are not the ones to use.
Since we haven't explicitly defined any return value in xl manpage, I think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). They are more appropriate then 0 and 1.

I guess so, yes.

Ian.

Dario Faggioli

author

2015-03-06 17:22 UTC #35 lore raw

PATCH 9/9 xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

In reply to Dario Faggioli

instead manually free 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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

---
 tools/libxl/xl_cmdimpl.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

tools/libxl/xl_cmdimpl.c +4 -6

index 14b5a3d..ec2b6a9 100644
@@ -7477,9 +7477,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;

@@ -7493,8 +7492,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

@@ -7503,7 +7501,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);

Wei Liu

Acked-by on v1 9/9

2015-03-09 11:02 UTC #36 lore raw

Re: [PATCH 9/9] xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

Quoted 6 lines · Dario Faggioli

instead manually free 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.

Quoted 6 lines

Signed-off-by Dario Faggioli <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Wei Liu <[email protected]>
Cc Juergen Gross <[email protected]>

Acked-by Wei Liu <[email protected]>

Ian Campbell

2015-03-11 14:54 UTC #37 lore raw

Re: [PATCH 9/9] xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

In reply to Dario Faggioli

Quoted 1 line · Dario Faggioli

instead manually free the elements of the list, which is

^of freeing

Quoted 56 lines

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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Wei Liu <[email protected]>

Cc Juergen Gross <[email protected]>

---
 tools/libxl/xl_cmdimpl.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

tools/libxl/xl_cmdimpl.c +4 -6

index 14b5a3d..ec2b6a9 100644
@@ -7477,9 +7477,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;

@@ -7493,8 +7492,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

@@ -7503,7 +7501,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);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2015-03-11 14:56 UTC #38 lore raw

In reply to Dario Faggioli

Quoted 39 lines · Dario Faggioli

Hi everyone,
The main goal of this series is making it possible to specify ranges of pCPUs when manipulating (creating, adding/removing pCPUs) cpupools. Something like this:
xl cpupool-cpu-remove Pool-node0 6-10
while, right now, only single pCPU IDs can be specified, which means that, to achieve the above, we need to do this:
xl cpupool-cpu-remove Pool-node0 6
xl cpupool-cpu-remove Pool-node0 7
xl cpupool-cpu-remove Pool-node0 8
xl cpupool-cpu-remove Pool-node0 9
xl cpupool-cpu-remove Pool-node0 10
This is done in patches 5/9, 6/9 and 7/9.
The series also add a new parameter to xl list', '-c' or '--cpupool', which shows in which cpupool the domain(s) lives. Such information was not easy to find in any other way. One can use xl sched-<schedule_of_cpupool_x>' and have, as a side effect, something similar, but that's rather impractical.
Sample output for the new `xl list' can be found here:
http://pastebin.com/qSagPimL
This happens in patch 3/9 and 4/9.
While there, the series includes some other, somewhat related, fixes of various kind.
This is 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-v1

Quoted 7 lines

Thanks and Regards,
Dario
---
Dario Faggioli (9):
1/9 docs: RTDS is a valid alternative as a scheduler for a cpupool 2/9 docs: fix `xl list' manpage entry

Acked + applied these two.

Quoted 1 line

3/9 xl: turn some int local variable into bool

I'd have applied this with Wei's ack but he latter noticed that some 0/1 hadn't been converted so I thought I'd give you the chance to update if you wanted.

For the rest I think there are outstanding comments to be addressed.

Dario Faggioli

author

2015-03-11 15:09 UTC #39 lore raw

Quoted 9 lines · Ian Campbell

Quoted 4 lines · Dario Faggioli
---
Dario Faggioli (9):
1/9 docs: RTDS is a valid alternative as a scheduler for a cpupool 2/9 docs: fix `xl list' manpage entry
Acked + applied these two.

Thanks.

Quoted 6 lines

Quoted 1 line
3/9 xl: turn some int local variable into bool
I'd have applied this with Wei's ack but he latter noticed that some 0/1 hadn't been converted so I thought I'd give you the chance to update if you wanted.

I am preparing v2, I was just waiting to hear from you and/or Ian about the xl exist code bit.

Quoted 2 lines

For the rest I think there are outstanding comments to be addressed.

They are. Will send v2.

Thanks again and Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel