Next patch will add minimum checking, so use a more generic name. Refactor return values to the commonly used semantics. --- src/qemu/qemu_process.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f083f3f..be0567a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3861,22 +3861,19 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, } -static bool -qemuValidateCpuMax(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) +static int +qemuValidateCpuCount(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) { - unsigned int maxCpus; - - maxCpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, def->os.machine); - if (!maxCpus) - return true; + unsigned int maxCpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, def->os.machine); - if (virDomainDefGetVcpusMax(def) > maxCpus) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("Maximum CPUs greater than specified machine type limit")); - return false; + if (maxCpus > 0 && virDomainDefGetVcpusMax(def) > maxCpus) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Maximum CPUs greater than specified machine type limit")); + return -1; } - return true; + return 0; } @@ -4667,7 +4664,7 @@ qemuProcessLaunch(virConnectPtr conn, } } - if (!qemuValidateCpuMax(vm->def, priv->qemuCaps)) + if (qemuValidateCpuCount(vm->def, priv->qemuCaps) < 0) goto cleanup; if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) -- 2.6.2

Counterintuitively the user would end up with a VM with maximum number of vCPUs available. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1290324 --- src/qemu/qemu_process.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index be0567a..845d5e1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3867,6 +3867,12 @@ qemuValidateCpuCount(virDomainDefPtr def, { unsigned int maxCpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, def->os.machine); + if (virDomainDefGetVcpus(def) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Domain requires at least 1 vCPU")); + return -1; + } + if (maxCpus > 0 && virDomainDefGetVcpusMax(def) > maxCpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Maximum CPUs greater than specified machine type limit")); -- 2.6.2

The array doesn't necessarily have the same cardinality as the count of vCPUs for a domain. Iterating it can cause access beyond the end of the array. --- src/vz/vz_sdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index b78c413..d610979 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1958,7 +1958,7 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) } if (def->cputune.vcpupin) { - for (i = 0; i < virDomainDefGetVcpus(def); i++) { + for (i = 0; i < def->cputune.nvcpupin; i++) { if (!virBitmapEqual(def->cpumask, def->cputune.vcpupin[i]->cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.6.2

This should be the last offender. --- src/qemu/qemu_process.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 845d5e1..dec4572 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2350,10 +2350,14 @@ qemuProcessSetSchedParams(int id, static int qemuProcessSetSchedulers(virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = vm->privateData; size_t i = 0; - for (i = 0; i < priv->nvcpupids; i++) { + for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, i); + + if (!vcpu->online) + continue; + if (qemuProcessSetSchedParams(i, qemuDomainGetVcpuPid(vm, i), vm->def->cputune.nvcpusched, vm->def->cputune.vcpusched) < 0) -- 2.6.2

On 01/15/2016 01:50 PM, John Ferlan wrote: So after thinking about this one some more and of course seeing most of the rest of the code - I revisited this one... I'm perhaps thinking ahead w/r/t how adding "a" vcpu could/would work as opposed to the current model of using setvcpus to change the current count as long as it doesn't go over max. There isn't a "chance" that the def->vcpus could have vcpuids 0, 1, and 4 'online' while vcpuid 3 is 'offline'. IOW: the def->vcpus array is would have the active/online vcpus first followed by the offline second. This would thus match the cardinality of the priv->vcpupids, so I think we're OK here. Although it would still be nice to have an OnlineVcpuMap available. In any case, ACK for the change, but it's definitely something that'll need to be thought about eventually. At least w/r/t the number of places where qemuDomainGetVcpuPid and virDomainDefGetVcpu assume the same order. John

On 01/14/2016 11:26 AM, Peter Krempa wrote: Existing, but sure would be nice to have a description of what this does in the comments... Especially w/r/t input/output expectations. I agree the whole param_idx usage/setting is a bit odd. Realistically it's [0] for CPU and [1] for the vCPU... Thus if 'nparams == 1', then fill in CPU data and if 'nparams == 2', then fill in vCPU data... John

In some cases it may be better to have a bitmap representing state of individual vcpus rather than iterating the definition. The new helper creates a bitmap representing the state from the domain definition. --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 26 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9706b0..36286d3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1374,6 +1374,30 @@ virDomainDefGetVcpus(const virDomainDef *def) } +/** + * virDomainDefGetVcpumap: + * @def: domain definition + * + * Returns a bitmap representing state of individual vcpus. + */ +virBitmapPtr +virDomainDefGetVcpumap(const virDomainDef *def) +{ + virBitmapPtr ret = NULL; + size_t i; + + if (!(ret = virBitmapNew(def->maxvcpus))) + return NULL; + + for (i = 0; i < def->maxvcpus; i++) { + if (def->vcpus[i].online) + ignore_value(virBitmapSetBit(ret, i)); + } + + return ret; +} + + virDomainVcpuInfoPtr virDomainDefGetVcpu(virDomainDefPtr def, unsigned int vcpu) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0141009..f15b558 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2358,6 +2358,7 @@ bool virDomainDefHasVcpusOffline(const virDomainDef *def); unsigned int virDomainDefGetVcpusMax(const virDomainDef *def); int virDomainDefSetVcpus(virDomainDefPtr def, unsigned int vcpus); unsigned int virDomainDefGetVcpus(const virDomainDef *def); +virBitmapPtr virDomainDefGetVcpumap(const virDomainDef *def); virDomainVcpuInfoPtr virDomainDefGetVcpu(virDomainDefPtr def, unsigned int vcpu) ATTRIBUTE_RETURN_CHECK; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 83f6e2c..e8efdc6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -217,6 +217,7 @@ virDomainDefGetMemoryActual; virDomainDefGetMemoryInitial; virDomainDefGetSecurityLabelDef; virDomainDefGetVcpu; +virDomainDefGetVcpumap; virDomainDefGetVcpus; virDomainDefGetVcpusMax; virDomainDefHasDeviceAddress; -- 2.6.2

Simplify the code by extracting a common code path. --- tools/virsh-domain.c | 55 +++++++++++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 62acecb..ad720cf 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7237,12 +7237,31 @@ static const vshCmdOptDef opts_cpu_stats[] = { {.name = NULL} }; +static void +vshCPUStatsPrintField(vshControl *ctl, + virTypedParameterPtr param) +{ + vshPrint(ctl, "\t%-12s ", param->field); + if ((STREQ(param->field, VIR_DOMAIN_CPU_STATS_CPUTIME) || + STREQ(param->field, VIR_DOMAIN_CPU_STATS_USERTIME) || + STREQ(param->field, VIR_DOMAIN_CPU_STATS_SYSTEMTIME)) && + param->type == VIR_TYPED_PARAM_ULLONG) { + vshPrint(ctl, "%9lld.%09lld seconds\n", + param->value.ul / 1000000000, + param->value.ul % 1000000000); + } else { + char *s = vshGetTypedParamValue(ctl, param); + vshPrint(ctl, "%s\n", s); + VIR_FREE(s); + } +} + static bool cmdCPUStats(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; virTypedParameterPtr params = NULL; - int pos, max_id, cpu = 0, show_count = -1, nparams = 0, stats_per_cpu; + int max_id, cpu = 0, show_count = -1, nparams = 0, stats_per_cpu; size_t i, j; bool show_total = false, show_per_cpu = false; unsigned int flags = 0; @@ -7317,21 +7336,8 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) continue; vshPrint(ctl, "CPU%zu:\n", cpu + i); - for (j = 0; j < nparams; j++) { - pos = i * nparams + j; - vshPrint(ctl, "\t%-12s ", params[pos].field); - if ((STREQ(params[pos].field, VIR_DOMAIN_CPU_STATS_CPUTIME) || - STREQ(params[pos].field, VIR_DOMAIN_CPU_STATS_VCPUTIME)) && - params[j].type == VIR_TYPED_PARAM_ULLONG) { - vshPrint(ctl, "%9lld.%09lld seconds\n", - params[pos].value.ul / 1000000000, - params[pos].value.ul % 1000000000); - } else { - char *s = vshGetTypedParamValue(ctl, &params[pos]); - vshPrint(ctl, _("%s\n"), s); - VIR_FREE(s); - } - } + for (j = 0; j < nparams; j++) + vshCPUStatsPrintField(ctl, params + (i * nparams + j)); } cpu += ncpus; show_count -= ncpus; @@ -7363,21 +7369,8 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) goto failed_stats; vshPrint(ctl, _("Total:\n")); - for (i = 0; i < stats_per_cpu; i++) { - vshPrint(ctl, "\t%-12s ", params[i].field); - if ((STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME) || - STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_USERTIME) || - STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_SYSTEMTIME)) && - params[i].type == VIR_TYPED_PARAM_ULLONG) { - vshPrint(ctl, "%9lld.%09lld seconds\n", - params[i].value.ul / 1000000000, - params[i].value.ul % 1000000000); - } else { - char *s = vshGetTypedParamValue(ctl, &params[i]); - vshPrint(ctl, "%s\n", s); - VIR_FREE(s); - } - } + for (i = 0; i < stats_per_cpu; i++) + vshCPUStatsPrintField(ctl, params + i); ret = true; -- 2.6.2

virDomainGetCPUStats doesn't support flags so there's no need to carry the 'flags' variable around. Additionally since the API is poorly designed I doubt that it will be extended. --- tools/virsh-domain.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ad720cf..4f34f2a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7264,7 +7264,6 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) int max_id, cpu = 0, show_count = -1, nparams = 0, stats_per_cpu; size_t i, j; bool show_total = false, show_per_cpu = false; - unsigned int flags = 0; bool ret = false; int rv = 0; @@ -7303,7 +7302,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) goto do_show_total; /* get number of cpus on the node */ - if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, flags)) < 0) + if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0)) < 0) goto failed_stats; if (show_count < 0 || show_count > max_id) { if (show_count > max_id) @@ -7312,7 +7311,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) } /* get percpu information */ - if ((nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, flags)) < 0) + if ((nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0)) < 0) goto failed_stats; if (!nparams) { @@ -7328,7 +7327,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) while (show_count) { int ncpus = MIN(show_count, 128); - if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, flags) < 0) + if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0) < 0) goto failed_stats; for (i = 0; i < ncpus; i++) { @@ -7352,7 +7351,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) do_show_total: /* get supported num of parameter for total statistics */ - if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, flags)) < 0) + if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0) goto failed_stats; if (!nparams) { @@ -7365,7 +7364,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) /* passing start_cpu == -1 gives us domain's total status */ if ((stats_per_cpu = virDomainGetCPUStats(dom, params, nparams, - -1, 1, flags)) < 0) + -1, 1, 0)) < 0) goto failed_stats; vshPrint(ctl, _("Total:\n")); -- 2.6.2

