Use 'ret' for return variable name, clarify use of 'param_idx' and avoid unnecessary 'success' label. No functional changes. Also document the function. --- Notes: V2: - Fixed mistake in the refactor that would return incorrect error values (Found by Jan) - Added comment explaining the terriblenes of the function (Requested by John) src/util/vircgroup.c | 64 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 9b56e27..da0df7a 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3189,6 +3189,26 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, } +/** + * virCgroupGetPercpuStats: + * @cgroup: cgroup data structure + * @params: typed parameter array where data is returned + * @nparams: cardinality of @params + * @start_cpu: offset of physical CPU to get data for + * @ncpus: number of physical CPUs to get data for + * @nvcpupids: number of vCPU threads for a domain (actual number of vcpus) + * + * This function is the worker that retrieves data in the appropriate format + * for the terribly designed 'virDomainGetCPUStats' API. Sharing semantics with + * the API, this function has two modes of operation depending on magic settings + * of the input arguments. Please refer to docs of 'virDomainGetCPUStats' for + * the usage patterns of the similarly named arguments. + * + * @nvcpupids determines the count of active vcpu threads for the vm. If the + * threads could not be detected the percpu data is skipped. + * + * Please DON'T use this function anywhere else. + */ int virCgroupGetPercpuStats(virCgroupPtr group, virTypedParameterPtr params, @@ -3197,7 +3217,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, unsigned int ncpus, unsigned int nvcpupids) { - int rv = -1; + int ret = -1; size_t i; int need_cpus, total_cpus; char *pos; @@ -3218,12 +3238,13 @@ virCgroupGetPercpuStats(virCgroupPtr group, /* To parse account file, we need to know how many cpus are present. */ if (!(cpumap = nodeGetPresentCPUBitmap(NULL))) - return rv; + return -1; total_cpus = virBitmapSize(cpumap); + /* return total number of cpus */ if (ncpus == 0) { - rv = total_cpus; + ret = total_cpus; goto cleanup; } @@ -3261,34 +3282,35 @@ virCgroupGetPercpuStats(virCgroupPtr group, goto cleanup; } - if (nvcpupids == 0 || param_idx + 1 >= nparams) - goto success; /* return percpu vcputime in index 1 */ - param_idx++; + param_idx = 1; - if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0) - goto cleanup; - if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, need_cpus, - cpumap) < 0) - goto cleanup; - - for (i = start_cpu; i < need_cpus; i++) { - if (virTypedParameterAssign(&params[(i - start_cpu) * nparams + - param_idx], - VIR_DOMAIN_CPU_STATS_VCPUTIME, - VIR_TYPED_PARAM_ULLONG, - sum_cpu_time[i]) < 0) + if (nvcpupids > 0 && 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) + goto cleanup; + + for (i = start_cpu; i < need_cpus; i++) { + if (virTypedParameterAssign(&params[(i - start_cpu) * nparams + + param_idx], + VIR_DOMAIN_CPU_STATS_VCPUTIME, + VIR_TYPED_PARAM_ULLONG, + sum_cpu_time[i]) < 0) + goto cleanup; + } + + param_idx++; } - success: - rv = param_idx + 1; + ret = param_idx; cleanup: virBitmapFree(cpumap); VIR_FREE(sum_cpu_time); VIR_FREE(buf); - return rv; + return ret; } -- 2.6.2

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. --- Notes: v2: - renamed 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 1ea74a6..ead76a1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1374,6 +1374,30 @@ virDomainDefGetVcpus(const virDomainDef *def) } +/** + * virDomainDefGetOnlineVcpumap: + * @def: domain definition + * + * Returns a bitmap representing state of individual vcpus. + */ +virBitmapPtr +virDomainDefGetOnlineVcpumap(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..9fdfdf2 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 virDomainDefGetOnlineVcpumap(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 3d0ec9d..1083782 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -217,6 +217,7 @@ virDomainDefFree; virDomainDefGetDefaultEmulator; virDomainDefGetMemoryActual; virDomainDefGetMemoryInitial; +virDomainDefGetOnlineVcpumap; virDomainDefGetSecurityLabelDef; virDomainDefGetVcpu; virDomainDefGetVcpus; -- 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 abcdbe6..0fa7d13 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18173,6 +18173,7 @@ qemuDomainGetCPUStats(virDomainPtr domain, virDomainObjPtr vm = NULL; int ret = -1; qemuDomainObjPrivatePtr priv; + virBitmapPtr guestvcpus = NULL; virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); @@ -18196,13 +18197,17 @@ qemuDomainGetCPUStats(virDomainPtr domain, goto cleanup; } + if (!(guestvcpus = virDomainDefGetOnlineVcpumap(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 da0df7a..37706c0 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3143,17 +3143,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; @@ -3215,7 +3215,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, unsigned int nparams, int start_cpu, unsigned int ncpus, - unsigned int nvcpupids) + virBitmapPtr guestvcpus) { int ret = -1; size_t i; @@ -3230,7 +3230,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; @@ -3285,11 +3285,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 d754b1f..4a87b39 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 0fa7d13..9dce1b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4761,6 +4761,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, unsigned int vcpu) { qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainVcpuInfoPtr vcpuinfo; int ret = -1; int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); @@ -4770,6 +4771,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); @@ -4785,7 +4795,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, if (rc < 0) goto cleanup; - ignore_value(virDomainDefSetVcpus(vm->def, oldvcpus + 1)); + vcpuinfo->online = true; if (ncpupids < 0) goto cleanup; @@ -4861,12 +4871,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); @@ -4890,7 +4910,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. --- Notes: v2: - fixed two typos 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 1df1b74..5ddf048 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4280,3 +4280,87 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm, return priv->vcpupids[vcpu]; } + + +/** + * qemuDomainDetectVcpuPids: + * @driver: qemu driver data + * @vm: domain object + * @asyncJob: current asynchronous job type + * + * Updates vCPU thread 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 ee94d3f..7b09ba7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1992,78 +1992,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, @@ -5020,7 +4948,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"); @@ -5727,7 +5655,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. The function now returns the number of detected VCPUs. --- Notes: v2: - fix comment spacing - fix leak of cpupids on monitor exit failure - change return value v2: - fix comment spacing - fix leak of cpupids on monitor exit failure src/qemu/qemu_domain.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5ddf048..1895520 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4290,7 +4290,8 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm, * * Updates vCPU thread ids in the private data of @vm. * - * Returns 0 on success -1 on error and reports an appropriate error. + * Returns number of detected vCPU threads on success, -1 on error and reports + * an appropriate error. */ int qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, @@ -4298,7 +4299,7 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, int asyncJob) { pid_t *cpupids = NULL; - int ncpupids; + int ncpupids = 0; qemuDomainObjPrivatePtr priv = vm->privateData; /* @@ -4329,26 +4330,23 @@ 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; ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids); - if (qemuDomainObjExitMonitor(driver, vm) < 0) + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + VIR_FREE(cpupids); return -1; - /* failure to get the VCPU<-> PID mapping or to execute the query + } + /* 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; + ncpupids = 0; + goto done; } if (ncpupids != virDomainDefGetVcpus(vm->def)) { @@ -4360,7 +4358,9 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, return -1; } + done: + VIR_FREE(priv->vcpupids); priv->nvcpupids = ncpupids; priv->vcpupids = cpupids; - return 0; + return ncpupids; } -- 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. --- Notes: v2: fix bugs regarding error codes from redetection src/qemu/qemu_driver.c | 86 +++++++++++++++++--------------------------------- 1 file changed, 29 insertions(+), 57 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9dce1b2..b8a60d8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4765,11 +4765,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; @@ -4784,9 +4783,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; @@ -4797,23 +4793,15 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, vcpuinfo->online = true; - if (ncpupids < 0) - goto cleanup; + if ((rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) { + /* vcpu pids were not detected, skip setting of affinity */ + if (rc == 0) + ret = 0; - /* 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; 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 && @@ -4823,11 +4811,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; } @@ -4839,26 +4825,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; @@ -4875,8 +4855,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; @@ -4887,49 +4865,43 @@ 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); + + /* Free vcpupin setting */ + virDomainPinDel(&vm->def->cputune.vcpupin, + &vm->def->cputune.nvcpupin, + vcpu); + + if (rc <= 0) { + if (rc == 0) + ret = 0; - /* check if hotunplug has failed */ - if (ncpupids != oldvcpus - 1) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("qemu didn't unplug vCPU '%u' properly"), vcpu); goto cleanup; } - vcpuinfo->online = false; - if (qemuDomainDelCgroupForThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu) < 0) goto cleanup; - /* Free vcpupin setting */ - virDomainPinDel(&vm->def->cputune.vcpupin, - &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. --- Notes: v2: use correct variable 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 | 12 ++++++++---- src/test/test_driver.c | 2 ++ src/vz/vz_sdk.c | 4 ++-- 7 files changed, 28 insertions(+), 73 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ead76a1..e06ec80 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15165,34 +15165,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")); @@ -21832,10 +21804,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 c74c55c..b098d3d 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -829,9 +829,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 4a9134e..6ab4f07 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2472,6 +2472,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 b8a60d8..c9d6d00 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4677,33 +4677,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, @@ -4820,11 +4793,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; @@ -5349,6 +5317,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 7b09ba7..5ede367 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2207,7 +2207,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")); @@ -2218,16 +2219,19 @@ 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 (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, n), - pininfo->cpumask) < 0) { + if (!(bitmap = pininfo->cpumask) && + !(bitmap = def->cpumask)) + continue; + + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, n), bitmap) < 0) goto cleanup; - } } ret = 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 37108ab..5986749 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2575,6 +2575,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

