libxl: Extend CPU affinity specification and enable it in config file.

Allow for "^<cpuid>" syntax while specifying the pCPUs list during a vcpu-pin. This enables doing the following:

xl vcpu-pin 1 1 0-4,^2

and achieving:

xl vcpu-list
Name ID VCPU CPU State Time(s) CPU Affinity
...
Squeeze_pv 1 1 3 -b- 2.4 0-1,3-4
...

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

diff -r 9cdcedc133e5 tools/libxl/libxl_utils.h

tools/libxl/libxl_utils.h +2 -0

@@ -71,6 +71,8 @@ int libxl_cpumap_test(libxl_cpumap *cpum
 void libxl_cpumap_set(libxl_cpumap *cpumap, int cpu);
 void libxl_cpumap_reset(libxl_cpumap *cpumap, int cpu);
 #define libxl_for_each_cpu(var, map) for (var = 0; var < (map).size * 8; var++)
+#define libxl_for_each_set_cpu(var, map) for (var = 0; var < (map).size * 8; var++) \
+                                             if (libxl_cpumap_test(&(map), var))
 
 int libxl_cpuarray_alloc(libxl_ctx *ctx, libxl_cpuarray *cpuarray);
 

diff -r 9cdcedc133e5 tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +59 -28

@@ -3501,13 +3501,67 @@ int main_vcpulist(int argc, char **argv)
     return 0;
 }
 