Iterate over all cpus skipping inactive ones. --- src/qemu/qemu_driver.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2300c7e..3102fc2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10005,7 +10005,12 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, goto cleanup; virCgroupFree(&cgroup_temp); - for (i = 0; i < priv->nvcpupids; i++) { + for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, i); + + if (!vcpu->online) + continue; + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) @@ -10238,7 +10243,6 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, unsigned long long period, long long quota) { size_t i; - qemuDomainObjPrivatePtr priv = vm->privateData; virCgroupPtr cgroup_vcpu = NULL; if (period == 0 && quota == 0) @@ -10247,7 +10251,12 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, if (!qemuDomainHasVcpuPids(vm)) return 0; - for (i = 0; i < priv->nvcpupids; i++) { + for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, i); + + if (!vcpu->online) + continue; + if (virCgroupNewThread(cgroup, VIR_CGROUP_THREAD_VCPU, i, false, &cgroup_vcpu) < 0) goto cleanup; -- 2.6.2

Pass a bitmap of enabled guest vCPUs to virCgroupGetPercpuStats so that un-continuous vCPU topologies can be used. --- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 7 ++++++- src/util/vircgroup.c | 16 ++++++++-------- src/util/vircgroup.h | 3 ++- tests/vircgrouptest.c | 2 +- 5 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 67088c8..f6fe207 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -5702,7 +5702,7 @@ lxcDomainGetCPUStats(virDomainPtr dom, params, nparams); else ret = virCgroupGetPercpuStats(priv->cgroup, params, - nparams, start_cpu, ncpus, 0); + nparams, start_cpu, ncpus, NULL); cleanup: virObjectUnlock(vm); return ret; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3102fc2..f3844d6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18092,6 +18092,7 @@ qemuDomainGetCPUStats(virDomainPtr domain, virDomainObjPtr vm = NULL; int ret = -1; qemuDomainObjPrivatePtr priv; + virBitmapPtr guestvcpus = NULL; virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); @@ -18115,13 +18116,17 @@ qemuDomainGetCPUStats(virDomainPtr domain, goto cleanup; } + if (!(guestvcpus = virDomainDefGetVcpumap(vm->def))) + goto cleanup; + if (start_cpu == -1) ret = virCgroupGetDomainTotalCpuStats(priv->cgroup, params, nparams); else ret = virCgroupGetPercpuStats(priv->cgroup, params, nparams, - start_cpu, ncpus, priv->nvcpupids); + start_cpu, ncpus, guestvcpus); cleanup: + virBitmapFree(guestvcpus); virDomainObjEndAPI(&vm); return ret; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 2fc0276..e6dbc8d 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3121,17 +3121,17 @@ virCgroupDenyDevicePath(virCgroupPtr group, const char *path, int perms) */ static int virCgroupGetPercpuVcpuSum(virCgroupPtr group, - unsigned int nvcpupids, + virBitmapPtr guestvcpus, unsigned long long *sum_cpu_time, size_t nsum, virBitmapPtr cpumap) { int ret = -1; - size_t i; + ssize_t i = -1; char *buf = NULL; virCgroupPtr group_vcpu = NULL; - for (i = 0; i < nvcpupids; i++) { + while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) { char *pos; unsigned long long tmp; ssize_t j; @@ -3173,7 +3173,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, unsigned int nparams, int start_cpu, unsigned int ncpus, - unsigned int nvcpupids) + virBitmapPtr guestvcpus) { int ret = -1; size_t i; @@ -3188,7 +3188,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, /* return the number of supported params */ if (nparams == 0 && ncpus != 0) { - if (nvcpupids == 0) + if (!guestvcpus) return CGROUP_NB_PER_CPU_STAT_PARAM; else return CGROUP_NB_PER_CPU_STAT_PARAM + 1; @@ -3243,11 +3243,11 @@ virCgroupGetPercpuStats(virCgroupPtr group, /* return percpu vcputime in index 1 */ param_idx = 1; - if (nvcpupids > 0 && param_idx < nparams) { + if (guestvcpus && param_idx < nparams) { if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0) goto cleanup; - if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, need_cpus, - cpumap) < 0) + if (virCgroupGetPercpuVcpuSum(group, guestvcpus, sum_cpu_time, + need_cpus, cpumap) < 0) goto cleanup; for (i = start_cpu; i < need_cpus; i++) { diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 63a9e1c..856b993 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -26,6 +26,7 @@ # define __VIR_CGROUP_H__ # include "virutil.h" +# include "virbitmap.h" struct virCgroup; typedef struct virCgroup *virCgroupPtr; @@ -246,7 +247,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, unsigned int nparams, int start_cpu, unsigned int ncpus, - unsigned int nvcpupids); + virBitmapPtr guestvcpus); int virCgroupGetDomainTotalCpuStats(virCgroupPtr group, diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 7ea6e13..322f6cb 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -656,7 +656,7 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED) if ((rv = virCgroupGetPercpuStats(cgroup, params, - 1, 0, EXPECTED_NCPUS, 0)) < 0) { + 1, 0, EXPECTED_NCPUS, NULL)) < 0) { fprintf(stderr, "Failed call to virCgroupGetPercpuStats for /virtualmachines cgroup: %d\n", -rv); goto cleanup; } -- 2.6.2

Avoid using virDomainDefSetVcpus when we can set it directly in the structure. --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f3844d6..3aa49f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4680,6 +4680,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, unsigned int vcpu) { qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainVcpuInfoPtr vcpuinfo; int ret = -1; int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); @@ -4689,6 +4690,15 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, char *mem_mask = NULL; virDomainNumatuneMemMode mem_mode; + if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu))) + return -1; + + if (vcpuinfo->online) { + virReportError(VIR_ERR_INVALID_ARG, + _("vCPU '%u' is already online"), vcpu); + return -1; + } + qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorSetCPU(priv->mon, vcpu, true); @@ -4704,7 +4714,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, if (rc < 0) goto cleanup; - ignore_value(virDomainDefSetVcpus(vm->def, oldvcpus + 1)); + vcpuinfo->online = true; if (ncpupids < 0) goto cleanup; @@ -4780,12 +4790,22 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, unsigned int vcpu) { qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainVcpuInfoPtr vcpuinfo; int ret = -1; int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); pid_t *cpupids = NULL; int ncpupids = 0; + if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu))) + return -1; + + if (!vcpuinfo->online) { + virReportError(VIR_ERR_INVALID_ARG, + _("vCPU '%u' is already offline"), vcpu); + return -1; + } + qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorSetCPU(priv->mon, vcpu, false); @@ -4809,7 +4829,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, goto cleanup; } - ignore_value(virDomainDefSetVcpus(vm->def, oldvcpus - 1)); + vcpuinfo->online = false; if (qemuDomainDelCgroupForThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu) < 0) -- 2.6.2

Future patches will tweak and reuse the function in different places so move it separately first. --- src/qemu/qemu_domain.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_process.c | 76 ++------------------------------------------ 3 files changed, 88 insertions(+), 74 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0fa2dbe..1ea1cd3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4257,3 +4257,87 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm, return priv->vcpupids[vcpu]; } + + +/** + * qemuDomainDetectVcpuPIDs: + * @driver: qemu driver data + * @vm: domain object + * @asyncJob: current asynchronous job type + * + * Updates vCPU trhead ids in the private data of @vm. + * + * Returns 0 on success -1 on error and reports an appropriate error. + */ +int +qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob) +{ + pid_t *cpupids = NULL; + int ncpupids; + qemuDomainObjPrivatePtr priv = vm->privateData; + + /* + * Current QEMU *can* report info about host threads mapped + * to vCPUs, but it is not in a manner we can correctly + * deal with. The TCG CPU emulation does have a separate vCPU + * thread, but it runs every vCPU in that same thread. So it + * is impossible to setup different affinity per thread. + * + * What's more the 'query-cpus' command returns bizarre + * data for the threads. It gives the TCG thread for the + * vCPU 0, but for vCPUs 1-> N, it actually replies with + * the main process thread ID. + * + * The result is that when we try to set affinity for + * vCPU 1, it will actually change the affinity of the + * emulator thread :-( When you try to set affinity for + * vCPUs 2, 3.... it will fail if the affinity was + * different from vCPU 1. + * + * We *could* allow vcpu pinning with TCG, if we made the + * restriction that all vCPUs had the same mask. This would + * at least let us separate emulator from vCPUs threads, as + * we do for KVM. It would need some changes to our cgroups + * CPU layout though, and error reporting for the config + * restrictions. + * + * Just disable CPU pinning with TCG until someone wants + * to try to do this hard work. + */ + if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) { + priv->nvcpupids = 0; + priv->vcpupids = NULL; + return 0; + } + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + /* failure to get the VCPU<-> PID mapping or to execute the query + * command will not be treated fatal as some versions of qemu don't + * support this command */ + if (ncpupids <= 0) { + virResetLastError(); + + priv->nvcpupids = 0; + priv->vcpupids = NULL; + return 0; + } + + if (ncpupids != virDomainDefGetVcpus(vm->def)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of vCPU pids from QEMU monitor. " + "got %d, wanted %d"), + ncpupids, virDomainDefGetVcpus(vm->def)); + VIR_FREE(cpupids); + return -1; + } + + priv->nvcpupids = ncpupids; + priv->vcpupids = cpupids; + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7fc4fff..6a8cf70 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -508,5 +508,7 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, bool qemuDomainHasVcpuPids(virDomainObjPtr vm); pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpu); +int qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, virDomainObjPtr vm, + int asyncJob); #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index dec4572..a7c3094 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1962,78 +1962,6 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver, return ret; } -static int -qemuProcessDetectVcpuPIDs(virQEMUDriverPtr driver, - virDomainObjPtr vm, int asyncJob) -{ - pid_t *cpupids = NULL; - int ncpupids; - qemuDomainObjPrivatePtr priv = vm->privateData; - - /* - * Current QEMU *can* report info about host threads mapped - * to vCPUs, but it is not in a manner we can correctly - * deal with. The TCG CPU emulation does have a separate vCPU - * thread, but it runs every vCPU in that same thread. So it - * is impossible to setup different affinity per thread. - * - * What's more the 'query-cpus' command returns bizarre - * data for the threads. It gives the TCG thread for the - * vCPU 0, but for vCPUs 1-> N, it actually replies with - * the main process thread ID. - * - * The result is that when we try to set affinity for - * vCPU 1, it will actually change the affinity of the - * emulator thread :-( When you try to set affinity for - * vCPUs 2, 3.... it will fail if the affinity was - * different from vCPU 1. - * - * We *could* allow vcpu pinning with TCG, if we made the - * restriction that all vCPUs had the same mask. This would - * at least let us separate emulator from vCPUs threads, as - * we do for KVM. It would need some changes to our cgroups - * CPU layout though, and error reporting for the config - * restrictions. - * - * Just disable CPU pinning with TCG until someone wants - * to try to do this hard work. - */ - if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) { - priv->nvcpupids = 0; - priv->vcpupids = NULL; - return 0; - } - - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - return -1; - ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - return -1; - /* failure to get the VCPU<-> PID mapping or to execute the query - * command will not be treated fatal as some versions of qemu don't - * support this command */ - if (ncpupids <= 0) { - virResetLastError(); - - priv->nvcpupids = 0; - priv->vcpupids = NULL; - return 0; - } - - if (ncpupids != virDomainDefGetVcpus(vm->def)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("got wrong number of vCPU pids from QEMU monitor. " - "got %d, wanted %d"), - ncpupids, virDomainDefGetVcpus(vm->def)); - VIR_FREE(cpupids); - return -1; - } - - priv->nvcpupids = ncpupids; - priv->vcpupids = cpupids; - return 0; -} - static int qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, @@ -4982,7 +4910,7 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; VIR_DEBUG("Detecting VCPU PIDs"); - if (qemuProcessDetectVcpuPIDs(driver, vm, asyncJob) < 0) + if (qemuDomainDetectVcpuPids(driver, vm, asyncJob) < 0) goto cleanup; VIR_DEBUG("Detecting IOThread PIDs"); @@ -5689,7 +5617,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, } VIR_DEBUG("Detecting VCPU PIDs"); - if (qemuProcessDetectVcpuPIDs(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + if (qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto error; VIR_DEBUG("Detecting IOThread PIDs"); -- 2.6.2

Free the old vcpupids array in case when this function is called again during the run of the VM. It will be later reused in the vCPU hotplug code. --- src/qemu/qemu_domain.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1ea1cd3..a6fb232 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4275,7 +4275,7 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, int asyncJob) { pid_t *cpupids = NULL; - int ncpupids; + int ncpupids = 0; qemuDomainObjPrivatePtr priv = vm->privateData; /* @@ -4306,11 +4306,8 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, * Just disable CPU pinning with TCG until someone wants * to try to do this hard work. */ - if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) { - priv->nvcpupids = 0; - priv->vcpupids = NULL; - return 0; - } + if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) + goto done; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; @@ -4322,10 +4319,8 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, * support this command */ if (ncpupids <= 0) { virResetLastError(); - - priv->nvcpupids = 0; - priv->vcpupids = NULL; - return 0; + ncpupids = 0; + goto done; } if (ncpupids != virDomainDefGetVcpus(vm->def)) { @@ -4337,6 +4332,8 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, return -1; } + done: + VIR_FREE(priv->vcpupids); priv->nvcpupids = ncpupids; priv->vcpupids = cpupids; return 0; -- 2.6.2

Now that qemuDomainDetectVcpuPids is able to refresh the vCPU pid information it can be reused in the hotplug and hotunplug code paths rather than open-coding a very similar algorithm. A slight algoirithm change is necessary for unplug since the vCPU needs to be marked offline prior to calling the thread detector function and eventually rolled back if something fails. --- src/qemu/qemu_driver.c | 72 +++++++++++++------------------------------------- 1 file changed, 18 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3aa49f2..b377738 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4684,11 +4684,10 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, int ret = -1; int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); - pid_t *cpupids = NULL; - int ncpupids = 0; virCgroupPtr cgroup_vcpu = NULL; char *mem_mask = NULL; virDomainNumatuneMemMode mem_mode; + pid_t vcpupid; if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu))) return -1; @@ -4703,9 +4702,6 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, rc = qemuMonitorSetCPU(priv->mon, vcpu, true); - if (rc == 0) - ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids); - if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -4716,23 +4712,10 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, vcpuinfo->online = true; - if (ncpupids < 0) - goto cleanup; - - /* failure to re-detect vCPU pids after hotplug due to lack of support was - * historically deemed not fatal. We need to skip the rest of the steps though. */ - if (ncpupids == 0) { - ret = 0; + if (qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; - } - if (ncpupids != oldvcpus + 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("got wrong number of vCPU pids from QEMU monitor. " - "got %d, wanted %d"), - ncpupids, oldvcpus + 1); - goto cleanup; - } + vcpupid = qemuDomainGetVcpuPid(vm, vcpu); if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && @@ -4742,11 +4725,9 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, goto cleanup; if (priv->cgroup) { - cgroup_vcpu = - qemuDomainAddCgroupForThread(priv->cgroup, - VIR_CGROUP_THREAD_VCPU, - vcpu, mem_mask, - cpupids[vcpu]); + cgroup_vcpu = qemuDomainAddCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_VCPU, + vcpu, mem_mask, vcpupid); if (!cgroup_vcpu) goto cleanup; } @@ -4758,26 +4739,20 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, &vm->def->cputune.nvcpupin) < 0) goto cleanup; - if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, cpupids[vcpu], + if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, vcpupid, cgroup_vcpu) < 0) { goto cleanup; } } - if (qemuProcessSetSchedParams(vcpu, cpupids[vcpu], + if (qemuProcessSetSchedParams(vcpu, vcpupid, vm->def->cputune.nvcpusched, vm->def->cputune.vcpusched) < 0) goto cleanup; - priv->nvcpupids = ncpupids; - VIR_FREE(priv->vcpupids); - priv->vcpupids = cpupids; - cpupids = NULL; - ret = 0; cleanup: - VIR_FREE(cpupids); VIR_FREE(mem_mask); virCgroupFree(&cgroup_vcpu); return ret; @@ -4794,8 +4769,6 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, int ret = -1; int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); - pid_t *cpupids = NULL; - int ncpupids = 0; if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu))) return -1; @@ -4806,30 +4779,23 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, return -1; } + vcpuinfo->online = false; + qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorSetCPU(priv->mon, vcpu, false); - if (rc == 0) - ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids); - if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", - rc == 0 && ncpupids == oldvcpus -1); + if (rc < 0) + rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE); - if (rc < 0 || ncpupids < 0) - goto cleanup; + virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", rc == 0); - /* check if hotunplug has failed */ - if (ncpupids != oldvcpus - 1) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("qemu didn't unplug vCPU '%u' properly"), vcpu); + if (rc < 0) goto cleanup; - } - vcpuinfo->online = false; if (qemuDomainDelCgroupForThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu) < 0) @@ -4840,15 +4806,13 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, &vm->def->cputune.nvcpupin, vcpu); - priv->nvcpupids = ncpupids; - VIR_FREE(priv->vcpupids); - priv->vcpupids = cpupids; - cpupids = NULL; - ret = 0; cleanup: - VIR_FREE(cpupids); + /* rollback the cpu state */ + if (ret < 0) + vcpuinfo->online = true; + return ret; } -- 2.6.2

