At 06/20/2011 02:46 PM, Daniel Veillard Write:

On Fri, Jun 10, 2011 at 03:38:55PM +0900, Taku Izumi wrote:

...

When using vcpupin command, we have to speficy comma-separated list as cpulist, but this is tedious in case the number of phsycal cpus is large. This patch improves this by introducing special markup "-" and "^" which are similar to XML schema of "cpuset" attribute.

That is:

The example

...

# virsh vcpupin Guest 0 0-15,^8

is identical to

# virsh vcpupin Guest 0 0,1,2,3,4,5,6,7,9,10,11,12,13,14,15

NOTE: The expression is sequencially evaluated, so "0-15,^8" is not identical

s/sequencially/sequentially/

...

to "^8,0-15".

and this limitation should also be added to the virsh command doc I think

...

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 125 +++++++++++++++++++++++++++++++------------------------- tools/virsh.pod | 4 + 2 files changed, 73 insertions(+), 56 deletions(-)

Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -2946,8 +2946,9 @@ cmdVcpupin(vshControl *ctl, const vshCmd bool ret = true; unsigned char *cpumap; int cpumaplen; - int i; - enum { expect_num, expect_num_or_comma } state; + int i, cpu, lastcpu, maxcpu; + bool unuse = false; + char *cur; int config = vshCommandOptBool(cmd, "config"); int live = vshCommandOptBool(cmd, "live"); int current = vshCommandOptBool(cmd, "current"); @@ -3003,66 +3004,74 @@ cmdVcpupin(vshControl *ctl, const vshCmd return false; }

- /* Check that the cpulist parameter is a comma-separated list of - * numbers and give an intelligent error message if not. - */ - if (cpulist[0] == '\0') { - vshError(ctl, "%s", _("cpulist: Invalid format. Empty string.")); - virDomainFree (dom); - return false; - } + maxcpu = VIR_NODEINFO_MAXCPUS(nodeinfo); + cpumaplen = VIR_CPU_MAPLEN(maxcpu); + cpumap = vshCalloc(ctl, 0, cpumaplen); + + /* Parse cpulist */ + cur = cpulist; + if (*cur == 0) + goto parse_error; + + while (*cur != 0) { + + /* the char '^' denotes exclusive */ + if (*cur == '^') { + cur++; + unuse = true; + } + + /* parse physical CPU number */ + if (!c_isdigit(*cur)) + goto parse_error; + cpu = virParseNumber(&cur); + if (cpu < 0) { + goto parse_error; + } + if (cpu >= maxcpu) { + vshError(ctl, _("Physical CPU %d doesn't exist."), cpu); + goto parse_error; + } + virSkipSpaces(&cur);

- state = expect_num; - for (i = 0; cpulist[i]; i++) { - switch (state) { - case expect_num: - if (!c_isdigit (cpulist[i])) { - vshError(ctl, _("cpulist: %s: Invalid format. Expecting " - "digit at position %d (near '%c')."), - cpulist, i, cpulist[i]); - virDomainFree (dom); - return false; + if ((*cur == ',') || (*cur == 0)) { + if (unuse) { + VIR_UNUSE_CPU(cpumap, cpu); + } else { + VIR_USE_CPU(cpumap, cpu); } - state = expect_num_or_comma; - break; - case expect_num_or_comma: - if (cpulist[i] == ',') - state = expect_num; - else if (!c_isdigit (cpulist[i])) { - vshError(ctl, _("cpulist: %s: Invalid format. Expecting " - "digit or comma at position %d (near '%c')."), - cpulist, i, cpulist[i]); - virDomainFree (dom); - return false; + } else if (*cur == '-') { + /* the char '-' denotes range */ + if (unuse) { + goto parse_error; + } + cur++; + virSkipSpaces(&cur); + /* parse the end of range */ + lastcpu = virParseNumber(&cur); + if (lastcpu < cpu) { + goto parse_error; + } + if (lastcpu >= maxcpu) { + vshError(ctl, _("Physical CPU %d doesn't exist."), maxcpu); + goto parse_error; + } + for (i = cpu; i <= lastcpu; i++) { + VIR_USE_CPU(cpumap, i); } + virSkipSpaces(&cur); } - } - if (state == expect_num) { - vshError(ctl, _("cpulist: %s: Invalid format. Trailing comma " - "at position %d."), - cpulist, i); - virDomainFree (dom); - return false; - }

- cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo)); - cpumap = vshCalloc(ctl, 1, cpumaplen); - - do { - unsigned int cpu = atoi(cpulist); - - if (cpu < VIR_NODEINFO_MAXCPUS(nodeinfo)) { - VIR_USE_CPU(cpumap, cpu); + if (*cur == ',') { + cur++; + virSkipSpaces(&cur); + unuse = false; + } else if (*cur == 0) { + break; } else { - vshError(ctl, _("Physical CPU %d doesn't exist."), cpu); - VIR_FREE(cpumap); - virDomainFree(dom); - return false; + goto parse_error; } - cpulist = strchr(cpulist, ','); - if (cpulist) - cpulist++; - } while (cpulist); + }

if (flags == -1) { if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) { @@ -3074,9 +3083,15 @@ cmdVcpupin(vshControl *ctl, const vshCmd } }

+cleanup: VIR_FREE(cpumap); virDomainFree(dom); return ret; + +parse_error: + vshError(ctl, "%s", _("cpulist: Invalid format.")); + ret = false; + goto cleanup; }

/* Index: libvirt/tools/virsh.pod =================================================================== --- libvirt.orig/tools/virsh.pod +++ libvirt/tools/virsh.pod @@ -794,7 +794,9 @@ vCPUs, the running time, the affinity to I<--current>

Pin domain VCPUs to host physical CPUs. The I<vcpu> number must be provided -and I<cpulist> is a comma separated list of physical CPU numbers. +and I<cpulist> is a list of physical CPU numbers. Its syntax is a comma +separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can +also be allowed. The '-' denotes the range and the '^' denotes exclusive. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state.

Okay, ACK, the point about having to generate the long comma list is right and this seems like the right solution.

I have pushed it with this(add extra documentation in the virsh.pod):

From bab581dce13850509b38beef0de329ef35e4c6d1 Mon Sep 17 00:00:00 2001 From: Taku Izumi <izumi.taku@jp.fujitsu.com> Date: Fri, 10 Jun 2011 15:38:55 +0900 Subject: [PATCH] vcpupin: improve vcpupin definition of virsh vcpupin

When using vcpupin command, we have to speficy comma-separated list as cpulist, but this is tedious in case the number of phsycal cpus is large. This patch improves this by introducing special markup "-" and "^" which are similar to XML schema of "cpuset" attribute. The example: # virsh vcpupin Guest 0 0-15,^8 is identical to # virsh vcpupin Guest 0 0,1,2,3,4,5,6,7,9,10,11,12,13,14,15 NOTE: The expression is sequencially evaluated, so "0-15,^8" is not identical to "^8,0-15". Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 125 +++++++++++++++++++++++++++++++------------------------ tools/virsh.pod | 7 +++- 2 files changed, 76 insertions(+), 56 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b7b9552..92faffc 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2998,8 +2998,9 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd) bool ret = true; unsigned char *cpumap; int cpumaplen; - int i; - enum { expect_num, expect_num_or_comma } state; + int i, cpu, lastcpu, maxcpu; + bool unuse = false; + char *cur; int config = vshCommandOptBool(cmd, "config"); int live = vshCommandOptBool(cmd, "live"); int current = vshCommandOptBool(cmd, "current"); @@ -3055,66 +3056,74 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd) return false; } - /* Check that the cpulist parameter is a comma-separated list of - * numbers and give an intelligent error message if not. - */ - if (cpulist[0] == '\0') { - vshError(ctl, "%s", _("cpulist: Invalid format. Empty string.")); - virDomainFree (dom); - return false; - } + maxcpu = VIR_NODEINFO_MAXCPUS(nodeinfo); + cpumaplen = VIR_CPU_MAPLEN(maxcpu); + cpumap = vshCalloc(ctl, 0, cpumaplen); - state = expect_num; - for (i = 0; cpulist[i]; i++) { - switch (state) { - case expect_num: - if (!c_isdigit (cpulist[i])) { - vshError(ctl, _("cpulist: %s: Invalid format. Expecting " - "digit at position %d (near '%c')."), - cpulist, i, cpulist[i]); - virDomainFree (dom); - return false; - } - state = expect_num_or_comma; - break; - case expect_num_or_comma: - if (cpulist[i] == ',') - state = expect_num; - else if (!c_isdigit (cpulist[i])) { - vshError(ctl, _("cpulist: %s: Invalid format. Expecting " - "digit or comma at position %d (near '%c')."), - cpulist, i, cpulist[i]); - virDomainFree (dom); - return false; - } + /* Parse cpulist */ + cur = cpulist; + if (*cur == 0) + goto parse_error; + + while (*cur != 0) { + + /* the char '^' denotes exclusive */ + if (*cur == '^') { + cur++; + unuse = true; } - } - if (state == expect_num) { - vshError(ctl, _("cpulist: %s: Invalid format. Trailing comma " - "at position %d."), - cpulist, i); - virDomainFree (dom); - return false; - } - cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo)); - cpumap = vshCalloc(ctl, 1, cpumaplen); + /* parse physical CPU number */ + if (!c_isdigit(*cur)) + goto parse_error; + cpu = virParseNumber(&cur); + if (cpu < 0) { + goto parse_error; + } + if (cpu >= maxcpu) { + vshError(ctl, _("Physical CPU %d doesn't exist."), cpu); + goto parse_error; + } + virSkipSpaces(&cur); - do { - unsigned int cpu = atoi(cpulist); + if ((*cur == ',') || (*cur == 0)) { + if (unuse) { + VIR_UNUSE_CPU(cpumap, cpu); + } else { + VIR_USE_CPU(cpumap, cpu); + } + } else if (*cur == '-') { + /* the char '-' denotes range */ + if (unuse) { + goto parse_error; + } + cur++; + virSkipSpaces(&cur); + /* parse the end of range */ + lastcpu = virParseNumber(&cur); + if (lastcpu < cpu) { + goto parse_error; + } + if (lastcpu >= maxcpu) { + vshError(ctl, _("Physical CPU %d doesn't exist."), maxcpu); + goto parse_error; + } + for (i = cpu; i <= lastcpu; i++) { + VIR_USE_CPU(cpumap, i); + } + virSkipSpaces(&cur); + } - if (cpu < VIR_NODEINFO_MAXCPUS(nodeinfo)) { - VIR_USE_CPU(cpumap, cpu); + if (*cur == ',') { + cur++; + virSkipSpaces(&cur); + unuse = false; + } else if (*cur == 0) { + break; } else { - vshError(ctl, _("Physical CPU %d doesn't exist."), cpu); - VIR_FREE(cpumap); - virDomainFree(dom); - return false; + goto parse_error; } - cpulist = strchr(cpulist, ','); - if (cpulist) - cpulist++; - } while (cpulist); + } if (flags == -1) { if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) { @@ -3126,9 +3135,15 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd) } } +cleanup: VIR_FREE(cpumap); virDomainFree(dom); return ret; + +parse_error: + vshError(ctl, "%s", _("cpulist: Invalid format.")); + ret = false; + goto cleanup; } /* diff --git a/tools/virsh.pod b/tools/virsh.pod index fa37442..b90f37e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -833,13 +833,18 @@ vCPUs, the running time, the affinity to physical processors. I<--current> Pin domain VCPUs to host physical CPUs. The I<vcpu> number must be provided -and I<cpulist> is a comma separated list of physical CPU numbers. +and I<cpulist> is a list of physical CPU numbers. Its syntax is a comma +separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can +also be allowed. The '-' denotes the range and the '^' denotes exclusive. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state. Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. If no flag is specified, behavior is different depending on hypervisor. +B<Note>: The expression is sequentially evaluated, so "0-15,^8" is not identical +to "^8,0-15". + =item B<vncdisplay> I<domain-id> Output the IP address and port number for the VNC display. If the information -- 1.7.1

Daniel

On Fri, Jun 10, 2011 at 03:39:38PM +0900, Taku Izumi wrote:

When resetting vcpupin setting, we have to specify all host physical cpus as a cpulist parameter of virsh vcpupin command. It's a little tedious.

This patch changes to allow to receive the special keyword 'r' as a cpulist parameter of virsh vcpupin command when resetting vcpupin setting.

If you set the following:

# virsh vcpupin VM 0 r

the vcpu0 will be pinned to all physical cpus.

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 7 ++++++- tools/virsh.pod | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-)

Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -3010,8 +3010,13 @@ cmdVcpupin(vshControl *ctl, const vshCmd

/* Parse cpulist */ cur = cpulist; - if (*cur == 0) + if (*cur == 0) { goto parse_error; + } else if (*cur == 'r') { + for (cpu = 0; cpu < maxcpu; cpu++) + VIR_USE_CPU(cpumap, cpu); + *cur = 0; + }