Add a helper function to do the checking. The check is used when determining whether the <cputune> element should be formatted. --- Notes: v2: - renamed func - used different variable to decide (not that it would matter) 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 e06ec80..3dc5e00 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1413,6 +1413,19 @@ virDomainDefGetVcpu(virDomainDefPtr def, } +/** + * virDomainDefHasVcpuPin: + * @def: domain definition + * + * This helper returns true if any of the domain's vcpus has cpu pinning set + */ +static bool +virDomainDefHasVcpuPin(const virDomainDef *def) +{ + return !!def->cputune.vcpupin; +} + + virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) { @@ -15319,7 +15332,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (virDomainNumatuneHasPlacementAuto(def->numa) && - !def->cpumask && !def->cputune.vcpupin && + !def->cpumask && !virDomainDefHasVcpuPin(def) && !def->cputune.emulatorpin && !virDomainIOThreadIDArrayHasPin(def)) def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO; -- 2.6.2

Now with the new struct the data can be stored in a much saner place. --- Notes: v2: - clear bitmap pointer after free to avoid use after free src/conf/domain_conf.c | 136 ++++++++++++++++++-------------------------- 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 | 143 ++++++++++++++++++++++------------------------- src/qemu/qemu_process.c | 36 ++++++------ src/test/test_driver.c | 43 ++++++-------- 8 files changed, 186 insertions(+), 246 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3dc5e00..c4b735e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1289,6 +1289,9 @@ virDomainVcpuInfoClear(virDomainVcpuInfoPtr info) { if (!info) return; + + virBitmapFree(info->cpumask); + info->cpumask = NULL; } @@ -1422,7 +1425,14 @@ virDomainDefGetVcpu(virDomainDefPtr def, static bool virDomainDefHasVcpuPin(const virDomainDef *def) { - return !!def->cputune.vcpupin; + size_t i; + + for (i = 0; i < def->maxvcpus; i++) { + if (def->vcpus[i].cpumask) + return true; + } + + return false; } @@ -2593,8 +2603,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,83 +14145,68 @@ 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; - if (virBitmapIsAllClear(def->cpumask)) { + if (virBitmapIsAllClear(vcpu->cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'cpuset': %s"), tmp); - goto error; + goto cleanup; } + ret = 0; + cleanup: VIR_FREE(tmp); - ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; } @@ -15147,34 +15140,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); @@ -21815,15 +21783,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 9fdfdf2..f33c575 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 b098d3d..19d5657 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -821,7 +821,7 @@ int libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); - virDomainPinDefPtr pin; + virDomainVcpuInfoPtr vcpu; libxl_bitmap map; virBitmapPtr cpumask = NULL; size_t i; @@ -830,13 +830,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) @@ -845,9 +844,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 6ab4f07..0b5481b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2329,6 +2329,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainDefPtr targetDef = NULL; virBitmapPtr pcpumap = NULL; + virDomainVcpuInfoPtr vcpuinfo; virDomainObjPtr vm; int ret = -1; @@ -2360,10 +2361,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) { @@ -2374,20 +2381,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; @@ -2463,15 +2459,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 e41f461..8dff76c 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 && qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9d6d00..54600dc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4847,11 +4847,6 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", rc >= 0); - /* Free vcpupin setting */ - virDomainPinDel(&vm->def->cputune.vcpupin, - &vm->def->cputune.nvcpupin, - vcpu); - if (rc <= 0) { if (rc == 0) ret = 0; @@ -4863,6 +4858,9 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, VIR_CGROUP_THREAD_VCPU, vcpu) < 0) goto cleanup; + virBitmapFree(vcpuinfo->cpumask); + vcpuinfo->cpumask = NULL; + ret = 0; cleanup: @@ -5026,10 +5024,15 @@ 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); + vcpu->cpumask = NULL; + } + } } if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { @@ -5088,9 +5091,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] = ""; @@ -5118,18 +5123,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))) @@ -5141,6 +5164,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, @@ -5148,26 +5175,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, @@ -5189,13 +5196,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; @@ -5214,24 +5217,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; @@ -5240,14 +5231,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; } @@ -5272,7 +5263,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; @@ -5304,16 +5296,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; @@ -5322,7 +5313,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 5ede367..4b94a89 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2193,23 +2193,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; @@ -2218,19 +2218,19 @@ 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), bitmap) < 0) + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, i), bitmap) < 0) goto cleanup; } 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