This step can be omitted, so that drivers can decide what to do when the user requests to use default vcpu pinning. --- src/conf/domain_conf.c | 32 -------------------------------- src/libxl/libxl_domain.c | 15 ++++++++++++--- src/libxl/libxl_driver.c | 2 ++ src/qemu/qemu_driver.c | 34 ++-------------------------------- src/qemu/qemu_process.c | 8 +++++++- src/test/test_driver.c | 2 ++ src/vz/vz_sdk.c | 4 ++-- 7 files changed, 27 insertions(+), 70 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 36286d3..caf5334 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15149,34 +15149,6 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); - /* Initialize the pinning policy for vcpus which doesn't has - * the policy specified explicitly as def->cpuset. - */ - if (def->cpumask) { - if (VIR_REALLOC_N(def->cputune.vcpupin, virDomainDefGetVcpus(def)) < 0) - goto error; - - for (i = 0; i < virDomainDefGetVcpus(def); i++) { - if (virDomainPinIsDuplicate(def->cputune.vcpupin, - def->cputune.nvcpupin, - i)) - continue; - - virDomainPinDefPtr vcpupin = NULL; - - if (VIR_ALLOC(vcpupin) < 0) - goto error; - - if (!(vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) { - VIR_FREE(vcpupin); - goto error; - } - virBitmapCopy(vcpupin->cpumask, def->cpumask); - vcpupin->id = i; - def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; - } - } - if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot extract emulatorpin nodes")); @@ -21855,10 +21827,6 @@ virDomainDefFormatInternal(virDomainDefPtr def, for (i = 0; i < def->cputune.nvcpupin; i++) { char *cpumask; - /* Ignore the vcpupin which inherit from "cpuset of "<vcpu>." */ - if (virBitmapEqual(def->cpumask, def->cputune.vcpupin[i]->cpumask)) - continue; - virBufferAsprintf(&childrenBuf, "<vcpupin vcpu='%u' ", def->cputune.vcpupin[i]->id); diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index cf5c9f6..37c92c6 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -824,9 +824,18 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) libxl_bitmap_init(&map); - for (i = 0; i < vm->def->cputune.nvcpupin; ++i) { - pin = vm->def->cputune.vcpupin[i]; - cpumask = pin->cpumask; + for (i = 0; i < virDomainDefGetVcpus(vm->def); ++i) { + pin = virDomainPinFind(vm->def->cputune.vcpupin, + vm->def->cputune.nvcpupin, + i); + + if (pin && pin->cpumask) + cpumask = pin->cpumask; + else + cpumask = vm->def->cpumask; + + if (!cpumask) + continue; if (virBitmapToData(cpumask, &map.map, (int *)&map.size) < 0) goto cleanup; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d4e9c2a7..26c1a43 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2464,6 +2464,8 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, if (pininfo && pininfo->cpumask) bitmap = pininfo->cpumask; + else if (targetDef->cpumask) + bitmap = targetDef->cpumask; else bitmap = allcpumap; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b377738..0a4de1b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4596,33 +4596,6 @@ qemuDomainAddCgroupForThread(virCgroupPtr cgroup, return NULL; } -static int -qemuDomainHotplugAddPin(virBitmapPtr cpumask, - int idx, - virDomainPinDefPtr **pindef_list, - size_t *npin) -{ - int ret = -1; - virDomainPinDefPtr pindef = NULL; - - if (VIR_ALLOC(pindef) < 0) - goto cleanup; - - if (!(pindef->cpumask = virBitmapNewCopy(cpumask))) { - VIR_FREE(pindef); - goto cleanup; - } - pindef->id = idx; - if (VIR_APPEND_ELEMENT_COPY(*pindef_list, *npin, pindef) < 0) { - virBitmapFree(pindef->cpumask); - VIR_FREE(pindef); - goto cleanup; - } - ret = 0; - - cleanup: - return ret; -} static int qemuDomainHotplugPinThread(virBitmapPtr cpumask, @@ -4734,11 +4707,6 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, /* Inherit def->cpuset */ if (vm->def->cpumask) { - if (qemuDomainHotplugAddPin(vm->def->cpumask, vcpu, - &vm->def->cputune.vcpupin, - &vm->def->cputune.nvcpupin) < 0) - goto cleanup; - if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, vcpupid, cgroup_vcpu) < 0) { goto cleanup; @@ -5260,6 +5228,8 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && priv->autoCpuset) bitmap = priv->autoCpuset; + else if (def->cpumask) + bitmap = def->cpumask; else bitmap = allcpumap; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a7c3094..64b58be 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2177,7 +2177,8 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm) * VM default affinity, we must reject it */ for (n = 0; n < def->cputune.nvcpupin; n++) { - if (!virBitmapEqual(def->cpumask, + if (def->cputune.vcpupin[n]->cpumask && + !virBitmapEqual(def->cpumask, def->cputune.vcpupin[n]->cpumask)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cpu affinity is not supported")); @@ -2188,12 +2189,17 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm) } for (n = 0; n < virDomainDefGetVcpus(def); n++) { + virBitmapPtr bitmap; /* set affinity only for existing vcpus */ if (!(pininfo = virDomainPinFind(def->cputune.vcpupin, def->cputune.nvcpupin, n))) continue; + if (!(bitmap = pininfo->cpumask) && + !(bitmap = def->cpumask)) + continue; + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, n), pininfo->cpumask) < 0) { goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 8d1402e..4d268de 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2574,6 +2574,8 @@ testDomainGetVcpuPinInfo(virDomainPtr dom, if (pininfo && pininfo->cpumask) bitmap = pininfo->cpumask; + else if (def->cpumask) + bitmap = def->cpumask; else bitmap = allcpumap; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index d610979..7cc24d3 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1959,8 +1959,8 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) if (def->cputune.vcpupin) { for (i = 0; i < def->cputune.nvcpupin; i++) { - if (!virBitmapEqual(def->cpumask, - def->cputune.vcpupin[i]->cpumask)) { + if (def->cputune.vcpupin[i]->cpumask && + !virBitmapEqual(def->cpumask, def->cputune.vcpupin[i]->cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("vcpupin cpumask differs from default cpumask")); return -1; -- 2.6.2

A future patch will refactor the storage of the pinning information in a way where the ordering will be lost. Order them numerically to avoid changing the tests later. --- tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreads.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreads.xml b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreads.xml index c8cc025..00d8bf0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreads.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreads.xml @@ -10,8 +10,8 @@ <iothreadpin iothread='2' cpuset='3'/> <iothreadpin iothread='1' cpuset='2'/> <emulatorpin cpuset='1'/> - <vcpupin vcpu='1' cpuset='1'/> <vcpupin vcpu='0' cpuset='0'/> + <vcpupin vcpu='1' cpuset='1'/> <quota>-1</quota> <period>1000000</period> <shares>2048</shares> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml index dc65564..435d0ae 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml @@ -9,8 +9,8 @@ <shares>2048</shares> <period>1000000</period> <quota>-1</quota> - <vcpupin vcpu='1' cpuset='1'/> <vcpupin vcpu='0' cpuset='0'/> + <vcpupin vcpu='1' cpuset='1'/> <emulatorpin cpuset='1'/> <iothreadpin iothread='1' cpuset='2'/> <iothreadpin iothread='2' cpuset='3'/> -- 2.6.2

Add a helper function to do the checking. The check is used when determining whether the <cputune> element should be formatted. --- src/conf/domain_conf.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index caf5334..29ef357 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1413,6 +1413,19 @@ virDomainDefGetVcpu(virDomainDefPtr def, } +/** + * virDomainDefHasVcpusPin: + * @def: domain definition + * + * This helper returns true if any of the domain's vcpus has cpu pinning set + */ +static bool +virDomainDefHasVcpusPin(const virDomainDef *def) +{ + return !!def->cputune.nvcpupin; +} + + virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) { @@ -15303,7 +15316,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (virDomainNumatuneHasPlacementAuto(def->numa) && - !def->cpumask && !def->cputune.vcpupin && + !def->cpumask && !virDomainDefHasVcpusPin(def) && !def->cputune.emulatorpin && !virDomainIOThreadIDArrayHasPin(def)) def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO; -- 2.6.2

Report error: invalid argument: requested vcpu '100' is not present in the domain instead of error: invalid argument: requested vcpu is higher than allocated vcpus --- src/test/test_driver.c | 5 +++-- tests/vcpupin | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4d268de..5986749 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2508,8 +2508,9 @@ static int testDomainPinVcpu(virDomainPtr domain, } if (vcpu > virDomainDefGetVcpus(privdom->def)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("requested vcpu is higher than allocated vcpus")); + virReportError(VIR_ERR_INVALID_ARG, + _("requested vcpu '%d' is not present in the domain"), + vcpu); goto cleanup; } diff --git a/tests/vcpupin b/tests/vcpupin index 213db93..9e656c0 100755 --- a/tests/vcpupin +++ b/tests/vcpupin @@ -43,7 +43,7 @@ compare exp out || fail=1 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: invalid argument: requested vcpu is higher than allocated vcpus +error: invalid argument: requested vcpu '100' is not present in the domain EOF compare exp out || fail=1 -- 2.6.2

Now with the new struct the data can be stored in a much saner place. --- src/conf/domain_conf.c | 131 ++++++++++++++++++-------------------------- src/conf/domain_conf.h | 3 +- src/libxl/libxl_domain.c | 17 +++--- src/libxl/libxl_driver.c | 39 ++++++-------- src/qemu/qemu_cgroup.c | 15 ++---- src/qemu/qemu_driver.c | 138 ++++++++++++++++++++++------------------------- src/qemu/qemu_process.c | 38 +++++++------ src/test/test_driver.c | 43 ++++++--------- 8 files changed, 179 insertions(+), 245 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 29ef357..ca32466 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1289,6 +1289,8 @@ virDomainVcpuInfoClear(virDomainVcpuInfoPtr info) { if (!info) return; + + virBitmapFree(info->cpumask); } @@ -1422,7 +1424,14 @@ virDomainDefGetVcpu(virDomainDefPtr def, static bool virDomainDefHasVcpusPin(const virDomainDef *def) { - return !!def->cputune.nvcpupin; + size_t i; + + for (i = 0; i < def->maxvcpus; i++) { + if (def->vcpus[i].cpumask) + return true; + } + + return false; } @@ -2593,8 +2602,6 @@ void virDomainDefFree(virDomainDefPtr def) virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids); - virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin); - virBitmapFree(def->cputune.emulatorpin); for (i = 0; i < def->cputune.nvcpusched; i++) @@ -14137,77 +14144,62 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node, } -/* Check if pin with same id already exists. */ -static bool -virDomainPinIsDuplicate(virDomainPinDefPtr *def, - int npin, - int id) -{ - size_t i; - - if (!def || !npin) - return false; - - for (i = 0; i < npin; i++) { - if (def[i]->id == id) - return true; - } - - return false; -} - /* Parse the XML definition for a vcpupin * * vcpupin has the form of * <vcpupin vcpu='0' cpuset='0'/> */ -static virDomainPinDefPtr -virDomainVcpuPinDefParseXML(xmlNodePtr node, - xmlXPathContextPtr ctxt) +static int +virDomainVcpuPinDefParseXML(virDomainDefPtr def, + xmlNodePtr node) { - virDomainPinDefPtr def; - xmlNodePtr oldnode = ctxt->node; + virDomainVcpuInfoPtr vcpu; unsigned int vcpuid; char *tmp = NULL; + int ret = -1; - if (VIR_ALLOC(def) < 0) - return NULL; - - ctxt->node = node; - - if (!(tmp = virXPathString("string(./@vcpu)", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing vcpu id in vcpupin")); - goto error; + if (!(tmp = virXMLPropString(node, "vcpu"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing vcpu id in vcpupin")); + goto cleanup; } if (virStrToLong_uip(tmp, NULL, 10, &vcpuid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for vcpu '%s'"), tmp); - goto error; + goto cleanup; } VIR_FREE(tmp); - def->id = vcpuid; + if (!(vcpu = virDomainDefGetVcpu(def, vcpuid)) || + !vcpu->online) { + /* To avoid the regression when daemon loading domain confs, we can't + * simply error out if <vcpupin> nodes greater than current vcpus. + * Ignore them instead. */ + VIR_WARN("Ignoring vcpupin for missing vcpus"); + ret = 0; + goto cleanup; + } if (!(tmp = virXMLPropString(node, "cpuset"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing cpuset for vcpupin")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpuset for vcpupin")); + goto cleanup; + } - goto error; + if (vcpu->cpumask) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate vcpupin for vcpu '%d'"), vcpuid); + goto cleanup; } - if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + if (virBitmapParse(tmp, 0, &vcpu->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + + ret = 0; cleanup: VIR_FREE(tmp); - ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; } @@ -15131,34 +15123,9 @@ virDomainDefParseXML(xmlDocPtr xml, if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) goto error; - if (n && VIR_ALLOC_N(def->cputune.vcpupin, n) < 0) - goto error; - for (i = 0; i < n; i++) { - virDomainPinDefPtr vcpupin; - if (!(vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt))) + if (virDomainVcpuPinDefParseXML(def, nodes[i])) goto error; - - if (virDomainPinIsDuplicate(def->cputune.vcpupin, - def->cputune.nvcpupin, - vcpupin->id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("duplicate vcpupin for same vcpu")); - virDomainPinDefFree(vcpupin); - goto error; - } - - if (vcpupin->id >= virDomainDefGetVcpus(def)) { - /* To avoid the regression when daemon loading - * domain confs, we can't simply error out if - * <vcpupin> nodes greater than current vcpus, - * ignoring them instead. - */ - VIR_WARN("Ignore vcpupin for missing vcpus"); - virDomainPinDefFree(vcpupin); - } else { - def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; - } } VIR_FREE(nodes); @@ -21838,15 +21805,19 @@ virDomainDefFormatInternal(virDomainDefPtr def, "</emulator_quota>\n", def->cputune.emulator_quota); - for (i = 0; i < def->cputune.nvcpupin; i++) { + for (i = 0; i < def->maxvcpus; i++) { char *cpumask; - virBufferAsprintf(&childrenBuf, "<vcpupin vcpu='%u' ", - def->cputune.vcpupin[i]->id); + virDomainVcpuInfoPtr vcpu = def->vcpus + i; - if (!(cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask))) + if (!vcpu->cpumask) + continue; + + if (!(cpumask = virBitmapFormat(vcpu->cpumask))) goto error; - virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask); + virBufferAsprintf(&childrenBuf, + "<vcpupin vcpu='%zu' cpuset='%s'/>\n", i, cpumask); + VIR_FREE(cpumask); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f15b558..4a8c199 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2137,8 +2137,6 @@ struct _virDomainCputune { long long quota; unsigned long long emulator_period; long long emulator_quota; - size_t nvcpupin; - virDomainPinDefPtr *vcpupin; virBitmapPtr emulatorpin; size_t nvcpusched; @@ -2153,6 +2151,7 @@ typedef virDomainVcpuInfo *virDomainVcpuInfoPtr; struct _virDomainVcpuInfo { bool online; + virBitmapPtr cpumask; }; typedef struct _virDomainBlkiotune virDomainBlkiotune; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 37c92c6..99ce44a 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -816,7 +816,7 @@ int libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); - virDomainPinDefPtr pin; + virDomainVcpuInfoPtr vcpu; libxl_bitmap map; virBitmapPtr cpumask = NULL; size_t i; @@ -825,13 +825,12 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) libxl_bitmap_init(&map); for (i = 0; i < virDomainDefGetVcpus(vm->def); ++i) { - pin = virDomainPinFind(vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, - i); + vcpu = virDomainDefGetVcpu(vm->def, i); - if (pin && pin->cpumask) - cpumask = pin->cpumask; - else + if (!vcpu->online) + continue; + + if (!(cpumask = vcpu->cpumask)) cpumask = vm->def->cpumask; if (!cpumask) @@ -840,9 +839,9 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) if (virBitmapToData(cpumask, &map.map, (int *)&map.size) < 0) goto cleanup; - if (libxl_set_vcpuaffinity(cfg->ctx, vm->def->id, pin->id, &map) != 0) { + if (libxl_set_vcpuaffinity(cfg->ctx, vm->def->id, i, &map) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to pin vcpu '%d' with libxenlight"), pin->id); + _("Failed to pin vcpu '%zu' with libxenlight"), i); goto cleanup; } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 26c1a43..b9da190 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2321,6 +2321,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainDefPtr targetDef = NULL; virBitmapPtr pcpumap = NULL; + virDomainVcpuInfoPtr vcpuinfo; virDomainObjPtr vm; int ret = -1; @@ -2352,10 +2353,16 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, /* Make sure coverity knows targetDef is valid at this point. */ sa_assert(targetDef); - pcpumap = virBitmapNewData(cpumap, maplen); - if (!pcpumap) + if (!(pcpumap = virBitmapNewData(cpumap, maplen))) goto endjob; + if (!(vcpuinfo = virDomainDefGetVcpu(targetDef, vcpu)) || + !vcpuinfo->online) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu '%u' is not active"), vcpu); + goto endjob; + } + if (flags & VIR_DOMAIN_AFFECT_LIVE) { libxl_bitmap map = { .size = maplen, .map = cpumap }; if (libxl_set_vcpuaffinity(cfg->ctx, vm->def->id, vcpu, &map) != 0) { @@ -2366,20 +2373,9 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, } } - if (!targetDef->cputune.vcpupin) { - if (VIR_ALLOC(targetDef->cputune.vcpupin) < 0) - goto endjob; - targetDef->cputune.nvcpupin = 0; - } - if (virDomainPinAdd(&targetDef->cputune.vcpupin, - &targetDef->cputune.nvcpupin, - cpumap, - maplen, - vcpu) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to update or add vcpupin xml")); - goto endjob; - } + virBitmapFree(vcpuinfo->cpumask); + vcpuinfo->cpumask = pcpumap; + pcpumap = NULL; ret = 0; @@ -2455,15 +2451,14 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, memset(cpumaps, 0x00, maplen * ncpumaps); for (vcpu = 0; vcpu < ncpumaps; vcpu++) { - virDomainPinDefPtr pininfo; + virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(targetDef, vcpu); virBitmapPtr bitmap = NULL; - pininfo = virDomainPinFind(targetDef->cputune.vcpupin, - targetDef->cputune.nvcpupin, - vcpu); + if (!vcpuinfo->online) + continue; - if (pininfo && pininfo->cpumask) - bitmap = pininfo->cpumask; + if (vcpuinfo->cpumask) + bitmap = vcpuinfo->cpumask; else if (targetDef->cpumask) bitmap = targetDef->cpumask; else diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1c406ce..3744b52 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1007,7 +1007,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupPtr cgroup_vcpu = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1065,20 +1065,13 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0) goto cleanup; - /* try to use the default cpu maps */ - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + if (vcpu->cpumask) + cpumap = vcpu->cpumask; + else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumap = priv->autoCpuset; else cpumap = vm->def->cpumask; - /* lookup a more specific pinning info */ - for (j = 0; j < def->cputune.nvcpupin; j++) { - if (def->cputune.vcpupin[j]->id == i) { - cpumap = def->cputune.vcpupin[j]->cpumask; - break; - } - } - if (!cpumap) continue; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a4de1b..f253248 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4769,10 +4769,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, VIR_CGROUP_THREAD_VCPU, vcpu) < 0) goto cleanup; - /* Free vcpupin setting */ - virDomainPinDel(&vm->def->cputune.vcpupin, - &vm->def->cputune.nvcpupin, - vcpu); + virBitmapFree(vcpuinfo->cpumask); ret = 0; @@ -4937,10 +4934,13 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (persistentDef) { /* remove vcpupin entries for vcpus that were unplugged */ if (nvcpus < virDomainDefGetVcpus(persistentDef)) { - for (i = virDomainDefGetVcpus(persistentDef) - 1; i >= nvcpus; i--) - virDomainPinDel(&persistentDef->cputune.vcpupin, - &persistentDef->cputune.nvcpupin, - i); + for (i = virDomainDefGetVcpus(persistentDef) - 1; i >= nvcpus; i--) { + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(persistentDef, + i); + + if (vcpu) + virBitmapFree(vcpu->cpumask); + } } if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { @@ -4999,9 +4999,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virCgroupPtr cgroup_vcpu = NULL; int ret = -1; qemuDomainObjPrivatePtr priv; - size_t newVcpuPinNum = 0; - virDomainPinDefPtr *newVcpuPin = NULL; virBitmapPtr pcpumap = NULL; + virBitmapPtr pcpumaplive = NULL; + virBitmapPtr pcpumappersist = NULL; + virDomainVcpuInfoPtr vcpuinfolive = NULL; + virDomainVcpuInfoPtr vcpuinfopersist = NULL; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -5029,18 +5031,36 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, priv = vm->privateData; - if (def && vcpu >= virDomainDefGetVcpus(def)) { - virReportError(VIR_ERR_INVALID_ARG, - _("vcpu %d is out of range of live cpu count %d"), - vcpu, virDomainDefGetVcpus(def)); - goto endjob; + if (def) { + if (!(vcpuinfolive = virDomainDefGetVcpu(def, vcpu))) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, virDomainDefGetVcpus(def)); + goto endjob; + } + + if (!vcpuinfolive->online) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("setting cpu pinning for inactive vcpu '%d' is not " + "supported"), vcpu); + goto endjob; + } } - if (persistentDef && vcpu >= virDomainDefGetVcpus(persistentDef)) { - virReportError(VIR_ERR_INVALID_ARG, - _("vcpu %d is out of range of persistent cpu count %d"), - vcpu, virDomainDefGetVcpus(persistentDef)); - goto endjob; + if (persistentDef) { + if (!(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, virDomainDefGetVcpus(persistentDef)); + goto endjob; + } + + if (!vcpuinfopersist->online) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("setting cpu pinning for inactive vcpu '%d' is not " + "supported"), vcpu); + goto endjob; + } } if (!(pcpumap = virBitmapNewData(cpumap, maplen))) @@ -5052,6 +5072,10 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; } + if ((def && !(pcpumaplive = virBitmapNewCopy(pcpumap))) || + (persistentDef && !(pcpumappersist = virBitmapNewCopy(pcpumap)))) + goto endjob; + if (def) { if (!qemuDomainHasVcpuPids(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -5059,26 +5083,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; } - if (def->cputune.vcpupin) { - newVcpuPin = virDomainPinDefCopy(def->cputune.vcpupin, - def->cputune.nvcpupin); - if (!newVcpuPin) - goto endjob; - - newVcpuPinNum = def->cputune.nvcpupin; - } else { - if (VIR_ALLOC(newVcpuPin) < 0) - goto endjob; - newVcpuPinNum = 0; - } - - if (virDomainPinAdd(&newVcpuPin, &newVcpuPinNum, - cpumap, maplen, vcpu) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update vcpupin")); - goto endjob; - } - /* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu, @@ -5100,13 +5104,9 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, } } - if (def->cputune.vcpupin) - virDomainPinDefArrayFree(def->cputune.vcpupin, - def->cputune.nvcpupin); - - def->cputune.vcpupin = newVcpuPin; - def->cputune.nvcpupin = newVcpuPinNum; - newVcpuPin = NULL; + virBitmapFree(vcpuinfolive->cpumask); + vcpuinfolive->cpumask = pcpumaplive; + pcpumaplive = NULL; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -5125,24 +5125,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, } if (persistentDef) { - if (!persistentDef->cputune.vcpupin) { - if (VIR_ALLOC(persistentDef->cputune.vcpupin) < 0) - goto endjob; - persistentDef->cputune.nvcpupin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.vcpupin, - &persistentDef->cputune.nvcpupin, - cpumap, - maplen, - vcpu) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add vcpupin xml of " - "a persistent domain")); - goto endjob; - } + virBitmapFree(vcpuinfopersist->cpumask); + vcpuinfopersist->cpumask = pcpumappersist; + pcpumappersist = NULL; - ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto endjob; + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto endjob; } ret = 0; @@ -5151,14 +5139,14 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (newVcpuPin) - virDomainPinDefArrayFree(newVcpuPin, newVcpuPinNum); if (cgroup_vcpu) virCgroupFree(&cgroup_vcpu); virDomainObjEndAPI(&vm); qemuDomainEventQueue(driver, event); VIR_FREE(str); virBitmapFree(pcpumap); + virBitmapFree(pcpumaplive); + virBitmapFree(pcpumappersist); virObjectUnref(cfg); return ret; } @@ -5183,7 +5171,8 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, virDomainObjPtr vm = NULL; virDomainDefPtr def; int ret = -1; - int hostcpus, vcpu; + int hostcpus; + size_t i; virBitmapPtr allcpumap = NULL; qemuDomainObjPrivatePtr priv = NULL; @@ -5215,16 +5204,15 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (ncpumaps < 1) goto cleanup; - for (vcpu = 0; vcpu < ncpumaps; vcpu++) { - virDomainPinDefPtr pininfo; + for (i = 0; i < ncpumaps; i++) { + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i); virBitmapPtr bitmap = NULL; - pininfo = virDomainPinFind(def->cputune.vcpupin, - def->cputune.nvcpupin, - vcpu); + if (!vcpu->online) + continue; - if (pininfo && pininfo->cpumask) - bitmap = pininfo->cpumask; + if (vcpu->cpumask) + bitmap = vcpu->cpumask; else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && priv->autoCpuset) bitmap = priv->autoCpuset; @@ -5233,7 +5221,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, else bitmap = allcpumap; - virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen); + virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, i), maplen); } ret = ncpumaps; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 64b58be..c0043c9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2163,23 +2163,23 @@ static int qemuProcessSetVcpuAffinities(virDomainObjPtr vm) { virDomainDefPtr def = vm->def; - virDomainPinDefPtr pininfo; - int n; + virDomainVcpuInfoPtr vcpu; + size_t i; int ret = -1; - VIR_DEBUG("Setting affinity on CPUs nvcpupin=%zu nvcpus=%d hasVcpupids=%d", - def->cputune.nvcpupin, virDomainDefGetVcpus(def), - qemuDomainHasVcpuPids(vm)); - if (!def->cputune.nvcpupin) - return 0; + VIR_DEBUG("Setting affinity on CPUs"); if (!qemuDomainHasVcpuPids(vm)) { /* If any CPU has custom affinity that differs from the * VM default affinity, we must reject it */ - for (n = 0; n < def->cputune.nvcpupin; n++) { - if (def->cputune.vcpupin[n]->cpumask && - !virBitmapEqual(def->cpumask, - def->cputune.vcpupin[n]->cpumask)) { + for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { + vcpu = virDomainDefGetVcpu(def, i); + + if (!vcpu->online) + continue; + + if (vcpu->cpumask && + !virBitmapEqual(def->cpumask, vcpu->cpumask)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cpu affinity is not supported")); return -1; @@ -2188,22 +2188,20 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm) return 0; } - for (n = 0; n < virDomainDefGetVcpus(def); n++) { + for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { virBitmapPtr bitmap; - /* set affinity only for existing vcpus */ - if (!(pininfo = virDomainPinFind(def->cputune.vcpupin, - def->cputune.nvcpupin, - n))) + + vcpu = virDomainDefGetVcpu(def, i); + + if (!vcpu->online) continue; - if (!(bitmap = pininfo->cpumask) && + if (!(bitmap = vcpu->cpumask) && !(bitmap = def->cpumask)) continue; - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, n), - pininfo->cpumask) < 0) { + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, i), bitmap) < 0) goto cleanup; - } } ret = 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5986749..ed4de12 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2455,15 +2455,14 @@ static int testDomainGetVcpus(virDomainPtr domain, memset(cpumaps, 0, maxinfo * maplen); for (i = 0; i < maxinfo; i++) { - virDomainPinDefPtr pininfo; + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i); virBitmapPtr bitmap = NULL; - pininfo = virDomainPinFind(def->cputune.vcpupin, - def->cputune.nvcpupin, - i); + if (vcpu && !vcpu->online) + continue; - if (pininfo && pininfo->cpumask) - bitmap = pininfo->cpumask; + if (vcpu->cpumask) + bitmap = vcpu->cpumask; else if (def->cpumask) bitmap = def->cpumask; else @@ -2492,6 +2491,7 @@ static int testDomainPinVcpu(virDomainPtr domain, unsigned char *cpumap, int maplen) { + virDomainVcpuInfoPtr vcpuinfo; virDomainObjPtr privdom; virDomainDefPtr def; int ret = -1; @@ -2507,29 +2507,21 @@ static int testDomainPinVcpu(virDomainPtr domain, goto cleanup; } - if (vcpu > virDomainDefGetVcpus(privdom->def)) { + if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu)) || + !vcpuinfo->online) { virReportError(VIR_ERR_INVALID_ARG, _("requested vcpu '%d' is not present in the domain"), vcpu); goto cleanup; } - if (!def->cputune.vcpupin) { - if (VIR_ALLOC(def->cputune.vcpupin) < 0) - goto cleanup; - def->cputune.nvcpupin = 0; - } - if (virDomainPinAdd(&def->cputune.vcpupin, - &def->cputune.nvcpupin, - cpumap, - maplen, - vcpu) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add vcpupin")); + virBitmapFree(vcpuinfo->cpumask); + + if (!(vcpuinfo->cpumask = virBitmapNewData(cpumap, maplen))) goto cleanup; - } ret = 0; + cleanup: virDomainObjEndAPI(&privdom); return ret; @@ -2566,15 +2558,14 @@ testDomainGetVcpuPinInfo(virDomainPtr dom, ncpumaps = virDomainDefGetVcpus(def); for (vcpu = 0; vcpu < ncpumaps; vcpu++) { - virDomainPinDefPtr pininfo; + virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(def, vcpu); virBitmapPtr bitmap = NULL; - pininfo = virDomainPinFind(def->cputune.vcpupin, - def->cputune.nvcpupin, - vcpu); + if (vcpuinfo && !vcpuinfo->online) + continue; - if (pininfo && pininfo->cpumask) - bitmap = pininfo->cpumask; + if (vcpuinfo->cpumask) + bitmap = vcpuinfo->cpumask; else if (def->cpumask) bitmap = def->cpumask; else -- 2.6.2

On 01/14/2016 11:27 AM, Peter Krempa wrote: Not sure why patch 19 is interspersed since 18 and 20 are "related" Will there need to be a vcpuinfo->cpumask = NULL afterwards? Since this is pass by value and not reference. This is called by virDomainDefSetVcpusMax so it's not just in a pur *Free path Is this message true for !online as well? or just there because of the maxvcpus check? BTW: Another place where checking an OnlineVcpuMap could be beneficial. I would think this check could go prior to parsing "cpuset" bikeshed... why not 'cpupinmask' (or something to make it easier to find) I understand the desire to keep things named similarly (cpumask), but it just makes it more difficult to "find" specific instances... Here's another OnlineVcpuMap perusal opportunity... Ditto here too in some way (OnlineVcpuMap) Will there need to be a vcpuinfo->cpumask = NULL afterwards? Since this is pass by value and not reference. Will there need to be a vcpuinfo->cpumask = NULL afterwards? Since this is pass by value and not reference. Another place for OnlineVcpuMap (persist path too) This looked OK, but the names of the new variables caused a few brain cramps... Like a long runon sentence. Another OnlineVcpuMap opportunity. Another OnlineVcpuMap opportunity (and the following change too)

On 01/14/2016 11:27 AM, Peter Krempa wrote: Is this going to be problematic for a running domain? e.g., would it disappear now? I agree with the failure, just worried about the consequences. Not that anyone should have done this, but seems to be one of those post parse, pre-run type checks since it's been allowed in the past. John

It's disallowed in the API. --- src/conf/domain_conf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1036057..e27de96 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14246,8 +14246,18 @@ virDomainEmulatorPinDefParseXML(xmlNodePtr node) return NULL; } - ignore_value(virBitmapParse(tmp, 0, &def, VIR_DOMAIN_CPUMASK_LEN)); + if (virBitmapParse(tmp, 0, &def, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + + if (virBitmapIsAllClear(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'cpuset': %s"), tmp); + virBitmapFree(def); + def = NULL; + goto cleanup; + } + cleanup: VIR_FREE(tmp); return def; } -- 2.6.2

virDomainDefFormatInternal is growing rather large. Extract the cputune formatter into a separate function. --- src/conf/domain_conf.c | 230 +++++++++++++++++++++++++++---------------------- 1 file changed, 125 insertions(+), 105 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e27de96..b18ce8d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21452,6 +21452,129 @@ virDomainDefHasCapabilitiesFeatures(virDomainDefPtr def) return false; } + +static int +virDomainCputuneDefFormat(virBufferPtr buf, + virDomainDefPtr def) +{ + size_t i; + virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; + + virBufferAdjustIndent(&childrenBuf, virBufferGetIndent(buf, false) + 2); + + if (def->cputune.sharesSpecified) + virBufferAsprintf(&childrenBuf, "<shares>%lu</shares>\n", + def->cputune.shares); + if (def->cputune.period) + virBufferAsprintf(&childrenBuf, "<period>%llu</period>\n", + def->cputune.period); + if (def->cputune.quota) + virBufferAsprintf(&childrenBuf, "<quota>%lld</quota>\n", + def->cputune.quota); + + if (def->cputune.emulator_period) + virBufferAsprintf(&childrenBuf, "<emulator_period>%llu" + "</emulator_period>\n", + def->cputune.emulator_period); + + if (def->cputune.emulator_quota) + virBufferAsprintf(&childrenBuf, "<emulator_quota>%lld" + "</emulator_quota>\n", + def->cputune.emulator_quota); + + for (i = 0; i < def->maxvcpus; i++) { + char *cpumask; + virDomainVcpuInfoPtr vcpu = def->vcpus + i; + + if (!vcpu->cpumask) + continue; + + if (!(cpumask = virBitmapFormat(vcpu->cpumask))) + goto cleanup; + + virBufferAsprintf(&childrenBuf, + "<vcpupin vcpu='%zu' cpuset='%s'/>\n", i, cpumask); + + VIR_FREE(cpumask); + } + + if (def->cputune.emulatorpin) { + char *cpumask; + virBufferAddLit(&childrenBuf, "<emulatorpin "); + + if (!(cpumask = virBitmapFormat(def->cputune.emulatorpin))) + goto cleanup; + + virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); + } + + for (i = 0; i < def->niothreadids; i++) { + char *cpumask; + + /* Ignore iothreadids with no cpumask */ + if (!def->iothreadids[i]->cpumask) + continue; + + virBufferAsprintf(&childrenBuf, "<iothreadpin iothread='%u' ", + def->iothreadids[i]->iothread_id); + + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) + goto cleanup; + + virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); + } + + for (i = 0; i < def->cputune.nvcpusched; i++) { + virDomainThreadSchedParamPtr sp = &def->cputune.vcpusched[i]; + char *ids = NULL; + + if (!(ids = virBitmapFormat(sp->ids))) + goto cleanup; + + virBufferAsprintf(&childrenBuf, "<vcpusched vcpus='%s' scheduler='%s'", + ids, virProcessSchedPolicyTypeToString(sp->policy)); + VIR_FREE(ids); + + if (sp->policy == VIR_PROC_POLICY_FIFO || + sp->policy == VIR_PROC_POLICY_RR) + virBufferAsprintf(&childrenBuf, " priority='%d'", sp->priority); + virBufferAddLit(&childrenBuf, "/>\n"); + } + + for (i = 0; i < def->cputune.niothreadsched; i++) { + virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i]; + char *ids = NULL; + + if (!(ids = virBitmapFormat(sp->ids))) + goto cleanup; + + virBufferAsprintf(&childrenBuf, "<iothreadsched iothreads='%s' scheduler='%s'", + ids, virProcessSchedPolicyTypeToString(sp->policy)); + VIR_FREE(ids); + + if (sp->policy == VIR_PROC_POLICY_FIFO || + sp->policy == VIR_PROC_POLICY_RR) + virBufferAsprintf(&childrenBuf, " priority='%d'", sp->priority); + virBufferAddLit(&childrenBuf, "/>\n"); + } + + if (virBufferUse(&childrenBuf)) { + virBufferAddLit(buf, "<cputune>\n"); + virBufferAddBuffer(buf, &childrenBuf); + virBufferAddLit(buf, "</cputune>\n"); + } + + ret = 0; + + cleanup: + virBufferFreeAndReset(&childrenBuf); + return ret; +} + + /* This internal version appends to an existing buffer * (possibly with auto-indent), rather than flattening * to string. @@ -21662,111 +21785,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, } } - /* start format cputune */ - indent = virBufferGetIndent(buf, false); - virBufferAdjustIndent(&childrenBuf, indent + 2); - if (def->cputune.sharesSpecified) - virBufferAsprintf(&childrenBuf, "<shares>%lu</shares>\n", - def->cputune.shares); - if (def->cputune.period) - virBufferAsprintf(&childrenBuf, "<period>%llu</period>\n", - def->cputune.period); - if (def->cputune.quota) - virBufferAsprintf(&childrenBuf, "<quota>%lld</quota>\n", - def->cputune.quota); - - if (def->cputune.emulator_period) - virBufferAsprintf(&childrenBuf, "<emulator_period>%llu" - "</emulator_period>\n", - def->cputune.emulator_period); - - if (def->cputune.emulator_quota) - virBufferAsprintf(&childrenBuf, "<emulator_quota>%lld" - "</emulator_quota>\n", - def->cputune.emulator_quota); - - for (i = 0; i < def->maxvcpus; i++) { - char *cpumask; - virDomainVcpuInfoPtr vcpu = def->vcpus + i; - - if (!vcpu->cpumask) - continue; - - if (!(cpumask = virBitmapFormat(vcpu->cpumask))) - goto error; - - virBufferAsprintf(&childrenBuf, - "<vcpupin vcpu='%zu' cpuset='%s'/>\n", i, cpumask); - - VIR_FREE(cpumask); - } - - if (def->cputune.emulatorpin) { - char *cpumask; - virBufferAddLit(&childrenBuf, "<emulatorpin "); - - if (!(cpumask = virBitmapFormat(def->cputune.emulatorpin))) - goto error; - - virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); - } - - for (i = 0; i < def->niothreadids; i++) { - char *cpumask; - - /* Ignore iothreadids with no cpumask */ - if (!def->iothreadids[i]->cpumask) - continue; - - virBufferAsprintf(&childrenBuf, "<iothreadpin iothread='%u' ", - def->iothreadids[i]->iothread_id); - - if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) - goto error; - - virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); - } - - for (i = 0; i < def->cputune.nvcpusched; i++) { - virDomainThreadSchedParamPtr sp = &def->cputune.vcpusched[i]; - char *ids = NULL; - - if (!(ids = virBitmapFormat(sp->ids))) - goto error; - virBufferAsprintf(&childrenBuf, "<vcpusched vcpus='%s' scheduler='%s'", - ids, virProcessSchedPolicyTypeToString(sp->policy)); - VIR_FREE(ids); - - if (sp->policy == VIR_PROC_POLICY_FIFO || - sp->policy == VIR_PROC_POLICY_RR) - virBufferAsprintf(&childrenBuf, " priority='%d'", sp->priority); - virBufferAddLit(&childrenBuf, "/>\n"); - } - - for (i = 0; i < def->cputune.niothreadsched; i++) { - virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i]; - char *ids = NULL; - - if (!(ids = virBitmapFormat(sp->ids))) - goto error; - virBufferAsprintf(&childrenBuf, "<iothreadsched iothreads='%s' scheduler='%s'", - ids, virProcessSchedPolicyTypeToString(sp->policy)); - VIR_FREE(ids); - - if (sp->policy == VIR_PROC_POLICY_FIFO || - sp->policy == VIR_PROC_POLICY_RR) - virBufferAsprintf(&childrenBuf, " priority='%d'", sp->priority); - virBufferAddLit(&childrenBuf, "/>\n"); - } - - if (virBufferUse(&childrenBuf)) { - virBufferAddLit(buf, "<cputune>\n"); - virBufferAddBuffer(buf, &childrenBuf); - virBufferAddLit(buf, "</cputune>\n"); - } - virBufferFreeAndReset(&childrenBuf); + if (virDomainCputuneDefFormat(buf, def) < 0) + goto error; if (virDomainNumatuneFormatXML(buf, def->numa) < 0) goto error; -- 2.6.2