while (*cur != 0) {

Index: libvirt/tools/virsh.pod =================================================================== --- libvirt.orig/tools/virsh.pod +++ libvirt/tools/virsh.pod @@ -797,6 +797,8 @@ Pin domain VCPUs to host physical CPUs. and I<cpulist> is a list of physical CPU numbers. Its syntax is a comma separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can also be allowed. The '-' denotes the range and the '^' denotes exclusive. +If you want to reset vcpupin setting, that is, to pin vcpu all physical cpus, +simply specify 'r' as a cpulist. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state.

ACK, a simple usability improvement Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Fri, Jun 10, 2011 at 03:40:36PM +0900, Taku Izumi wrote:

This patch add the private API (virDomainVcpupinDel). This API can delete the vcpupin setting of a specified virtual cpu.

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 42 insertions(+)

Index: libvirt/src/conf/domain_conf.c =================================================================== --- libvirt.orig/src/conf/domain_conf.c +++ libvirt/src/conf/domain_conf.c @@ -8126,6 +8126,45 @@ cleanup: return -1; }

Not useful as is, but used in the next patch, ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Fri, Jun 10, 2011 at 03:41:18PM +0900, Taku Izumi wrote:

Pinning to all physical cpus means resetting, hence it is preferable to delete vcpupin setting of XML.