Now that the pinning info is stored elsewhere we can delete all the obsolete code. --- src/conf/domain_conf.c | 136 ----------------------------------------------- src/conf/domain_conf.h | 27 ---------- src/libvirt_private.syms | 6 --- 3 files changed, 169 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c4b735e..e289b6f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2273,39 +2273,6 @@ virDomainClockDefClear(virDomainClockDefPtr def) VIR_FREE(def->timers); } -virDomainPinDefPtr * -virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) -{ - size_t i; - virDomainPinDefPtr *ret = NULL; - - if (VIR_ALLOC_N(ret, npin) < 0) - goto error; - - for (i = 0; i < npin; i++) { - if (VIR_ALLOC(ret[i]) < 0) - goto error; - ret[i]->id = src[i]->id; - if ((ret[i]->cpumask = virBitmapNewCopy(src[i]->cpumask)) == NULL) - goto error; - } - - return ret; - - error: - if (ret) { - for (i = 0; i < npin; i++) { - if (ret[i]) { - virBitmapFree(ret[i]->cpumask); - VIR_FREE(ret[i]); - } - } - VIR_FREE(ret); - } - - return NULL; -} - static bool virDomainIOThreadIDArrayHasPin(virDomainDefPtr def) @@ -2400,31 +2367,6 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) void -virDomainPinDefFree(virDomainPinDefPtr def) -{ - if (def) { - virBitmapFree(def->cpumask); - VIR_FREE(def); - } -} - -void -virDomainPinDefArrayFree(virDomainPinDefPtr *def, - int npin) -{ - size_t i; - - if (!def) - return; - - for (i = 0; i < npin; i++) - virDomainPinDefFree(def[i]); - - VIR_FREE(def); -} - - -void virDomainResourceDefFree(virDomainResourceDefPtr resource) { if (!resource) @@ -18397,84 +18339,6 @@ virDomainIOThreadSchedDelId(virDomainDefPtr def, } } -virDomainPinDefPtr -virDomainPinFind(virDomainPinDefPtr *def, - int npin, - int id) -{ - size_t i; - - if (!def || !npin) - return NULL; - - for (i = 0; i < npin; i++) { - if (def[i]->id == id) - return def[i]; - } - - return NULL; -} - -int -virDomainPinAdd(virDomainPinDefPtr **pindef_list, - size_t *npin, - unsigned char *cpumap, - int maplen, - int id) -{ - virDomainPinDefPtr pindef = NULL; - - if (!pindef_list) - return -1; - - pindef = virDomainPinFind(*pindef_list, *npin, id); - if (pindef) { - pindef->id = id; - virBitmapFree(pindef->cpumask); - pindef->cpumask = virBitmapNewData(cpumap, maplen); - if (!pindef->cpumask) - return -1; - - return 0; - } - - /* No existing pindef matches id, adding a new one */ - - if (VIR_ALLOC(pindef) < 0) - goto error; - - pindef->id = id; - pindef->cpumask = virBitmapNewData(cpumap, maplen); - if (!pindef->cpumask) - goto error; - - if (VIR_APPEND_ELEMENT(*pindef_list, *npin, pindef) < 0) - goto error; - - return 0; - - error: - virDomainPinDefFree(pindef); - return -1; -} - -void -virDomainPinDel(virDomainPinDefPtr **pindef_list, - size_t *npin, - int id) -{ - int n; - - for (n = 0; n < *npin; n++) { - if ((*pindef_list)[n]->id == id) { - virBitmapFree((*pindef_list)[n]->cpumask); - VIR_FREE((*pindef_list)[n]); - VIR_DELETE_ELEMENT(*pindef_list, n, *npin); - return; - } - } -} - static int virDomainEventActionDefFormat(virBufferPtr buf, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f33c575..6a5f615 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2100,33 +2100,6 @@ struct _virDomainIOThreadIDDef { void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); -typedef struct _virDomainPinDef virDomainPinDef; -typedef virDomainPinDef *virDomainPinDefPtr; -struct _virDomainPinDef { - int id; - virBitmapPtr cpumask; -}; - -void virDomainPinDefFree(virDomainPinDefPtr def); -void virDomainPinDefArrayFree(virDomainPinDefPtr *def, int npin); - -virDomainPinDefPtr *virDomainPinDefCopy(virDomainPinDefPtr *src, - int npin); - -virDomainPinDefPtr virDomainPinFind(virDomainPinDefPtr *def, - int npin, - int id); - -int virDomainPinAdd(virDomainPinDefPtr **pindef_list, - size_t *npin, - unsigned char *cpumap, - int maplen, - int id); - -void virDomainPinDel(virDomainPinDefPtr **pindef_list, - size_t *npin, - int vcpu); - typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1083782..3806238 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -418,12 +418,6 @@ virDomainOSTypeToString; virDomainParseMemory; virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; -virDomainPinAdd; -virDomainPinDefArrayFree; -virDomainPinDefCopy; -virDomainPinDefFree; -virDomainPinDel; -virDomainPinFind; virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; -- 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 e289b6f..0c28c03 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21414,6 +21414,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. @@ -21624,111 +21747,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

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 3806238..7e9102a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1155,6 +1155,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

--- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 33 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0c28c03..556cd71 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18292,6 +18292,35 @@ virDomainIOThreadIDAdd(virDomainDefPtr def, return NULL; } + +/* + * virDomainIOThreadIDMap: + * @def: domain definition + * + * Returns a map of active iothreads for @def. + */ +virBitmapPtr +virDomainIOThreadIDMap(virDomainDefPtr def) +{ + unsigned int max = 0; + size_t i; + virBitmapPtr ret = NULL; + + for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->iothread_id > max) + max = def->iothreadids[i]->iothread_id; + } + + if (!(ret = virBitmapNew(max))) + return NULL; + + for (i = 0; i < def->niothreadids; i++) + ignore_value(virBitmapSetBit(ret, def->iothreadids[i]->iothread_id)); + + return ret; +} + + void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6a5f615..fe8334c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2700,6 +2700,9 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id); virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id); + +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); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7e9102a..af36a37 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -344,6 +344,7 @@ virDomainIOThreadIDAdd; virDomainIOThreadIDDefFree; virDomainIOThreadIDDel; virDomainIOThreadIDFind; +virDomainIOThreadIDMap; virDomainIOThreadSchedDelId; virDomainKeyWrapCipherNameTypeFromString; virDomainKeyWrapCipherNameTypeToString; -- 2.6.2