Idioms are usually weird and obscure when translated literally. --- src/util/virbuffer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 55dad37..43cd1a7 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -180,8 +180,7 @@ virBufferAdd(virBufferPtr buf, const char *str, int len) * virBufferContentAndReset(), virBufferAdd(). Auto indentation * is (intentionally) NOT applied! * - * Moreover, be aware that @toadd is eaten with hair. IOW, the - * @toadd buffer is reset after this. + * The @toadd virBuffer is consumed and cleared. */ void virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd) -- 2.6.2

Performs binary subtraction of two bitmaps. Stores result in the first operand. --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 21 ++++++++++++++++++ src/util/virbitmap.h | 3 +++ tests/virbitmaptest.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9cebaeb..8213ff3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1151,6 +1151,7 @@ virBitmapSetAll; virBitmapSetBit; virBitmapSize; virBitmapString; +virBitmapSubtract; virBitmapToData; virBitmapToDataBuf; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 57135a0..f116607 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -859,3 +859,24 @@ virBitmapOverlaps(virBitmapPtr b1, return false; } + +/** + * virBitmapSubtract: + * @a: minuend/result + * @b: subtrahend + * + * Performs bitwise subtraction: a = a - b + */ +void +virBitmapSubtract(virBitmapPtr a, + virBitmapPtr b) +{ + size_t i; + size_t max = a->map_len; + + if (max > b->map_len) + max = b->map_len; + + for (i = 0; i < max; i++) + a->map[i] &= ~b->map[i]; +} diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 47488de..846aca3 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -129,4 +129,7 @@ bool virBitmapOverlaps(virBitmapPtr b1, virBitmapPtr b2) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 8e458d2..967a5c8 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -552,9 +552,55 @@ test10(const void *opaque ATTRIBUTE_UNUSED) return ret; } +struct testBinaryOpData { + const char *a; + const char *b; + const char *res; +}; + +static int +test11(const void *opaque) +{ + const struct testBinaryOpData *data = opaque; + virBitmapPtr amap = NULL; + virBitmapPtr bmap = NULL; + virBitmapPtr resmap = NULL; + int ret = -1; + + if (virBitmapParse(data->a, 0, &amap, 256) < 0 || + virBitmapParse(data->b, 0, &bmap, 256) < 0 || + virBitmapParse(data->res, 0, &resmap, 256) < 0) + goto cleanup; + + virBitmapSubtract(amap, bmap); + + if (!virBitmapEqual(amap, resmap)) { + fprintf(stderr, "\n bitmap subtraction failed: '%s'-'%s'!='%s'\n", + data->a, data->b, data->res); + goto cleanup; + } + + ret = 0; + + cleanup: + virBitmapFree(amap); + virBitmapFree(bmap); + virBitmapFree(resmap); + + return ret; +} + +#define TESTBINARYOP(A, B, RES, FUNC) \ + testBinaryOpData.a = A; \ + testBinaryOpData.b = B; \ + testBinaryOpData.res = RES; \ + if (virtTestRun(virtTestCounterNext(), FUNC, &testBinaryOpData) < 0) \ + ret = -1; + static int mymain(void) { + struct testBinaryOpData testBinaryOpData; int ret = 0; if (virtTestRun("test1", test1, NULL) < 0) @@ -578,6 +624,15 @@ mymain(void) if (virtTestRun("test10", test10, NULL) < 0) ret = -1; + virtTestCounterReset("test11-"); + TESTBINARYOP("0", "0", "0,^0", test11); + TESTBINARYOP("0-3", "0", "1-3", test11); + TESTBINARYOP("0-3", "0,3", "1-2", test11); + TESTBINARYOP("0,^0", "0", "0,^0", test11); + TESTBINARYOP("0-3", "0-3", "0,^0", test11); + TESTBINARYOP("0-3", "0,^0", "0-3", test11); + TESTBINARYOP("0,2", "1,3", "0,2", test11); + return ret; } -- 2.6.2

