[libvirt] [PATCH 0/6] vcpu info refactors - part 3a
A short dump of patches that are separate enough which can be reviewed while I'm working on other things. Peter Krempa (6): virsh: cmdVcpuPin: Simplify handling of API flags util: Use virBitmapIsBitSet in freebsd impl of virProcessSetAffinity qemu: vcpupin: Don't overwrite errors from functions setting pinning qemu: vcpupin: Always set affinity even when cgroups are supported qemu: qemuDomainGetStatsVcpu: Fix output for possible sparse vCPU settings virsh: cpupin: Extract getter code into a separate function src/qemu/qemu_driver.c | 23 +++------ src/util/virprocess.c | 5 +- tools/virsh-domain.c | 134 +++++++++++++++++++++++++++---------------------- 3 files changed, 82 insertions(+), 80 deletions(-) -- 2.6.2
Rather than setting flags to -1 if none were specified, move the logic to use the old API to the place where we need to decide. It simplifies the logic a bit. --- tools/virsh-domain.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6dd75e2..028df1f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6432,9 +6432,6 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; - /* none of the options were specified */ - if (!current && !live && !config) - flags = -1; if (vshCommandOptStringReq(ctl, cmd, "cpulist", &cpulist) < 0) return false; @@ -6459,11 +6456,6 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) /* Query mode: show CPU affinity information then exit.*/ if (!cpulist) { - /* When query mode and neither "live", "config" nor "current" - * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags */ - if (flags == -1) - flags = VIR_DOMAIN_AFFECT_CURRENT; - if ((ncpus = virshCPUCountCollect(ctl, dom, flags, true)) < 0) { if (ncpus == -1) { if (flags & VIR_DOMAIN_AFFECT_LIVE) @@ -6511,7 +6503,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) if (!(cpumap = virshParseCPUList(ctl, &cpumaplen, cpulist, maxcpu))) goto cleanup; - if (flags == -1) { + /* use old API without any explicit flags */ + if (flags == 0 && !current) { if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) goto cleanup; } else { -- 2.6.2
On 02/17/2016 11:25 AM, Peter Krempa wrote:
Rather than setting flags to -1 if none were specified, move the logic to use the old API to the place where we need to decide. It simplifies the logic a bit. --- tools/virsh-domain.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6dd75e2..028df1f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6432,9 +6432,6 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; - /* none of the options were specified */ - if (!current && !live && !config) - flags = -1;
if (vshCommandOptStringReq(ctl, cmd, "cpulist", &cpulist) < 0) return false; @@ -6459,11 +6456,6 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
/* Query mode: show CPU affinity information then exit.*/ if (!cpulist) { - /* When query mode and neither "live", "config" nor "current" - * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags */ - if (flags == -1) - flags = VIR_DOMAIN_AFFECT_CURRENT; - if ((ncpus = virshCPUCountCollect(ctl, dom, flags, true)) < 0) { if (ncpus == -1) { if (flags & VIR_DOMAIN_AFFECT_LIVE) @@ -6511,7 +6503,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) if (!(cpumap = virshParseCPUList(ctl, &cpumaplen, cpulist, maxcpu))) goto cleanup;
- if (flags == -1) { + /* use old API without any explicit flags */ + if (flags == 0 && !current) {
s/0/VIR_DOMAIN_AFFECT_CURRENT Just to be painfully obvious... ACK - John
if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) goto cleanup; } else {
Use the helper that does not return errors to fix spuriously looking dead return of -1. --- src/util/virprocess.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index c7ffa42..bf6a6df 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -535,13 +535,10 @@ int virProcessSetAffinity(pid_t pid, { size_t i; cpuset_t mask; - bool set = false; CPU_ZERO(&mask); for (i = 0; i < virBitmapSize(map); i++) { - if (virBitmapGetBit(map, i, &set) < 0) - return -1; - if (set) + if (virBitmapIsBitSet(map, i)) CPU_SET(i, &mask); } -- 2.6.2
Both errors from the cgroups code and from the affinity code would be overwritten by the API. Report the more specific error. --- src/qemu/qemu_driver.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a762521..24c1ca4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5066,20 +5066,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu, false, &cgroup_vcpu) < 0) goto endjob; - if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("failed to set cpuset.cpus in cgroup" - " for vcpu %d"), vcpu); + if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) goto endjob; - } } else { if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), - pcpumap) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("failed to set cpu affinity for vcpu %d"), - vcpu); + pcpumap) < 0) goto endjob; - } } virBitmapFree(vcpuinfolive->cpumask); -- 2.6.2
VM startup and CPU hotplug always set the affinity regardless of cgroups support. Use the same approach for the pinning API. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 24c1ca4..d1e5188 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5068,12 +5068,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) goto endjob; - } else { - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), - pcpumap) < 0) - goto endjob; } + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), pcpumap) < 0) + goto endjob; + virBitmapFree(vcpuinfolive->cpumask); vcpuinfolive->cpumask = pcpumaplive; pcpumaplive = NULL; -- 2.6.2
On 02/17/2016 11:25 AM, Peter Krempa wrote:
VM startup and CPU hotplug always set the affinity regardless of cgroups support. Use the same approach for the pinning API. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 24c1ca4..d1e5188 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5068,12 +5068,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) goto endjob; - } else { - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), - pcpumap) < 0) - goto endjob; }
+ if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), pcpumap) < 0) + goto endjob; + virBitmapFree(vcpuinfolive->cpumask); vcpuinfolive->cpumask = pcpumaplive; pcpumaplive = NULL;
Why not the same for qemuDomainPinEmulator and qemuDomainPinIOThread? Since qemuProcessSetEmulatorAffinity and qemuProcessSetupIOThread do the same as qemuProcessSetupVcpu. John
On Fri, Feb 19, 2016 at 07:19:38 -0500, John Ferlan wrote:
On 02/17/2016 11:25 AM, Peter Krempa wrote:
VM startup and CPU hotplug always set the affinity regardless of cgroups support. Use the same approach for the pinning API. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 24c1ca4..d1e5188 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5068,12 +5068,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) goto endjob; - } else { - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), - pcpumap) < 0) - goto endjob; }
+ if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), pcpumap) < 0) + goto endjob; + virBitmapFree(vcpuinfolive->cpumask); vcpuinfolive->cpumask = pcpumaplive; pcpumaplive = NULL;
Why not the same for qemuDomainPinEmulator and qemuDomainPinIOThread?
I'm too focused on the cpus in this series apparently ...
Since qemuProcessSetEmulatorAffinity and qemuProcessSetupIOThread do the same as qemuProcessSetupVcpu.
Are you okay with separate patches for this?
On 02/19/2016 08:09 AM, Peter Krempa wrote:
On Fri, Feb 19, 2016 at 07:19:38 -0500, John Ferlan wrote:
On 02/17/2016 11:25 AM, Peter Krempa wrote:
VM startup and CPU hotplug always set the affinity regardless of cgroups support. Use the same approach for the pinning API. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 24c1ca4..d1e5188 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5068,12 +5068,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) goto endjob; - } else { - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), - pcpumap) < 0) - goto endjob; }
+ if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), pcpumap) < 0) + goto endjob; + virBitmapFree(vcpuinfolive->cpumask); vcpuinfolive->cpumask = pcpumaplive; pcpumaplive = NULL;
Why not the same for qemuDomainPinEmulator and qemuDomainPinIOThread?
I'm too focused on the cpus in this series apparently ...
Since qemuProcessSetEmulatorAffinity and qemuProcessSetupIOThread do the same as qemuProcessSetupVcpu.
Are you okay with separate patches for this?
Yes that's fine John
qemuDomainHelperGetVcpus would correctly return an array of virVcpuInfoPtr structs for online vcpus even for sparse topologies, but the loop that fills the returned typed parameters would number the vcpus incorrectly. Fortunately sparse topologies aren't supported yet. --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d1e5188..b1f94bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18965,7 +18965,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, for (i = 0; i < virDomainDefGetVcpus(dom->def); i++) { snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, - "vcpu.%zu.state", i); + "vcpu.%u.state", cpuinfo[i].number); if (virTypedParamsAddInt(&record->params, &record->nparams, maxparams, @@ -18978,7 +18978,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, continue; snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, - "vcpu.%zu.time", i); + "vcpu.%u.time", cpuinfo[i].number); if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams, @@ -18986,7 +18986,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, cpuinfo[i].cpuTime) < 0) goto cleanup; snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, - "vcpu.%zu.wait", i); + "vcpu.%u.wait", cpuinfo[i].number); if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams, -- 2.6.2
--- tools/virsh-domain.c | 129 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 75 insertions(+), 54 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 028df1f..1546826 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6374,6 +6374,68 @@ virshPrintPinInfo(vshControl *ctl, return true; } + +static bool +virshVcpuPinQuery(vshControl *ctl, + virDomainPtr dom, + unsigned int vcpu, + bool got_vcpu, + int maxcpu, + unsigned int flags) +{ + unsigned char *cpumap = NULL; + int cpumaplen; + size_t i; + int ncpus; + bool ret = false; + + if ((ncpus = virshCPUCountCollect(ctl, dom, flags, true)) < 0) { + if (ncpus == -1) { + if (flags & VIR_DOMAIN_AFFECT_LIVE) + vshError(ctl, "%s", _("cannot get vcpupin for offline domain")); + else + vshError(ctl, "%s", _("cannot get vcpupin for transient domain")); + } + return false; + } + + if (got_vcpu && vcpu >= ncpus) { + if (flags & VIR_DOMAIN_AFFECT_LIVE || + (!(flags & VIR_DOMAIN_AFFECT_CONFIG) && + virDomainIsActive(dom) == 1)) + vshError(ctl, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, ncpus); + else + vshError(ctl, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, ncpus); + return false; + } + + cpumaplen = VIR_CPU_MAPLEN(maxcpu); + cpumap = vshMalloc(ctl, ncpus * cpumaplen); + if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap, + cpumaplen, flags)) >= 0) { + vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU Affinity")); + vshPrintExtra(ctl, "----------------------------------\n"); + for (i = 0; i < ncpus; i++) { + if (got_vcpu && i != vcpu) + continue; + + vshPrint(ctl, "%4zu: ", i); + ret = virshPrintPinInfo(ctl, VIR_GET_CPUMAP(cpumap, cpumaplen, i), + cpumaplen); + vshPrint(ctl, "\n"); + if (!ret) + break; + } + } + + return ret; +} + + static unsigned char * virshParseCPUList(vshControl *ctl, int *cpumaplen, const char *cpulist, int maxcpu) @@ -6416,8 +6478,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) bool ret = false; unsigned char *cpumap = NULL; int cpumaplen; - int maxcpu, ncpus; - size_t i; + int maxcpu; bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); @@ -6456,63 +6517,23 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) /* Query mode: show CPU affinity information then exit.*/ if (!cpulist) { - if ((ncpus = virshCPUCountCollect(ctl, dom, flags, true)) < 0) { - if (ncpus == -1) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) - vshError(ctl, "%s", _("cannot get vcpupin for offline domain")); - else - vshError(ctl, "%s", _("cannot get vcpupin for transient domain")); - } - goto cleanup; - } - - if (got_vcpu && vcpu >= ncpus) { - if (flags & VIR_DOMAIN_AFFECT_LIVE || - (!(flags & VIR_DOMAIN_AFFECT_CONFIG) && - virDomainIsActive(dom) == 1)) - vshError(ctl, - _("vcpu %d is out of range of live cpu count %d"), - vcpu, ncpus); - else - vshError(ctl, - _("vcpu %d is out of range of persistent cpu count %d"), - vcpu, ncpus); - goto cleanup; - } + ret = virshVcpuPinQuery(ctl, dom, vcpu, got_vcpu, maxcpu, flags); + goto cleanup; + } - cpumaplen = VIR_CPU_MAPLEN(maxcpu); - cpumap = vshMalloc(ctl, ncpus * cpumaplen); - if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap, - cpumaplen, flags)) >= 0) { - vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU Affinity")); - vshPrintExtra(ctl, "----------------------------------\n"); - for (i = 0; i < ncpus; i++) { - if (got_vcpu && i != vcpu) - continue; + /* Pin mode: pinning specified vcpu to specified physical cpus*/ + if (!(cpumap = virshParseCPUList(ctl, &cpumaplen, cpulist, maxcpu))) + goto cleanup; - vshPrint(ctl, "%4zu: ", i); - ret = virshPrintPinInfo(ctl, VIR_GET_CPUMAP(cpumap, cpumaplen, i), - cpumaplen); - vshPrint(ctl, "\n"); - if (!ret) - break; - } - } + /* use old API without any explicit flags */ + if (flags == 0 && !current) { + if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) + goto cleanup; } else { - /* Pin mode: pinning specified vcpu to specified physical cpus*/ - if (!(cpumap = virshParseCPUList(ctl, &cpumaplen, cpulist, maxcpu))) + if (virDomainPinVcpuFlags(dom, vcpu, cpumap, cpumaplen, flags) != 0) goto cleanup; - - /* use old API without any explicit flags */ - if (flags == 0 && !current) { - if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) - goto cleanup; - } else { - if (virDomainPinVcpuFlags(dom, vcpu, cpumap, cpumaplen, flags) != 0) - goto cleanup; - } - ret = true; } + ret = true; cleanup: VIR_FREE(cpumap); -- 2.6.2
On 02/17/2016 11:25 AM, Peter Krempa wrote:
A short dump of patches that are separate enough which can be reviewed while I'm working on other things.
Peter Krempa (6): virsh: cmdVcpuPin: Simplify handling of API flags util: Use virBitmapIsBitSet in freebsd impl of virProcessSetAffinity qemu: vcpupin: Don't overwrite errors from functions setting pinning qemu: vcpupin: Always set affinity even when cgroups are supported qemu: qemuDomainGetStatsVcpu: Fix output for possible sparse vCPU settings virsh: cpupin: Extract getter code into a separate function
src/qemu/qemu_driver.c | 23 +++------ src/util/virprocess.c | 5 +- tools/virsh-domain.c | 134 +++++++++++++++++++++++++++---------------------- 3 files changed, 82 insertions(+), 80 deletions(-)
ACK series, but check comments specifically made for patch 1 and 4 John
participants (2)
-
John Ferlan -
Peter Krempa