As the scheduler info elements are represented orthogonally to how it makes sense to actually store the information, the extracted code will be later used when converting between XML and internal definitions. --- Notes: v2: tweaked spelling in error message src/conf/domain_conf.c | 69 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 556cd71..1ee9041 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14522,36 +14522,34 @@ virDomainLoaderDefParseXML(xmlNodePtr node, return ret; } -static int -virDomainThreadSchedParse(xmlNodePtr node, - unsigned int minid, - unsigned int maxid, - const char *name, - virDomainThreadSchedParamPtr sp) + +static virBitmapPtr +virDomainSchedulerParse(xmlNodePtr node, + const char *name, + virProcessSchedPolicy *policy, + int *priority) { + virBitmapPtr ret = NULL; char *tmp = NULL; int pol = 0; - tmp = virXMLPropString(node, name); - if (!tmp) { + if (!(tmp = virXMLPropString(node, name))) { virReportError(VIR_ERR_XML_ERROR, _("Missing attribute '%s' in element '%sched'"), name, name); goto error; } - if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0) + if (virBitmapParse(tmp, 0, &ret, VIR_DOMAIN_CPUMASK_LEN) < 0) goto error; - if (virBitmapIsAllClear(sp->ids) || - virBitmapNextSetBit(sp->ids, -1) < minid || - virBitmapLastSetBit(sp->ids) > maxid) { - + if (virBitmapIsAllClear(ret)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid value of '%s': %s"), + _("'%s' scheduler bitmap '%s' is empty"), name, tmp); goto error; } + VIR_FREE(tmp); if (!(tmp = virXMLPropString(node, "scheduler"))) { @@ -14562,22 +14560,22 @@ virDomainThreadSchedParse(xmlNodePtr node, if ((pol = virProcessSchedPolicyTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid scheduler attribute: '%s'"), - tmp); + _("Invalid scheduler attribute: '%s'"), tmp); goto error; } - sp->policy = pol; + *policy = pol; VIR_FREE(tmp); - if (sp->policy == VIR_PROC_POLICY_FIFO || - sp->policy == VIR_PROC_POLICY_RR) { - tmp = virXMLPropString(node, "priority"); - if (!tmp) { + + if (pol == VIR_PROC_POLICY_FIFO || + pol == VIR_PROC_POLICY_RR) { + if (!(tmp = virXMLPropString(node, "priority"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing scheduler priority")); goto error; } - if (virStrToLong_i(tmp, NULL, 10, &sp->priority) < 0) { + + if (virStrToLong_i(tmp, NULL, 10, priority) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid value for element priority")); goto error; @@ -14585,11 +14583,34 @@ virDomainThreadSchedParse(xmlNodePtr node, VIR_FREE(tmp); } - return 0; + return ret; error: VIR_FREE(tmp); - return -1; + virBitmapFree(ret); + return NULL; +} + + +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; + + if (virBitmapNextSetBit(sp->ids, -1) < minid || + virBitmapLastSetBit(sp->ids) > maxid) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%sched bitmap is out of range"), name); + return -1; + } + + return 0; } -- 2.6.2

Due to bad design the vcpu sched element is orthogonal to the way how the data belongs to the corresponding objects. Now that vcpus are a struct that allow to store other info too, let's convert the data to the sane structure. The helpers for the conversion are made universal so that they can be reused for iothreads too. This patch also resolves https://bugzilla.redhat.com/show_bug.cgi?id=1235180 since with the correct storage approach you can't have dangling data. --- src/conf/domain_conf.c | 246 +++++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 5 +- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_process.c | 8 +- 4 files changed, 214 insertions(+), 51 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1ee9041..1d76beb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1416,6 +1416,19 @@ virDomainDefGetVcpu(virDomainDefPtr def, } +static virDomainThreadSchedParamPtr +virDomainDefGetVcpuSched(virDomainDefPtr def, + unsigned int vcpu) +{ + virDomainVcpuInfoPtr vcpuinfo; + + if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu))) + return NULL; + + return &vcpuinfo->sched; +} + + /** * virDomainDefHasVcpuPin: * @def: domain definition @@ -2547,10 +2560,6 @@ void virDomainDefFree(virDomainDefPtr def) virBitmapFree(def->cputune.emulatorpin); - for (i = 0; i < def->cputune.nvcpusched; i++) - virBitmapFree(def->cputune.vcpusched[i].ids); - VIR_FREE(def->cputune.vcpusched); - for (i = 0; i < def->cputune.niothreadsched; i++) virBitmapFree(def->cputune.iothreadsched[i].ids); VIR_FREE(def->cputune.iothreadsched); @@ -14593,6 +14602,55 @@ virDomainSchedulerParse(xmlNodePtr node, static int +virDomainThreadSchedParseHelper(xmlNodePtr node, + const char *name, + virDomainThreadSchedParamPtr (*func)(virDomainDefPtr, unsigned int), + virDomainDefPtr def) +{ + ssize_t next = -1; + virBitmapPtr map = NULL; + virDomainThreadSchedParamPtr sched; + virProcessSchedPolicy policy; + int priority; + int ret = -1; + + if (!(map = virDomainSchedulerParse(node, name, &policy, &priority))) + goto cleanup; + + while ((next = virBitmapNextSetBit(map, next)) > -1) { + if (!(sched = func(def, next))) + goto cleanup; + + if (sched->policy != VIR_PROC_POLICY_NONE) { + virReportError(VIR_ERR_XML_DETAIL, + _("%ssched attributes 'vcpus' must not overlap"), + name); + goto cleanup; + } + + sched->policy = policy; + sched->priority = priority; + } + + ret = 0; + + cleanup: + virBitmapFree(map); + return ret; +} + + +static int +virDomainVcpuThreadSchedParse(xmlNodePtr node, + virDomainDefPtr def) +{ + return virDomainThreadSchedParseHelper(node, "vcpus", + virDomainDefGetVcpuSched, + def); +} + + +static int virDomainThreadSchedParse(xmlNodePtr node, unsigned int minid, unsigned int maxid, @@ -15146,29 +15204,10 @@ virDomainDefParseXML(xmlDocPtr xml, _("cannot extract vcpusched nodes")); goto error; } - if (n) { - if (VIR_ALLOC_N(def->cputune.vcpusched, n) < 0) - goto error; - def->cputune.nvcpusched = n; - - for (i = 0; i < def->cputune.nvcpusched; i++) { - if (virDomainThreadSchedParse(nodes[i], - 0, - virDomainDefGetVcpusMax(def) - 1, - "vcpus", - &def->cputune.vcpusched[i]) < 0) - goto error; - for (j = 0; j < i; j++) { - if (virBitmapOverlaps(def->cputune.vcpusched[i].ids, - def->cputune.vcpusched[j].ids)) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("vcpusched attributes 'vcpus' " - "must not overlap")); - goto error; - } - } - } + for (i = 0; i < n; i++) { + if (virDomainVcpuThreadSchedParse(nodes[i], def) < 0) + goto error; } VIR_FREE(nodes); @@ -21465,6 +21504,143 @@ virDomainDefHasCapabilitiesFeatures(virDomainDefPtr def) } +/** + * virDomainFormatSchedDef: + * @def: domain definiton + * @buf: target XML buffer + * @name: name of the target XML element + * @func: function that returns the thread scheduler parameter struct for an object + * @resourceMap: bitmap of indexes of objects that shall be formated (used with @func + * + * Formats one of the two scheduler tuning elements to the XML. This function + * transforms the internal representation where the scheduler info is stored + * per-object to the XML representation where the info is stored per group of + * objects. This function autogroups all the relevant scheduler configs. + * + * Returns 0 on success -1 on error. + */ +static int +virDomainFormatSchedDef(virDomainDefPtr def, + virBufferPtr buf, + const char *name, + virDomainThreadSchedParamPtr (*func)(virDomainDefPtr, unsigned int), + virBitmapPtr resourceMap) +{ + virBitmapPtr schedMap = NULL; + virBitmapPtr prioMap = NULL; + virDomainThreadSchedParamPtr sched; + char *tmp = NULL; + ssize_t next; + size_t i; + int ret = -1; + + if (!(schedMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)) || + !(prioMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) + goto cleanup; + + for (i = VIR_PROC_POLICY_NONE + 1; i < VIR_PROC_POLICY_LAST; i++) { + virBitmapClearAll(schedMap); + + /* find vcpus using a particular scheduler */ + next = -1; + while ((next = virBitmapNextSetBit(resourceMap, next)) > -1) { + sched = func(def, next); + + if (sched->policy == i) + ignore_value(virBitmapSetBit(schedMap, next)); + } + + /* it's necessary to discriminate priority levels for schedulers that + * have them */ + while (!virBitmapIsAllClear(schedMap)) { + virBitmapPtr currentMap = NULL; + ssize_t nextprio; + bool hasPriority = false; + int priority; + + switch ((virProcessSchedPolicy) i) { + case VIR_PROC_POLICY_NONE: + case VIR_PROC_POLICY_BATCH: + case VIR_PROC_POLICY_IDLE: + case VIR_PROC_POLICY_LAST: + currentMap = schedMap; + break; + + case VIR_PROC_POLICY_FIFO: + case VIR_PROC_POLICY_RR: + virBitmapClearAll(prioMap); + hasPriority = true; + + /* we need to find a subset of vCPUs with the given scheduler + * that share the priority */ + nextprio = virBitmapNextSetBit(schedMap, -1); + sched = func(def, nextprio); + priority = sched->priority; + + ignore_value(virBitmapSetBit(prioMap, nextprio)); + + while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { + sched = func(def, nextprio); + if (sched->priority == priority) + ignore_value(virBitmapSetBit(prioMap, nextprio)); + } + + currentMap = prioMap; + break; + } + + /* now we have the complete group */ + if (!(tmp = virBitmapFormat(currentMap))) + goto cleanup; + + virBufferAsprintf(buf, + "<%sched %s='%s' scheduler='%s'", + name, name, tmp, + virProcessSchedPolicyTypeToString(i)); + VIR_FREE(tmp); + + if (hasPriority) + virBufferAsprintf(buf, " priority='%d'", priority); + + virBufferAddLit(buf, "/>\n"); + + /* subtract all vCPUs that were already found */ + virBitmapSubtract(schedMap, currentMap); + } + } + + ret = 0; + + cleanup: + virBitmapFree(schedMap); + virBitmapFree(prioMap); + return ret; +} + + +static int +virDomainFormatVcpuSchedDef(virDomainDefPtr def, + virBufferPtr buf) +{ + virBitmapPtr allcpumap; + int ret; + + if (virDomainDefGetVcpusMax(def) == 0) + return 0; + + if (!(allcpumap = virBitmapNew(virDomainDefGetVcpusMax(def)))) + return -1; + + virBitmapSetAll(allcpumap); + + ret = virDomainFormatSchedDef(def, buf, "vcpus", virDomainDefGetVcpuSched, + allcpumap); + + virBitmapFree(allcpumap); + return ret; +} + + static int virDomainCputuneDefFormat(virBufferPtr buf, virDomainDefPtr def) @@ -21539,22 +21715,8 @@ virDomainCputuneDefFormat(virBufferPtr buf, 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"); - } + if (virDomainFormatVcpuSchedDef(def, &childrenBuf) < 0) + goto cleanup; for (i = 0; i < def->cputune.niothreadsched; i++) { virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i]; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fe8334c..b095456 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2112,8 +2112,6 @@ struct _virDomainCputune { long long emulator_quota; virBitmapPtr emulatorpin; - size_t nvcpusched; - virDomainThreadSchedParamPtr vcpusched; size_t niothreadsched; virDomainThreadSchedParamPtr iothreadsched; }; @@ -2125,6 +2123,9 @@ typedef virDomainVcpuInfo *virDomainVcpuInfoPtr; struct _virDomainVcpuInfo { bool online; virBitmapPtr cpumask; + + /* note: the sched.ids bitmap is unused so it doesn't have to be cleared */ + virDomainThreadSchedParam sched; }; typedef struct _virDomainBlkiotune virDomainBlkiotune; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 54600dc..f9c746a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4799,9 +4799,9 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, } } - if (qemuProcessSetSchedParams(vcpu, vcpupid, - vm->def->cputune.nvcpusched, - vm->def->cputune.vcpusched) < 0) + if (vcpuinfo->sched.policy != VIR_PROC_POLICY_NONE && + virProcessSetScheduler(vcpupid, vcpuinfo->sched.policy, + vcpuinfo->sched.priority) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4b94a89..6246b17 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2317,12 +2317,12 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, i); - if (!vcpu->online) + if (!vcpu->online || + vcpu->sched.policy == VIR_PROC_POLICY_NONE) continue; - if (qemuProcessSetSchedParams(i, qemuDomainGetVcpuPid(vm, i), - vm->def->cputune.nvcpusched, - vm->def->cputune.vcpusched) < 0) + if (virProcessSetScheduler(qemuDomainGetVcpuPid(vm, i), + vcpu->sched.policy, vcpu->sched.priority) < 0) return -1; } -- 2.6.2

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 1d76beb..8c372aa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2560,10 +2560,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); @@ -14650,25 +14646,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, - _("%sched 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); } @@ -15216,46 +15213,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); @@ -18405,29 +18366,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, @@ -21642,6 +21580,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, "iothreads", + virDomainDefGetIOThreadSched, allthreadmap); + + virBitmapFree(allthreadmap); + return ret; +} + + +static int virDomainCputuneDefFormat(virBufferPtr buf, virDomainDefPtr def) { @@ -21718,22 +21677,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 b095456..15ba6ab 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 doesn't have to be cleared */ virDomainThreadSchedParam sched; }; @@ -2705,7 +2702,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 af36a37..346fdf8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -345,7 +345,6 @@ virDomainIOThreadIDDefFree; virDomainIOThreadIDDel; virDomainIOThreadIDFind; virDomainIOThreadIDMap; -virDomainIOThreadSchedDelId; virDomainKeyWrapCipherNameTypeFromString; virDomainKeyWrapCipherNameTypeToString; virDomainLeaseDefFree; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f9c746a..a247dc0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6135,8 +6135,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) @@ -6226,7 +6224,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 6246b17..b1c2a43 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2281,34 +2281,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) { @@ -2327,10 +2299,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 5177114..4d665e9 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 | 95 --------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_process.c | 216 +++++++++++++++++++++++++++++++++--------------- src/qemu/qemu_process.h | 4 + 4 files changed, 152 insertions(+), 164 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 8dff76c..0a73477 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1001,101 +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 && 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 b1c2a43..1682a76 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2188,56 +2188,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 @@ -2286,18 +2236,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]; @@ -4464,6 +4402,152 @@ qemuProcessInit(virQEMUDriverPtr driver, /** + * qemuProcessSetupVcpu: + * @vm: domain object + * @vcpuid: id of VCPU to set defaults + * + * This function sets resource properities (affinity, cgroups, scheduler) for a + * vCPU. This function expects that the vCPU is online and the vCPU pids were + * correctly detected at the point when it's called. + * + * Returns 0 on success, -1 on error. + */ +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: * * Launch a new QEMU process with stopped virtual CPUs. @@ -4934,18 +5018,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 a247dc0..51e2e86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4738,10 +4738,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; @@ -4774,41 +4770,12 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, 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 | 165 +++++++++++++++++++++++++++++++++--------------- src/qemu/qemu_process.h | 2 + 4 files changed, 116 insertions(+), 145 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 0a73477..eb55d68 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1067,99 +1067,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 1682a76..f7ff572 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2208,47 +2208,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, @@ -4548,6 +4507,118 @@ qemuProcessSetupVcpus(virDomainObjPtr vm) /** + * qemuProcessSetupIOThread: + * @vm: domain object + * @iothread: iothread data structure to set the data for + * + * This function sets resource properities (affinity, cgroups, scheduler) for a + * IOThread. This function expects that the IOThread is online and the IOThread + * pids were correctly detected at the point when it's called. + * + * Returns 0 on success, -1 on error. + */ +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: * * Launch a new QEMU process with stopped virtual CPUs. @@ -5022,16 +5093,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 51e2e86..60518f9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4644,70 +4644,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, @@ -5928,11 +5864,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, @@ -5972,14 +5904,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 @@ -6001,27 +5925,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; @@ -6031,10 +5936,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