On 01/14/2016 11:27 AM, Peter Krempa wrote: Could be closer to usage in 30/34, but I understand the current order after reading ahead... Does the function really need to be added to libvirt_private.syms? It's only used in domain_conf.c. I also have similar question and concern about virDomainDefGetVcpuSched in patch 29. John

On 01/14/2016 11:27 AM, Peter Krempa wrote: I see 'sp' being changed to two args by reference is necessary for patch 29... Considering what I'll note later, this needs to change to "Missing attribute '%s' in element '%sched'" Ahh - I see this is setting up for a future patch where %s will be "vcpus" or "iothreads"... It's the attribute name found in the XML for the <iothreadsched iothreads='xxx'" or "<vcpusched vcpus='xxx'" Wasn't really "clear" without a description and without peeking ahead a few patches. I think the message should be: "%sched bitmap is out of range" since 'name' is passed as "vcpus" or "iothreads" (with the 's' already). The resultant element name of vcpusched or iothreadsched is then generated (similar to how it's done in virDomainFormatSchedDef). John

On 01/14/2016 11:27 AM, Peter Krempa wrote: This is related to patch 26 at least w/r/t virBitmapSubtract usage. There's also multiple things going on - between code motion and code addition. In particular virDomainFormatSchedDef is does quite a lot... Hopefully someone else (perhaps Martin) who's worked in the sched code before can take a look! s/^/^static / ?? Does there need to be a vcpuinfo->online check? (aka OnlineVcpuMap) Will the caller need to differentiate? I know this is the parsing code... just thinking while typing especially for non-static function. Later thoughts made me think this should be static for parse... Replacing the virBitmapOverlaps... Could this be 'continue;' ? That is, is the data required? I'm thinking primarily of the vcpu->online == false case... Since the code will be shared in patch 30, change message to: "%sched attribute '%s' must not overlap", using 'name' for both %s params. Similar to virDomainFormatSchedDef has done things. Probably a good place to note the arguments, but specifically that "name" is used to generate the XML "<vcpusched vcpus='..." or "<iothreadsched iothreads='..." This format works right because the passed name is "vcpu" or "iothread" This is one heckuva loop - I hope others can look as well because my eyes and brain decided to run in the other direction ;-) Hmm... a zero for maxvcpus... In patch 2 we disallow machines with 0 vcpus online... Just a strange check. use of same function - although I assume the compiler will optimize that for you anyway... This differs slightly from Parse which uses "vcpus" - I'm wondering if it should be consistent. At the very least documented... s/doens't/doesn't/ Not in libvirt_private.syms... So far this isn't external to domain_conf.c - so probably should be static to there. Concern is calling from driver/qemu code could result in returning an 'offline' definition.