This patch changes qemu driver to delete vcpupin setting by invoking virDomainVcpupinDel API when pinning the specified virtual cpu to all host physical cpus.

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 64 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 18 deletions(-)

Index: libvirt/src/qemu/qemu_driver.c =================================================================== --- libvirt.orig/src/qemu/qemu_driver.c +++ libvirt/src/qemu/qemu_driver.c @@ -2894,6 +2894,7 @@ qemudDomainPinVcpuFlags(virDomainPtr dom int ret = -1; bool isActive; qemuDomainObjPrivatePtr priv; + bool canResetting = true;

virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -2944,15 +2945,23 @@ qemudDomainPinVcpuFlags(virDomainPtr dom goto cleanup; }

- if (flags & VIR_DOMAIN_AFFECT_LIVE) { - - if (nodeGetInfo(dom->conn, &nodeinfo) < 0) - goto cleanup; + if (nodeGetInfo(dom->conn, &nodeinfo) < 0) + goto cleanup; + hostcpus = VIR_NODEINFO_MAXCPUS(nodeinfo); + maxcpu = maplen * 8; + if (maxcpu > hostcpus) + maxcpu = hostcpus; + /* pinning to all physical cpus means resetting, + * so check if we can reset setting. + */ + for (int pcpu = 0; pcpu < hostcpus; pcpu++) { + if ((cpumap[pcpu/8] & (1 << (pcpu % 8))) == 0) { + canResetting = false; + break; + } + }

- hostcpus = VIR_NODEINFO_MAXCPUS(nodeinfo); - maxcpu = maplen * 8; - if (maxcpu > hostcpus) - maxcpu = hostcpus; + if (flags & VIR_DOMAIN_AFFECT_LIVE) {

if (priv->vcpupids != NULL) { if (virProcessInfoSetAffinity(priv->vcpupids[vcpu], @@ -2964,23 +2973,42 @@ qemudDomainPinVcpuFlags(virDomainPtr dom goto cleanup; }

- if (virDomainVcpupinAdd(vm->def, cpumap, maplen, vcpu) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add vcpupin xml of " - "a running domain")); - goto cleanup; + if (canResetting) { + if (virDomainVcpupinDel(vm->def, vcpu) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to delete vcpupin xml of " + "a running domain")); + goto cleanup; + } + } else { + if (virDomainVcpupinAdd(vm->def, cpumap, maplen, vcpu) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add vcpupin xml of " + "a running domain")); + goto cleanup; + } }

}

if (flags & VIR_DOMAIN_AFFECT_CONFIG) {

- if (virDomainVcpupinAdd(persistentDef, cpumap, maplen, vcpu) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add vcpupin xml of " - "a persistent domain")); - goto cleanup; + if (canResetting) { + if (virDomainVcpupinDel(persistentDef, vcpu) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to delete vcpupin xml of " + "a persistent domain")); + goto cleanup; + } + } else { + if (virDomainVcpupinAdd(persistentDef, cpumap, maplen, vcpu) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add vcpupin xml of " + "a persistent domain")); + goto cleanup; + } } + ret = virDomainSaveConfig(driver->configDir, persistentDef); goto cleanup; }

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/