+static int vcpupin_parse(char *cpu, libxl_cpumap *cpumap)
+{
+    libxl_cpumap exclude_cpumap;
+    uint32_t cpuida, cpuidb;
+    char *endptr, *toka, *tokb;
+    int i, rmcpu, ret = 0;
+
+    if (libxl_cpumap_alloc(ctx, &exclude_cpumap))
+        return ENOMEM;
+
+    if (strcmp(cpu, "all")) {
+        for (toka = strtok(cpu, ","), i = 0; toka; toka = strtok(NULL, ","), ++i) {
+            rmcpu = 0;
+            if (*toka == '^') {
+                toka++; rmcpu = 1;
+            }
+            cpuida = strtoul(toka, &endptr, 10);
+            if (toka == endptr) {
+                fprintf(stderr, "Error: Invalid argument.\n");
+                ret = EINVAL;
+                goto vcpp_out;
+            }
+            if (rmcpu) {
+                libxl_cpumap_set(&exclude_cpumap, cpuida);
+            } else if (*endptr == '-') {
+                tokb = endptr + 1;
+                cpuidb = strtoul(tokb, &endptr, 10);
+                if ((tokb == endptr) || (cpuida > cpuidb)) {
+                    fprintf(stderr, "Error: Invalid argument.\n");
+                    ret = EINVAL;
+                    goto vcpp_out;
+                }
+                while (cpuida <= cpuidb) {
+                    libxl_cpumap_set(cpumap, cpuida);
+                    ++cpuida;
+                }
+            } else {
+                libxl_cpumap_set(cpumap, cpuida);
+            }
+        }
+
+        libxl_for_each_set_cpu(i, exclude_cpumap) {
+            libxl_cpumap_reset(cpumap, i);
+        }
+    } else {
+        memset(cpumap->map, -1, cpumap->size);
+    }
+
+vcpp_out:
+    libxl_cpumap_dispose(&exclude_cpumap);
+
+    return ret;
+}
+
 static void vcpupin(const char *d, const char *vcpu, char *cpu)
 {
     libxl_vcpuinfo *vcpuinfo;
     libxl_cpumap cpumap;
 
-    uint32_t vcpuid, cpuida, cpuidb;
-    char *endptr, *toka, *tokb;
+    uint32_t vcpuid;
+    char *endptr;
     int i, nb_vcpu;
 
     vcpuid = strtoul(vcpu, &endptr, 10);

@@ -3524,32 +3578,9 @@ static void vcpupin(const char *d, const
     if (libxl_cpumap_alloc(ctx, &cpumap)) {
         goto vcpupin_out;
     }
-    if (strcmp(cpu, "all")) {
-        for (toka = strtok(cpu, ","), i = 0; toka; toka = strtok(NULL, ","), ++i) {
-            cpuida = strtoul(toka, &endptr, 10);
-            if (toka == endptr) {
-                fprintf(stderr, "Error: Invalid argument.\n");
-                goto vcpupin_out1;
-            }
-            if (*endptr == '-') {
-                tokb = endptr + 1;
-                cpuidb = strtoul(tokb, &endptr, 10);
-                if ((tokb == endptr) || (cpuida > cpuidb)) {
-                    fprintf(stderr, "Error: Invalid argument.\n");
-                    goto vcpupin_out1;
-                }
-                while (cpuida <= cpuidb) {
-                    libxl_cpumap_set(&cpumap, cpuida);
-                    ++cpuida;
-                }
-            } else {
-                libxl_cpumap_set(&cpumap, cpuida);
-            }
-        }
-    }
-    else {
-        memset(cpumap.map, -1, cpumap.size);
-    }
+
+    if (vcpupin_parse(cpu, &cpumap))
+        goto vcpupin_out1;
 
     if (vcpuid != -1) {
         if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, &cpumap) == -1) {

Allow for "^<cpuid>" syntax while specifying the pCPUs list during a vcpu-pin. This enables doing the following:

xl vcpu-pin 1 1 0-4,^2

Dario Faggioli writes ("[Xen-devel] [PATCH 1 of 3] libxl: extend pCPUs specification for vcpu-pin."):

Quoted 4 lines

Allow for "^<cpuid>" syntax while specifying the pCPUs list during a vcpu-pin. This enables doing the following:

xl vcpu-pin 1 1 0-4,^2

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+    if (strcmp(cpu, "all")) {
+        for (toka = strtok(cpu, ","), i = 0; toka; toka = strtok(NULL, ","), ++i) {

OMG you used strtok. strtok is not thread-safe.

strtok_r is but
still modifies its input string - hence your need to strdup. If you do want to do it this way IMO the strdup should be in vcpupin_parse which should take a const char*. Are you sure this is the right approach to parsing this ?

<record type="broken">
Your patch 2 has a number of overly long lines.
</record>

Well, that's not properly me, is it that?

hg annotate tools/libxl/xl_cmdimpl.c
...
21247: if (strcmp(cpu, "all")) {
21247: for (toka = strtok(cpu, ","), i = 0; toka; toka = strtok(NULL, ","), ++i) {
21247: cpuida = strtoul(toka, &endptr, 10);
21247: if (toka == endptr) {
...

Again, I know that's probably not an alibi, but that's not my code! :-P What I did is just move this from the body of vcpupin() to an helper function, and try to add as _less_ thing as I can to support a slightly improved syntax.

That being said, if you think I should rewrite the parsing from scratch, I can think about it, just wanted to clarify my position. :-)

Oops, sorry for wrongly pointing the finger.

Quoted 2 lines

That being said, if you think I should rewrite the parsing from scratch, I can think about it, just wanted to clarify my position. :-)

I really don't like the existing code here; feel free to do whatever you think is best.

Ok, I'm not really a "string parsing guy" but I'm interested in getting better. I can try restructuring this with strtok_r and strdup... Would that be better or you where thinking to something more "radical"?

Dario Faggioli writes ("Re: [Xen-devel] [PATCH 1 of 3] libxl: extend pCPUs specification for vcpu-pin."):

Quoted 3 lines

Ok, I'm not really a "string parsing guy" but I'm interested in getting better. I can try restructuring this with strtok_r and strdup... Would that be better or you where thinking to something more "radical"?

Personally I'm a fan of flex although it's probably overkill for this. But you should use an approach your comfortable with.

What I care most about is that whatever approach you take doesn't make it too easy to make and hide :-) programming mistakes, since otherwise parsing code often turns out to be buggy.

Can we take the version which just moves the dodgy parsee now and worry about reworking it later? I think we'd like to see vcpu pinning in 4.2.

Quoted 3 lines · Ian Campbell

Can we take the version which just moves the dodgy parsee now and worry about reworking it later? I think we'd like to see vcpu pinning in 4.2.

Hey, sorry for being so late in resubmitting, I've been distrcted by those (other) NUME issues... I'll send a new version of this in the afternoon!

Sorry again,

Enable CPU affinity specification in a VM's config file with the exact syntax `xl vcpu-pin' provides.

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

diff -r 9ce68a4036b1 tools/libxl/libxl_create.c

tools/libxl/libxl_create.c +0 -0

     b_info->max_memkb = 32 * 1024;
     b_info->target_memkb = b_info->max_memkb;
     b_info->disable_migrate = 0;

diff -r 9ce68a4036b1 tools/libxl/libxl_dom.c

tools/libxl/libxl_dom.c +7 -1

@@ -72,8 +72,14 @@ int libxl__build_pre(libxl__gc *gc, uint
               libxl_domain_build_info *info, libxl__domain_build_state *state)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    int tsc_mode;
+    int i, tsc_mode;
     xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
+    for (i = 0; i < info->max_vcpus; i++) {
+        if (libxl_set_vcpuaffinity(ctx, domid, i, &info->cpumap) == -1) {
+                LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "libxl_set_vcpuaffinity failed. "
+                           "Going ahead without setting affinity for cpu %d.\n", i);
+        }
+    }4b
     xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + LIBXL_MAXMEM_CONSTANT);
     if (info->type == LIBXL_DOMAIN_TYPE_PV)
         xc_domain_set_memmap_limit(ctx->xch, domid,

diff -r 9ce68a4036b1 tools/libxl/libxl_types.idl

tools/libxl/libxl_types.idl +1 -0

@@ -162,6 +162,7 @@ libxl_domain_create_info = Struct("domai
 libxl_domain_build_info = Struct("domain_build_info",[
     ("max_vcpus",       integer),
     ("cur_vcpus",       integer),
+    ("cpumap",          libxl_cpumap),
     ("tsc_mode",        libxl_tsc_mode),
     ("max_memkb",       uint32),
     ("target_memkb",    uint32),

diff -r 9ce68a4036b1 tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +11 -1

+
 static void printf_info(int domid,
                         libxl_domain_config *d_config,
                         libxl_device_model_info *dm_info)
 {
-    int i;
+    int i, nr_cpus = -1;
     libxl_dominfo info;
+    libxl_physinfo physinfo;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
     libxl_domain_build_info *b_info = &d_config->b_info;
 
+    if (libxl_get_physinfo(ctx, &physinfo) == 0)
+        nr_cpus = physinfo.nr_cpus;
+    else
+        fprintf(stderr, "libxl_physinfo failed.\n");
+
     printf("(domain\n\t(domid %d)\n", domid);
     printf("\t(create_info)\n");
     printf("\t(hvm %d)\n", c_info->type == LIBXL_DOMAIN_TYPE_HVM);

@@ -328,6 +336,9 @@ static void printf_info(int domid,
 
     printf("\t(build_info)\n");
     printf("\t(max_vcpus %d)\n", b_info->max_vcpus);
+    printf("\t(CPU affinity ");
+    print_bitmap(b_info->cpumap.map, nr_cpus, stdout);
+    printf(")\n");
     printf("\t(tsc_mode %s)\n", libxl_tsc_mode_to_string(b_info->tsc_mode));
     printf("\t(max_memkb %d)\n", b_info->max_memkb);
     printf("\t(target_memkb %d)\n", b_info->target_memkb);

tools/libxl/xl_cmdimpl.c +2 -0

+    }
+
     if (!xlu_cfg_get_long (config, "memory", &l, 0)) {
         b_info->max_memkb = l * 1024;
         b_info->target_memkb = b_info->max_memkb;

tools/libxl/libxl_create.c int libxl_init_build_info(libxl_ctx *ctx · line 82

Quoted 9 lines

diff -r 9ce68a4036b1 tools/libxl/libxl_create.c

tools/libxl/libxl_create.c +2 -0

@@ -78,6 +78,8 @@ int libxl_init_build_info(libxl_ctx *ctx
     memset(b_info, '\0', sizeof(*b_info));
     b_info->max_vcpus = 1;
     b_info->cur_vcpus = 1;
+    if (libxl_cpumap_alloc(ctx, &b_info->cpumap))
+        return ERROR_NOMEM;

Should probably set all here since that is the best default?

Quoted 1 line

tools/libxl/libxl_create.c +1 -0

+static void print_bitmap(uint8_t *map, int maplen, FILE *stream);

[..]

Quoted 1 line

+static int vcpupin_parse(char *cpu, libxl_cpumap *cpumap);

Again, maybe just reorder the functions?

Quoted 7 lines

static void parse_config_data(const char *configfile_filename_report,
const char *configfile_data, int configfile_len,

tools/libxl/libxl_create.c +0 -0

@@ -661,6 +674,13 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
         b_info->max_vcpus = l;
 

tools/libxl/libxl_create.c static void parse_config_data(const char · line 677

Quoted 1 line

tools/libxl/libxl_create.c +1 -0

+    if (!xlu_cfg_get_string (config, "cpus", &buf, 0)) {

The syntax supported here is different to that in a cpupool cfg? (see main_cpupoolcreate) Perhaps they should be similar?

tools/libxl/libxl_create.c static void parse_config_data(const char · line 681

Quoted 4 lines

tools/libxl/libxl_create.c +4 -0

+        char *buf2 = strdup(buf);
+        vcpupin_parse(buf2, &b_info->cpumap);
+    } else {
+        memset(b_info->cpumap.map, -1, b_info->cpumap.size);

This could do with a helper function in libxl_utils.h.

Quoted 12 lines · Ian Campbell

tools/libxl/libxl_create.c int libxl_init_build_info(libxl_ctx *ctx · line 82

Quoted 9 lines

diff -r 9ce68a4036b1 tools/libxl/libxl_create.c

tools/libxl/libxl_create.c +2 -0

@@ -78,6 +78,8 @@ int libxl_init_build_info(libxl_ctx *ctx
     memset(b_info, '\0', sizeof(*b_info));
     b_info->max_vcpus = 1;
     b_info->cur_vcpus = 1;
+    if (libxl_cpumap_alloc(ctx, &b_info->cpumap))
+        return ERROR_NOMEM;

Should probably set all here since that is the best default?

Having this set to "none" here simplifies a bit the code when we came to config file parsing (if I set it to "any CPU" here, I'll have to reset to "none" before parsing). Also, default is exactly what you're suggesting already, as I set the map to "any CPU" if no "cpus" config option is found.

Anyway, I guess I can do as you suggest if you really think it's better. :-)

Quoted 7 lines

Quoted 7 lines

tools/libxl/libxl_create.c +0 -0

 static void parse_config_data(const char *configfile_filename_report,
                               const char *configfile_data,
                               int configfile_len,

@@ -661,6 +674,13 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
         b_info->max_vcpus = l;
 

Quoted 5 lines

tools/libxl/libxl_create.c static void parse_config_data(const char · line 677

Quoted 1 line

tools/libxl/libxl_create.c +1 -0

+    if (!xlu_cfg_get_string (config, "cpus", &buf, 0)) {

The syntax supported here is different to that in a cpupool cfg? (see main_cpupoolcreate) Perhaps they should be similar?

It is different, and that was intentional. What I wanted, was the syntax to be exactly the same of `xl vcpu-pin', which is the command line equivalent of this option, much more than cpupool-*. If you want, I think I can easily enable list-like CPU specification as in cpupool config file so that _both_ syntax are allowed. What do you think?

Quoted 5 lines

Having this set to "none" here simplifies a bit the code when we came to config file parsing (if I set it to "any CPU" here, I'll have to reset to "none" before parsing). Also, default is exactly what you're suggesting already, as I set the map to "any CPU" if no "cpus" config option is found.

xl's default is "any CPU" but libxl's default is "no CPU" which will mean every toolstack author needs to do be aware of this even if they don't care about affinity.

Quoted 2 lines

Anyway, I guess I can do as you suggest if you really think it's better. :-)

Please ;-)

Quoted 5 lines

It is different, and that was intentional. What I wanted, was the syntax to be exactly the same of `xl vcpu-pin', which is the command line equivalent of this option, much more than cpupool-*. If you want, I think I can easily enable list-like CPU specification as in cpupool config file so that _both_ syntax are allowed. What do you think?

I think supporting both makes sense, we do something similar in a couple of places already.

tools/examples/xmexample.hvm +0 -2

@@ -59,10 +59,8 @@ name = "ExampleHVMDomain"
 # xen_extended_power_mgmt=0
 
 # List of which CPUS this domain is allowed to use, default Xen picks
-#cpus = ""         # leave to Xen to pick
 #cpus = "0"        # all vcpus run on CPU0
 #cpus = "0-3,5,^1" # all vcpus run on cpus 0,2,3,5
-#cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
 
 # Optionally define mac and/or bridge for the network interfaces.
 # Random MACs are assigned if not given.

diff -r 897ea4d3c2d7 tools/examples/xmexample.hvm-stubdom

tools/examples/xmexample.hvm-stubdom +0 -2

@@ -50,10 +50,8 @@ name = "xmexample.hvm"
 #apic=1
 
 # List of which CPUS this domain is allowed to use, default Xen picks
-#cpus = ""         # leave to Xen to pick
 #cpus = "0"        # all vcpus run on CPU0
 #cpus = "0-3,5,^1" # all vcpus run on cpus 0,2,3,5
-#cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
 
 # Optionally define mac and/or bridge for the network interfaces.
 # Random MACs are assigned if not given.

diff -r 897ea4d3c2d7 tools/examples/xmexample.pv-grub

tools/examples/xmexample.pv-grub +0 -2

@@ -35,10 +35,8 @@ name = "ExampleDomain"
 #uuid = "06ed00fe-1162-4fc4-b5d8-11993ee4a8b9"
 
 # List of which CPUS this domain is allowed to use, default Xen picks
-#cpus = ""         # leave to Xen to pick
 #cpus = "0"        # all vcpus run on CPU0
 #cpus = "0-3,5,^1" # all vcpus run on cpus 0,2,3,5
-#cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
 
 # Number of Virtual CPUS to use, default is 1
 #vcpus = 1

diff -r 897ea4d3c2d7 tools/examples/xmexample.vti

tools/examples/xmexample.vti +0 -2

@@ -31,10 +31,8 @@ name = "ExampleVTIDomain"
 #vcpus=1
 
 # List of which CPUS this domain is allowed to use, default Xen picks
-#cpus = ""         # leave to Xen to pick
 #cpus = "0"        # all vcpus run on CPU0
 #cpus = "0-3,5,^1" # all vcpus run on cpus 0,2,3,5
-#cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
 
 # Log2 of VHPT size, default=23 (8MB), minimum=15 (32KB).
 # In Windows OS, smaller size shows better performance.

diff -r 897ea4d3c2d7 tools/examples/xmexample1

tools/examples/xmexample1 +0 -2

@@ -31,10 +31,8 @@ name = "ExampleDomain"
 #uuid = "06ed00fe-1162-4fc4-b5d8-11993ee4a8b9"
 
 # List of which CPUS this domain is allowed to use, default Xen picks
-#cpus = ""         # leave to Xen to pick
 #cpus = "0"        # all vcpus run on CPU0
 #cpus = "0-3,5,^1" # all vcpus run on cpus 0,2,3,5
-#cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
 
 # Number of Virtual CPUS to use, default is 1
 #vcpus = 1

diff -r 897ea4d3c2d7 tools/examples/xmexample2

tools/examples/xmexample2 +0 -3

@@ -60,11 +60,8 @@ name = "VM%d" % vmid
 #uuid = "06ed00fe-1162-4fc4-b5d8-11993ee4a8b9"
 
 # List of which CPUS this domain is allowed to use, default Xen picks
-#cpus = ""         # leave to Xen to pick
 #cpus = "0"        # all vcpus run on CPU0
 #cpus = "0-3,5,^1" # all vcpus run on cpus 0,2,3,5
-#cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
-#cpus = "%s" % vmid # set based on vmid (mod number of CPUs)
 
 # Number of Virtual CPUS to use, default is 1
 #vcpus = 1

diff -r 897ea4d3c2d7 tools/examples/xmexample3

tools/examples/xmexample3 +0 -3

@@ -60,11 +60,8 @@ name = "VM%d" % vmid
 #uuid = "06ed00fe-1162-4fc4-b5d8-11993ee4a8b9"
 
 # List of which CPUS this domain is allowed to use, default Xen picks
-#cpus = ""         # leave to Xen to pick
 #cpus = "0"        # all vcpus run on CPU0
 #cpus = "0-3,5,^1" # all vcpus run on cpus 0,2,3,5
-#cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
-cpus = "%s" % vmid # set based on vmid (mod number of CPUs)
 
 #----------------------------------------------------------------------------
 # Define network interfaces.

tools/examples/xmexample.hvm

Quoted 3 lines

diff -r 897ea4d3c2d7 tools/examples/xmexample.hvm

tools/examples/xmexample.hvm +0 -0

These are the xend/xm examples which you didn't change. Or is it that the current examples don't actually reflect the current xend syntax either?