Similarly to previous commit change the way how iothread scheduler info is stored and clean up a lot of unnecessary code. --- src/conf/domain_conf.c | 141 +++++++-------------- src/conf/domain_conf.h | 8 +- src/libvirt_private.syms | 1 - src/qemu/qemu_driver.c | 3 - src/qemu/qemu_process.c | 39 +----- .../qemuxml2xmlout-cputune-iothreadsched.xml | 3 +- 6 files changed, 53 insertions(+), 142 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e2dda9a..4ca03d9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2559,10 +2559,6 @@ void virDomainDefFree(virDomainDefPtr def) virBitmapFree(def->cputune.emulatorpin); - for (i = 0; i < def->cputune.niothreadsched; i++) - virBitmapFree(def->cputune.iothreadsched[i].ids); - VIR_FREE(def->cputune.iothreadsched); - virDomainNumaFree(def->numa); virSysinfoDefFree(def->sysinfo); @@ -14649,25 +14645,26 @@ virDomainVcpuThreadSchedParse(xmlNodePtr node, } -static int -virDomainThreadSchedParse(xmlNodePtr node, - unsigned int minid, - unsigned int maxid, - const char *name, - virDomainThreadSchedParamPtr sp) -{ - if (!(sp->ids = virDomainSchedulerParse(node, name, &sp->policy, - &sp->priority))) - return -1; +static virDomainThreadSchedParamPtr +virDomainDefGetIOThreadSched(virDomainDefPtr def, + unsigned int iothread) +{ + virDomainIOThreadIDDefPtr iothrinfo; - if (virBitmapNextSetBit(sp->ids, -1) < minid || - virBitmapLastSetBit(sp->ids) > maxid) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("%ssched bitmap is out of range"), name); - return -1; - } + if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread))) + return NULL; - return 0; + return &iothrinfo->sched; +} + + +static int +virDomainIOThreadSchedParse(xmlNodePtr node, + virDomainDefPtr def) +{ + return virDomainThreadSchedParseHelper(node, "iothreads", + virDomainDefGetIOThreadSched, + def); } @@ -15215,46 +15212,10 @@ virDomainDefParseXML(xmlDocPtr xml, _("cannot extract iothreadsched nodes")); goto error; } - if (n) { - if (n > def->niothreadids) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("too many iothreadsched nodes in cputune")); - goto error; - } - if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0) + for (i = 0; i < n; i++) { + if (virDomainIOThreadSchedParse(nodes[i], def) < 0) goto error; - def->cputune.niothreadsched = n; - - for (i = 0; i < def->cputune.niothreadsched; i++) { - ssize_t pos = -1; - - if (virDomainThreadSchedParse(nodes[i], - 1, UINT_MAX, - "iothreads", - &def->cputune.iothreadsched[i]) < 0) - goto error; - - while ((pos = virBitmapNextSetBit(def->cputune.iothreadsched[i].ids, - pos)) > -1) { - if (!virDomainIOThreadIDFind(def, pos)) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("iothreadsched attribute 'iothreads' " - "uses undefined iothread ids")); - goto error; - } - } - - for (j = 0; j < i; j++) { - if (virBitmapOverlaps(def->cputune.iothreadsched[i].ids, - def->cputune.iothreadsched[j].ids)) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("iothreadsched attributes 'iothreads' " - "must not overlap")); - goto error; - } - } - } } VIR_FREE(nodes); @@ -18448,29 +18409,6 @@ virDomainIOThreadIDDel(virDomainDefPtr def, } } -void -virDomainIOThreadSchedDelId(virDomainDefPtr def, - unsigned int iothreadid) -{ - size_t i; - - if (!def->cputune.iothreadsched || !def->cputune.niothreadsched) - return; - - for (i = 0; i < def->cputune.niothreadsched; i++) { - if (virBitmapIsBitSet(def->cputune.iothreadsched[i].ids, iothreadid)) { - ignore_value(virBitmapClearBit(def->cputune.iothreadsched[i].ids, - iothreadid)); - if (virBitmapIsAllClear(def->cputune.iothreadsched[i].ids)) { - virBitmapFree(def->cputune.iothreadsched[i].ids); - VIR_DELETE_ELEMENT(def->cputune.iothreadsched, i, - def->cputune.niothreadsched); - } - return; - } - } -} - static int virDomainEventActionDefFormat(virBufferPtr buf, @@ -21665,6 +21603,27 @@ virDomainFormatVcpuSchedDef(virDomainDefPtr def, static int +virDomainFormatIOThreadSchedDef(virDomainDefPtr def, + virBufferPtr buf) +{ + virBitmapPtr allthreadmap; + int ret; + + if (def->niothreadids == 0) + return 0; + + if (!(allthreadmap = virDomainIOThreadIDMap(def))) + return -1; + + ret = virDomainFormatSchedDef(def, buf, "iothread", + virDomainDefGetIOThreadSched, allthreadmap); + + virBitmapFree(allthreadmap); + return ret; +} + + +static int virDomainCputuneDefFormat(virBufferPtr buf, virDomainDefPtr def) { @@ -21741,22 +21700,8 @@ virDomainCputuneDefFormat(virBufferPtr buf, if (virDomainFormatVcpuSchedDef(def, &childrenBuf) < 0) goto cleanup; - for (i = 0; i < def->cputune.niothreadsched; i++) { - virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i]; - char *ids = NULL; - - if (!(ids = virBitmapFormat(sp->ids))) - goto cleanup; - - virBufferAsprintf(&childrenBuf, "<iothreadsched iothreads='%s' scheduler='%s'", - ids, virProcessSchedPolicyTypeToString(sp->policy)); - VIR_FREE(ids); - - if (sp->policy == VIR_PROC_POLICY_FIFO || - sp->policy == VIR_PROC_POLICY_RR) - virBufferAsprintf(&childrenBuf, " priority='%d'", sp->priority); - virBufferAddLit(&childrenBuf, "/>\n"); - } + if (virDomainFormatIOThreadSchedDef(def, &childrenBuf) < 0) + goto cleanup; if (virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "<cputune>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 85740bc..b93835a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1898,7 +1898,6 @@ typedef enum { typedef struct _virDomainThreadSchedParam virDomainThreadSchedParam; typedef virDomainThreadSchedParam *virDomainThreadSchedParamPtr; struct _virDomainThreadSchedParam { - virBitmapPtr ids; virProcessSchedPolicy policy; int priority; }; @@ -2095,6 +2094,8 @@ struct _virDomainIOThreadIDDef { unsigned int iothread_id; int thread_id; virBitmapPtr cpumask; + + virDomainThreadSchedParam sched; }; void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); @@ -2111,9 +2112,6 @@ struct _virDomainCputune { unsigned long long emulator_period; long long emulator_quota; virBitmapPtr emulatorpin; - - size_t niothreadsched; - virDomainThreadSchedParamPtr iothreadsched; }; @@ -2124,7 +2122,6 @@ struct _virDomainVcpuInfo { bool online; virBitmapPtr cpumask; - /* note: the sched.ids bitmap is unused so it doens't have to be cleared */ virDomainThreadSchedParam sched; }; @@ -2708,7 +2705,6 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, virBitmapPtr virDomainIOThreadIDMap(virDomainDefPtr def) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id); -void virDomainIOThreadSchedDelId(virDomainDefPtr def, unsigned int iothread_id); unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c84672..cd595b0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -343,7 +343,6 @@ virDomainIOThreadIDDefFree; virDomainIOThreadIDDel; virDomainIOThreadIDFind; virDomainIOThreadIDMap; -virDomainIOThreadSchedDelId; virDomainKeyWrapCipherNameTypeFromString; virDomainKeyWrapCipherNameTypeToString; virDomainLeaseDefFree; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dfed936..34e82c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6043,8 +6043,6 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, virDomainIOThreadIDDel(vm->def, iothread_id); - virDomainIOThreadSchedDelId(vm->def, iothread_id); - if (qemuDomainDelCgroupForThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, iothread_id) < 0) @@ -6134,7 +6132,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, } virDomainIOThreadIDDel(persistentDef, iothread_id); - virDomainIOThreadSchedDelId(persistentDef, iothread_id); persistentDef->iothreads--; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3c1c6d8..abafbb1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2251,34 +2251,6 @@ qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) return ret; } -/* Set Scheduler parameters for vCPU or I/O threads. */ -int -qemuProcessSetSchedParams(int id, - pid_t pid, - size_t nsp, - virDomainThreadSchedParamPtr sp) -{ - bool val = false; - size_t i = 0; - virDomainThreadSchedParamPtr s = NULL; - - for (i = 0; i < nsp; i++) { - if (virBitmapGetBit(sp[i].ids, id, &val) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot get bit from bitmap")); - } - if (val) { - s = &sp[i]; - break; - } - } - - if (!s) - return 0; - - return virProcessSetScheduler(pid, s->policy, s->priority); -} - static int qemuProcessSetSchedulers(virDomainObjPtr vm) { @@ -2297,10 +2269,13 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) } for (i = 0; i < vm->def->niothreadids; i++) { - if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, - vm->def->iothreadids[i]->thread_id, - vm->def->cputune.niothreadsched, - vm->def->cputune.iothreadsched) < 0) + virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i]; + + if (info->sched.policy == VIR_PROC_POLICY_NONE) + continue; + + if (virProcessSetScheduler(info->thread_id, info->sched.policy, + info->sched.priority) < 0) return -1; } diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml index 9f61336..01f1af9 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml @@ -13,8 +13,7 @@ <vcpupin vcpu='1' cpuset='1'/> <emulatorpin cpuset='1'/> <vcpusched vcpus='0-1' scheduler='fifo' priority='1'/> - <iothreadsched iothreads='1,3' scheduler='batch'/> - <iothreadsched iothreads='2' scheduler='batch'/> + <iothreadsched iothreads='1-3' scheduler='batch'/> </cputune> <os> <type arch='i686' machine='pc'>hvm</type> -- 2.6.2

