[libvirt] [PATCH 0/3] Clean up some duplicated code in qemuProcessSetup* functions
Initially posted in order to achieve something else, but that got rejected, so at least the cleanup part of it could make it in. The previous versions (whose goal was something completely idfferent) are here: - https://www.redhat.com/archives/libvir-list/2016-June/msg01537.html - https://www.redhat.com/archives/libvir-list/2016-July/msg00173.html Martin Kletzander (3): qemu: Add qemuProcessSetupPid() and use it in qemuProcessSetupIOThread() qemu: Use qemuProcessSetupPid() in qemuProcessSetupEmulator() qemu: Use qemuProcessSetupPid() in qemuProcessSetupVcpu() src/qemu/qemu_process.c | 283 ++++++++++++++++-------------------------------- 1 file changed, 92 insertions(+), 191 deletions(-) -- 2.9.0
Setting up cgroups and other things for all kinds of threads (the emulator thread, vCPU threads, I/O threads) was copy-pasted every time new thing was added. Over time each one of those functions changed a bit differently. So create one function that does all that setup and start using it, starting with I/O thread setup. That will shave some duplicated code and maybe fix some bugs as well. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 200 +++++++++++++++++++++++++++--------------------- 1 file changed, 114 insertions(+), 86 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 129c070d40d0..6fd6ee3d2d9a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2304,6 +2304,114 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, } +/** + * qemuProcessSetupPid: + * + * This function sets resource properities (affinity, cgroups, + * scheduler) for any PID associated with a domain. It should be used + * to set up emulator PIDs as well as vCPU and I/O thread pids to + * ensure they are all handled the same way. + * + * Returns 0 on success, -1 on error. + */ +static int +qemuProcessSetupPid(virDomainObjPtr vm, + pid_t pid, + virCgroupThreadName nameval, + int id, + virBitmapPtr cpumask, + unsigned long long period, + long long quota, + virDomainThreadSchedParamPtr sched) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainNumatuneMemMode mem_mode; + virCgroupPtr cgroup = NULL; + virBitmapPtr use_cpumask; + char *mem_mask = 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")); + goto cleanup; + } + + /* Infer which cpumask shall be used. */ + if (cpumask) + use_cpumask = cpumask; + else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + use_cpumask = priv->autoCpuset; + else + use_cpumask = vm->def->cpumask; + + /* + * 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)) { + + 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, nameval, id, true, &cgroup) < 0) + goto cleanup; + + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + if (use_cpumask && + qemuSetupCgroupCpusetCpus(cgroup, use_cpumask) < 0) + goto cleanup; + + /* + * Don't setup cpuset.mems for the emulator, they need to + * be set up after initialization in order for kvm + * allocations to succeed. + */ + if (nameval != VIR_CGROUP_THREAD_EMULATOR && + mem_mask && virCgroupSetCpusetMems(cgroup, mem_mask) < 0) + goto cleanup; + + } + + if ((period || quota) && + qemuSetupCgroupVcpuBW(cgroup, period, quota) < 0) + goto cleanup; + + /* Move the thread to the sub dir */ + if (virCgroupAddTask(cgroup, pid) < 0) + goto cleanup; + + } + + /* Setup legacy affinity. */ + if (use_cpumask && virProcessSetAffinity(pid, use_cpumask) < 0) + goto cleanup; + + /* Set scheduler type and priority. */ + if (sched && + virProcessSetScheduler(pid, sched->policy, sched->priority) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(mem_mask); + if (cgroup) { + if (ret < 0) + virCgroupRemove(cgroup); + virCgroupFree(&cgroup); + } + + return ret; +} + + static int qemuProcessSetupEmulator(virDomainObjPtr vm) { @@ -4704,98 +4812,18 @@ 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, + return qemuProcessSetupPid(vm, iothread->thread_id, + 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 (cgroup_iothread) { - 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 (virCgroupAddTask(cgroup_iothread, iothread->thread_id) < 0) - goto cleanup; - } - - if (cpumask && virProcessSetAffinity(iothread->thread_id, cpumask) < 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; + iothread->cpumask, + vm->def->cputune.period, + vm->def->cputune.quota, + &iothread->sched); } -- 2.9.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 69 ++++--------------------------------------------- 1 file changed, 5 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6fd6ee3d2d9a..0f8ef59c6fe5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2415,70 +2415,11 @@ qemuProcessSetupPid(virDomainObjPtr vm, static int qemuProcessSetupEmulator(virDomainObjPtr vm) { - virBitmapPtr cpumask = NULL; - virCgroupPtr cgroup_emulator = NULL; - qemuDomainObjPrivatePtr priv = vm->privateData; - unsigned long long period = vm->def->cputune.emulator_period; - long long quota = vm->def->cputune.emulator_quota; - 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 (vm->def->cputune.emulatorpin) - cpumask = vm->def->cputune.emulatorpin; - else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && - priv->autoCpuset) - cpumask = priv->autoCpuset; - else - cpumask = vm->def->cpumask; - - /* 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)) { - - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, - true, &cgroup_emulator) < 0) - goto cleanup; - - if (virCgroupAddTask(cgroup_emulator, vm->pid) < 0) - goto cleanup; - - - if (cpumask) { - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET) && - qemuSetupCgroupCpusetCpus(cgroup_emulator, cpumask) < 0) - goto cleanup; - } - - if (period || quota) { - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && - qemuSetupCgroupVcpuBW(cgroup_emulator, period, - quota) < 0) - goto cleanup; - } - } - - if (cpumask && - virProcessSetAffinity(vm->pid, cpumask) < 0) - goto cleanup; - - ret = 0; - - cleanup: - if (cgroup_emulator) { - if (ret < 0) - virCgroupRemove(cgroup_emulator); - virCgroupFree(&cgroup_emulator); - } - - return ret; + return qemuProcessSetupPid(vm, vm->pid, VIR_CGROUP_THREAD_EMULATOR, + 0, vm->def->cputune.emulatorpin, + vm->def->cputune.emulator_period, + vm->def->cputune.emulator_quota, + NULL); } -- 2.9.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 78 ++++--------------------------------------------- 1 file changed, 5 insertions(+), 73 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0f8ef59c6fe5..d329f6dd3ac6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4627,80 +4627,12 @@ qemuProcessSetupVcpu(virDomainObjPtr vm, { 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 (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; - /* setup cgroups */ - if (cgroup_vcpu) { - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - if (mem_mask && virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0) - goto cleanup; - - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumask) < 0) - goto cleanup; - } - - /* move the thread for vcpu to sub dir */ - if (virCgroupAddTask(cgroup_vcpu, vcpupid) < 0) - goto cleanup; - } - - /* setup legacy affinty */ - if (cpumask && virProcessSetAffinity(vcpupid, cpumask) < 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; + return qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU, + vcpuid, vcpu->cpumask, + vm->def->cputune.period, + vm->def->cputune.quota, + &vcpu->sched); } -- 2.9.0
On 07.07.2016 15:27, Martin Kletzander wrote:
Initially posted in order to achieve something else, but that got rejected, so at least the cleanup part of it could make it in.
The previous versions (whose goal was something completely idfferent) are here:
- https://www.redhat.com/archives/libvir-list/2016-June/msg01537.html
- https://www.redhat.com/archives/libvir-list/2016-July/msg00173.html
Martin Kletzander (3): qemu: Add qemuProcessSetupPid() and use it in qemuProcessSetupIOThread() qemu: Use qemuProcessSetupPid() in qemuProcessSetupEmulator() qemu: Use qemuProcessSetupPid() in qemuProcessSetupVcpu()
src/qemu/qemu_process.c | 283 ++++++++++++++++-------------------------------- 1 file changed, 92 insertions(+), 191 deletions(-)
ACK series. Nice cleanup! Michal
participants (2)
-
Martin Kletzander -
Michal Privoznik