Rather than iterating 3 times for various settings this function aggregates all the code into single place. One of the other advantages is that it can then be reused for properly setting vCPU info on hotplug. --- src/qemu/qemu_cgroup.c | 98 ----------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_process.c | 205 ++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 4 + 4 files changed, 141 insertions(+), 167 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3744b52..827401e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1001,104 +1001,6 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, return ret; } -int -qemuSetupCgroupForVcpu(virDomainObjPtr vm) -{ - virCgroupPtr cgroup_vcpu = NULL; - qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainDefPtr def = vm->def; - size_t i; - unsigned long long period = vm->def->cputune.period; - long long quota = vm->def->cputune.quota; - char *mem_mask = NULL; - virDomainNumatuneMemMode mem_mode; - - if ((period || quota) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cgroup cpu is required for scheduler tuning")); - return -1; - } - - /* - * If CPU cgroup controller is not initialized here, then we need - * neither period nor quota settings. And if CPUSET controller is - * not initialized either, then there's nothing to do anyway. CPU pinning - * will be set via virProcessSetAffinity. - */ - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) - return 0; - - /* If vCPU<->pid mapping is missing we can't do vCPU pinning */ - if (!qemuDomainHasVcpuPids(vm)) - return 0; - - if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && - mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virDomainNumatuneMaybeFormatNodeset(vm->def->numa, - priv->autoNodeset, - &mem_mask, -1) < 0) - goto cleanup; - - for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { - virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i); - - if (!vcpu->online) - continue; - - virCgroupFree(&cgroup_vcpu); - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i, - true, &cgroup_vcpu) < 0) - goto cleanup; - - if (period || quota) { - if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) - goto cleanup; - } - - /* Set vcpupin in cgroup if vcpupin xml is provided */ - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - virBitmapPtr cpumap = NULL; - - if (mem_mask && - virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0) - goto cleanup; - - if (vcpu->cpumask) - cpumap = vcpu->cpumask; - else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - cpumap = priv->autoCpuset; - else - cpumap = vm->def->cpumask; - - if (!cpumap) - continue; - - if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) - goto cleanup; - } - - /* move the thread for vcpu to sub dir */ - if (virCgroupAddTask(cgroup_vcpu, - qemuDomainGetVcpuPid(vm, i)) < 0) - goto cleanup; - - } - virCgroupFree(&cgroup_vcpu); - VIR_FREE(mem_mask); - - return 0; - - cleanup: - if (cgroup_vcpu) { - virCgroupRemove(cgroup_vcpu); - virCgroupFree(&cgroup_vcpu); - } - VIR_FREE(mem_mask); - - return -1; -} int qemuSetupCgroupForEmulator(virDomainObjPtr vm) diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 2bcf071..fa3353a 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -53,7 +53,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, unsigned long long period, long long quota); int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); -int qemuSetupCgroupForVcpu(virDomainObjPtr vm); int qemuSetupCgroupForIOThreads(virDomainObjPtr vm); int qemuSetupCgroupForEmulator(virDomainObjPtr vm); int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index abafbb1..2a783e5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2158,56 +2158,6 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, return ret; } -/* Set CPU affinities for vcpus if vcpupin xml provided. */ -static int -qemuProcessSetVcpuAffinities(virDomainObjPtr vm) -{ - virDomainDefPtr def = vm->def; - virDomainVcpuInfoPtr vcpu; - size_t i; - int ret = -1; - VIR_DEBUG("Setting affinity on CPUs"); - - if (!qemuDomainHasVcpuPids(vm)) { - /* If any CPU has custom affinity that differs from the - * VM default affinity, we must reject it - */ - for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { - vcpu = virDomainDefGetVcpu(def, i); - - if (!vcpu->online) - continue; - - if (vcpu->cpumask && - !virBitmapEqual(def->cpumask, vcpu->cpumask)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cpu affinity is not supported")); - return -1; - } - } - return 0; - } - - for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { - virBitmapPtr bitmap; - - vcpu = virDomainDefGetVcpu(def, i); - - if (!vcpu->online) - continue; - - if (!(bitmap = vcpu->cpumask) && - !(bitmap = def->cpumask)) - continue; - - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, i), bitmap) < 0) - goto cleanup; - } - - ret = 0; - cleanup: - return ret; -} /* Set CPU affinities for emulator threads. */ static int @@ -2256,18 +2206,6 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) { size_t i = 0; - for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { - virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, i); - - if (!vcpu->online || - vcpu->sched.policy == VIR_PROC_POLICY_NONE) - continue; - - if (virProcessSetScheduler(qemuDomainGetVcpuPid(vm, i), - vcpu->sched.policy, vcpu->sched.priority) < 0) - return -1; - } - for (i = 0; i < vm->def->niothreadids; i++) { virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i]; @@ -4425,6 +4363,141 @@ qemuProcessInit(virQEMUDriverPtr driver, } +int +qemuProcessSetupVcpu(virDomainObjPtr vm, + unsigned int vcpuid) +{ + pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid); + virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid); + qemuDomainObjPrivatePtr priv = vm->privateData; + char *mem_mask = NULL; + virDomainNumatuneMemMode mem_mode; + unsigned long long period = vm->def->cputune.period; + long long quota = vm->def->cputune.quota; + virCgroupPtr cgroup_vcpu = NULL; + virBitmapPtr cpumask; + int ret = -1; + + if (period || quota) { + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cgroup cpu is required for scheduler tuning")); + goto cleanup; + } + } + + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) || + virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && + mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, + priv->autoNodeset, + &mem_mask, -1) < 0) + goto cleanup; + + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpuid, + true, &cgroup_vcpu) < 0) + goto cleanup; + + if (period || quota) { + if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) + goto cleanup; + } + } + + /* infer which cpumask shall be used */ + if (vcpu->cpumask) + cpumask = vcpu->cpumask; + else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + cpumask = priv->autoCpuset; + else + cpumask = vm->def->cpumask; + + if (cpumask) { + /* setup legacy affinty */ + if (virProcessSetAffinity(vcpupid, cpumask) < 0) + goto cleanup; + + /* setup cgroups */ + if (cgroup_vcpu && + virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + if (mem_mask && + virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0) + goto cleanup; + + if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumask) < 0) + goto cleanup; + } + } + + /* move the thread for vcpu to sub dir */ + if (cgroup_vcpu && + virCgroupAddTask(cgroup_vcpu, vcpupid) < 0) + goto cleanup; + + /* set scheduler type and priority */ + if (vcpu->sched.policy != VIR_PROC_POLICY_NONE) { + if (virProcessSetScheduler(vcpupid, vcpu->sched.policy, + vcpu->sched.priority) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(mem_mask); + if (cgroup_vcpu) { + if (ret < 0) + virCgroupRemove(cgroup_vcpu); + virCgroupFree(&cgroup_vcpu); + } + + return ret; +} + + +static int +qemuProcessSetupVcpus(virDomainObjPtr vm) +{ + virDomainVcpuInfoPtr vcpu; + unsigned int maxvcpus = virDomainDefGetVcpusMax(vm->def); + size_t i; + + if (!qemuDomainHasVcpuPids(vm)) { + /* If any CPU has custom affinity that differs from the + * VM default affinity, we must reject it */ + for (i = 0; i < maxvcpus; i++) { + vcpu = virDomainDefGetVcpu(vm->def, i); + + if (!vcpu->online) + continue; + + if (vcpu->cpumask && + !virBitmapEqual(vm->def->cpumask, vcpu->cpumask)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cpu affinity is not supported")); + return -1; + } + } + + return 0; + } + + for (i = 0; i < maxvcpus; i++) { + vcpu = virDomainDefGetVcpu(vm->def, i); + + if (!vcpu->online) + continue; + + if (qemuProcessSetupVcpu(vm, i) < 0) + return -1; + } + + return 0; +} + + /** * qemuProcessLaunch: * @@ -4896,18 +4969,14 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0) goto cleanup; - VIR_DEBUG("Setting cgroup for each VCPU (if required)"); - if (qemuSetupCgroupForVcpu(vm) < 0) + VIR_DEBUG("Setting vCPU tuning/settings"); + if (qemuProcessSetupVcpus(vm) < 0) goto cleanup; VIR_DEBUG("Setting cgroup for each IOThread (if required)"); if (qemuSetupCgroupForIOThreads(vm) < 0) goto cleanup; - VIR_DEBUG("Setting VCPU affinities"); - if (qemuProcessSetVcpuAffinities(vm) < 0) - goto cleanup; - VIR_DEBUG("Setting affinity of IOThread threads"); if (qemuProcessSetIOThreadsAffinity(vm) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index c674111..a2663a0 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -155,4 +155,8 @@ virDomainDiskDefPtr qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm, int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm); + +int qemuProcessSetupVcpu(virDomainObjPtr vm, + unsigned int vcpuid); + #endif /* __QEMU_PROCESS_H__ */ -- 2.6.2

Since majority of the steps is shared, the function can be reused to simplify code. Additionally this resolves https://bugzilla.redhat.com/show_bug.cgi?id=1244128 since the cpu bandwidth limiting with cgroups would not be set on the hotplug path. --- src/qemu/qemu_driver.c | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34e82c2..71a35e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4657,10 +4657,6 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, int ret = -1; int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); - virCgroupPtr cgroup_vcpu = NULL; - char *mem_mask = NULL; - virDomainNumatuneMemMode mem_mode; - pid_t vcpupid; if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu))) return -1; @@ -4688,41 +4684,12 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, if (qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; - vcpupid = qemuDomainGetVcpuPid(vm, vcpu); - - if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && - mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virDomainNumatuneMaybeFormatNodeset(vm->def->numa, - priv->autoNodeset, - &mem_mask, -1) < 0) - goto cleanup; - - if (priv->cgroup) { - cgroup_vcpu = qemuDomainAddCgroupForThread(priv->cgroup, - VIR_CGROUP_THREAD_VCPU, - vcpu, mem_mask, vcpupid); - if (!cgroup_vcpu) - goto cleanup; - } - - /* Inherit def->cpuset */ - if (vm->def->cpumask) { - if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, vcpupid, - cgroup_vcpu) < 0) { - goto cleanup; - } - } - - if (vcpuinfo->sched.policy != VIR_PROC_POLICY_NONE && - virProcessSetScheduler(vcpupid, vcpuinfo->sched.policy, - vcpuinfo->sched.priority) < 0) + if (qemuProcessSetupVcpu(vm, vcpu) < 0) goto cleanup; ret = 0; cleanup: - VIR_FREE(mem_mask); - virCgroupFree(&cgroup_vcpu); return ret; } -- 2.6.2

Rather than iterating 3 times for various settings this function aggregates all the code into single place. One of the other advantages is that it can then be reused for properly setting IOThread info on hotplug. --- src/qemu/qemu_cgroup.c | 93 ----------------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_process.c | 154 ++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 2 + 4 files changed, 105 insertions(+), 145 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 827401e..eff059c 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1071,99 +1071,6 @@ qemuSetupCgroupForEmulator(virDomainObjPtr vm) return -1; } -int -qemuSetupCgroupForIOThreads(virDomainObjPtr vm) -{ - virCgroupPtr cgroup_iothread = NULL; - qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainDefPtr def = vm->def; - size_t i; - unsigned long long period = vm->def->cputune.period; - long long quota = vm->def->cputune.quota; - char *mem_mask = NULL; - virDomainNumatuneMemMode mem_mode; - - if (def->niothreadids == 0) - return 0; - - if ((period || quota) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cgroup cpu is required for scheduler tuning")); - return -1; - } - - /* - * If CPU cgroup controller is not initialized here, then we need - * neither period nor quota settings. And if CPUSET controller is - * not initialized either, then there's nothing to do anyway. - */ - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) - return 0; - - if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && - mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virDomainNumatuneMaybeFormatNodeset(vm->def->numa, - priv->autoNodeset, - &mem_mask, -1) < 0) - goto cleanup; - - for (i = 0; i < def->niothreadids; i++) { - /* IOThreads are numbered 1..n, although the array is 0..n-1, - * so we will account for that here - */ - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, - def->iothreadids[i]->iothread_id, - true, &cgroup_iothread) < 0) - goto cleanup; - - if (period || quota) { - if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0) - goto cleanup; - } - - /* Set iothreadpin in cgroup if iothreadpin xml is provided */ - if (virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_CPUSET)) { - virBitmapPtr cpumask = NULL; - - if (mem_mask && - virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0) - goto cleanup; - - if (def->iothreadids[i]->cpumask) - cpumask = def->iothreadids[i]->cpumask; - else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - cpumask = priv->autoCpuset; - else - cpumask = def->cpumask; - - if (cpumask && - qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) - goto cleanup; - } - - /* move the thread for iothread to sub dir */ - if (virCgroupAddTask(cgroup_iothread, - def->iothreadids[i]->thread_id) < 0) - goto cleanup; - - virCgroupFree(&cgroup_iothread); - } - VIR_FREE(mem_mask); - - return 0; - - cleanup: - if (cgroup_iothread) { - virCgroupRemove(cgroup_iothread); - virCgroupFree(&cgroup_iothread); - } - VIR_FREE(mem_mask); - - return -1; -} int qemuRemoveCgroup(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index fa3353a..a9718b5 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -53,7 +53,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, unsigned long long period, long long quota); int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); -int qemuSetupCgroupForIOThreads(virDomainObjPtr vm); int qemuSetupCgroupForEmulator(virDomainObjPtr vm); int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm); int qemuAddToCgroup(virDomainObjPtr vm); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2a783e5..f5a806b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2178,47 +2178,6 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) return ret; } -/* Set CPU affinities for IOThreads threads. */ -static int -qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) -{ - virDomainDefPtr def = vm->def; - size_t i; - int ret = -1; - - for (i = 0; i < def->niothreadids; i++) { - /* set affinity only for existing iothreads */ - if (!def->iothreadids[i]->cpumask) - continue; - - if (virProcessSetAffinity(def->iothreadids[i]->thread_id, - def->iothreadids[i]->cpumask) < 0) - goto cleanup; - } - ret = 0; - - cleanup: - return ret; -} - -static int -qemuProcessSetSchedulers(virDomainObjPtr vm) -{ - size_t i = 0; - - for (i = 0; i < vm->def->niothreadids; i++) { - virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i]; - - if (info->sched.policy == VIR_PROC_POLICY_NONE) - continue; - - if (virProcessSetScheduler(info->thread_id, info->sched.policy, - info->sched.priority) < 0) - return -1; - } - - return 0; -} static int qemuProcessInitPasswords(virConnectPtr conn, @@ -4498,6 +4457,107 @@ qemuProcessSetupVcpus(virDomainObjPtr vm) } +int +qemuProcessSetupIOThread(virDomainObjPtr vm, + virDomainIOThreadIDDefPtr iothread) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + unsigned long long period = vm->def->cputune.period; + long long quota = vm->def->cputune.quota; + virDomainNumatuneMemMode mem_mode; + char *mem_mask = NULL; + virCgroupPtr cgroup_iothread = NULL; + virBitmapPtr cpumask = NULL; + int ret = -1; + + if ((period || quota) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cgroup cpu is required for scheduler tuning")); + return -1; + } + + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) || + virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && + mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, + priv->autoNodeset, + &mem_mask, -1) < 0) + goto cleanup; + + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, + iothread->iothread_id, + true, &cgroup_iothread) < 0) + goto cleanup; + } + + if (iothread->cpumask) + cpumask = iothread->cpumask; + else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + cpumask = priv->autoCpuset; + else + cpumask = vm->def->cpumask; + + if (period || quota) { + if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0) + goto cleanup; + } + + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + if (mem_mask && + virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0) + goto cleanup; + + if (cpumask && + qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) + goto cleanup; + } + + if (cpumask) { + if (virProcessSetAffinity(iothread->thread_id, cpumask) < 0) + goto cleanup; + } + + if (cgroup_iothread && + virCgroupAddTask(cgroup_iothread, iothread->thread_id) < 0) + goto cleanup; + + if (iothread->sched.policy != VIR_PROC_POLICY_NONE && + virProcessSetScheduler(iothread->thread_id, iothread->sched.policy, + iothread->sched.priority) < 0) + goto cleanup; + + ret = 0; + + cleanup: + if (cgroup_iothread) { + if (ret < 0) + virCgroupRemove(cgroup_iothread); + virCgroupFree(&cgroup_iothread); + } + + VIR_FREE(mem_mask); + return ret; +} + + +static int +qemuProcessSetupIOThreads(virDomainObjPtr vm) +{ + size_t i; + + for (i = 0; i < vm->def->niothreadids; i++) { + virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i]; + + if (qemuProcessSetupIOThread(vm, info) < 0) + return -1; + } + + return 0; +} + + /** * qemuProcessLaunch: * @@ -4973,16 +5033,8 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessSetupVcpus(vm) < 0) goto cleanup; - VIR_DEBUG("Setting cgroup for each IOThread (if required)"); - if (qemuSetupCgroupForIOThreads(vm) < 0) - goto cleanup; - - VIR_DEBUG("Setting affinity of IOThread threads"); - if (qemuProcessSetIOThreadsAffinity(vm) < 0) - goto cleanup; - - VIR_DEBUG("Setting scheduler parameters"); - if (qemuProcessSetSchedulers(vm) < 0) + VIR_DEBUG("Setting IOThread tuning/settings"); + if (qemuProcessSetupIOThreads(vm) < 0) goto cleanup; VIR_DEBUG("Setting any required VM passwords"); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index a2663a0..ff7a722 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -158,5 +158,7 @@ int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm); int qemuProcessSetupVcpu(virDomainObjPtr vm, unsigned int vcpuid); +int qemuProcessSetupIOThread(virDomainObjPtr vm, + virDomainIOThreadIDDefPtr iothread); #endif /* __QEMU_PROCESS_H__ */ -- 2.6.2

Since majority of the steps is shared, the function can be reused to simplify code. Similarly to previous path doing this same for vCPUs this also fixes the a similar bug (which is not tracked). --- src/qemu/qemu_driver.c | 101 +------------------------------------------------ 1 file changed, 2 insertions(+), 99 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 71a35e4..f996ede 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4563,70 +4563,6 @@ static void qemuProcessEventHandler(void *data, void *opaque) VIR_FREE(processEvent); } -static virCgroupPtr -qemuDomainAddCgroupForThread(virCgroupPtr cgroup, - virCgroupThreadName nameval, - int idx, - char *mem_mask, - pid_t pid) -{ - virCgroupPtr new_cgroup = NULL; - int rv = -1; - - /* Create cgroup */ - if (virCgroupNewThread(cgroup, nameval, idx, true, &new_cgroup) < 0) - return NULL; - - if (mem_mask && - virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET) && - virCgroupSetCpusetMems(new_cgroup, mem_mask) < 0) - goto error; - - /* Add pid/thread to the cgroup */ - rv = virCgroupAddTask(new_cgroup, pid); - if (rv < 0) { - virCgroupRemove(new_cgroup); - goto error; - } - - return new_cgroup; - - error: - virCgroupFree(&new_cgroup); - return NULL; -} - - -static int -qemuDomainHotplugPinThread(virBitmapPtr cpumask, - int idx, - pid_t pid, - virCgroupPtr cgroup) -{ - int ret = -1; - - if (cgroup && - virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - if (qemuSetupCgroupCpusetCpus(cgroup, cpumask) < 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("failed to set cpuset.cpus in cgroup for id %d"), - idx); - goto cleanup; - } - } else { - if (virProcessSetAffinity(pid, cpumask) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("failed to set cpu affinity for id %d"), - idx); - goto cleanup; - } - } - - ret = 0; - - cleanup: - return ret; -} static int qemuDomainDelCgroupForThread(virCgroupPtr cgroup, @@ -5836,11 +5772,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, unsigned int exp_niothreads = vm->def->niothreadids; int new_niothreads = 0; qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; - virCgroupPtr cgroup_iothread = NULL; - char *mem_mask = NULL; - virDomainNumatuneMemMode mode; virDomainIOThreadIDDefPtr iothrid; - virBitmapPtr cpumask; if (virDomainIOThreadIDFind(vm->def, iothread_id)) { virReportError(VIR_ERR_INVALID_ARG, @@ -5880,14 +5812,6 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, } vm->def->iothreads = exp_niothreads; - if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) == 0 && - mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virDomainNumatuneMaybeFormatNodeset(vm->def->numa, - priv->autoNodeset, - &mem_mask, -1) < 0) - goto cleanup; - - /* * If we've successfully added an IOThread, find out where we added it * in the QEMU IOThread list, so we can add it to our iothreadids list @@ -5909,27 +5833,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, iothrid->thread_id = new_iothreads[idx]->thread_id; - /* Add IOThread to cgroup if present */ - if (priv->cgroup) { - cgroup_iothread = - qemuDomainAddCgroupForThread(priv->cgroup, - VIR_CGROUP_THREAD_IOTHREAD, - iothread_id, mem_mask, - iothrid->thread_id); - if (!cgroup_iothread) - goto cleanup; - } - - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - cpumask = priv->autoCpuset; - else - cpumask = vm->def->cpumask; - - if (cpumask) { - if (qemuDomainHotplugPinThread(cpumask, iothread_id, - iothrid->thread_id, cgroup_iothread) < 0) - goto cleanup; - } + if (qemuProcessSetupIOThread(vm, iothrid) < 0) + goto cleanup; ret = 0; @@ -5939,10 +5844,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, VIR_FREE(new_iothreads[idx]); VIR_FREE(new_iothreads); } - VIR_FREE(mem_mask); virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, "update", rc == 0); - virCgroupFree(&cgroup_iothread); VIR_FREE(alias); return ret; -- 2.6.2