[libvirt] [PATCH v2 0/5] Warn users with weird thread/memory alignment
v2: - Split into more patches for easier review. I tried splitting it even more and found out this is just enough not to increase complexity. - Also worked in some review comments from Jan and John v1: - https://www.redhat.com/archives/libvir-list/2016-June/msg01537.html Martin Kletzander (5): qemu: Add qemuProcessSetupPid() and use it in qemuProcessSetupIOThread() qemu: Use qemuProcessSetupPid() in qemuProcessSetupEmulator() qemu: Use qemuProcessSetupPid() in qemuProcessSetupVcpu() qemu: Add driver parameter to qemuProcessSetupPid() and callers qemu: Check for thread <=> memory alignment src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_process.c | 380 +++++++++++++++++++++++------------------------- src/qemu/qemu_process.h | 6 +- 3 files changed, 186 insertions(+), 204 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
It will be used later. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_process.c | 34 ++++++++++++++++++++-------------- src/qemu/qemu_process.h | 6 ++++-- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f9a3b1522391..9a476b838f39 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4642,7 +4642,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, goto cleanup; } - if (qemuProcessSetupVcpu(vm, vcpu) < 0) + if (qemuProcessSetupVcpu(driver, vm, vcpu) < 0) goto cleanup; ret = 0; @@ -5768,7 +5768,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, iothrid->thread_id = new_iothreads[idx]->thread_id; - if (qemuProcessSetupIOThread(vm, iothrid) < 0) + if (qemuProcessSetupIOThread(driver, vm, iothrid) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d329f6dd3ac6..0aab01fd4d50 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2315,7 +2315,8 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, * Returns 0 on success, -1 on error. */ static int -qemuProcessSetupPid(virDomainObjPtr vm, +qemuProcessSetupPid(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr vm, pid_t pid, virCgroupThreadName nameval, int id, @@ -2413,9 +2414,10 @@ qemuProcessSetupPid(virDomainObjPtr vm, static int -qemuProcessSetupEmulator(virDomainObjPtr vm) +qemuProcessSetupEmulator(virQEMUDriverPtr driver, + virDomainObjPtr vm) { - return qemuProcessSetupPid(vm, vm->pid, VIR_CGROUP_THREAD_EMULATOR, + return qemuProcessSetupPid(driver, vm, vm->pid, VIR_CGROUP_THREAD_EMULATOR, 0, vm->def->cputune.emulatorpin, vm->def->cputune.emulator_period, vm->def->cputune.emulator_quota, @@ -4622,13 +4624,14 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def) * Returns 0 on success, -1 on error. */ int -qemuProcessSetupVcpu(virDomainObjPtr vm, +qemuProcessSetupVcpu(virQEMUDriverPtr driver, + virDomainObjPtr vm, unsigned int vcpuid) { pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid); virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid); - return qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU, + return qemuProcessSetupPid(driver, vm, vcpupid, VIR_CGROUP_THREAD_VCPU, vcpuid, vcpu->cpumask, vm->def->cputune.period, vm->def->cputune.quota, @@ -4637,7 +4640,8 @@ qemuProcessSetupVcpu(virDomainObjPtr vm, static int -qemuProcessSetupVcpus(virDomainObjPtr vm) +qemuProcessSetupVcpus(virQEMUDriverPtr driver, + virDomainObjPtr vm) { virDomainVcpuInfoPtr vcpu; unsigned int maxvcpus = virDomainDefGetVcpusMax(vm->def); @@ -4677,7 +4681,7 @@ qemuProcessSetupVcpus(virDomainObjPtr vm) if (!vcpu->online) continue; - if (qemuProcessSetupVcpu(vm, i) < 0) + if (qemuProcessSetupVcpu(driver, vm, i) < 0) return -1; } @@ -4686,11 +4690,12 @@ qemuProcessSetupVcpus(virDomainObjPtr vm) int -qemuProcessSetupIOThread(virDomainObjPtr vm, +qemuProcessSetupIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainIOThreadIDDefPtr iothread) { - return qemuProcessSetupPid(vm, iothread->thread_id, + return qemuProcessSetupPid(driver, vm, iothread->thread_id, VIR_CGROUP_THREAD_IOTHREAD, iothread->iothread_id, iothread->cpumask, @@ -4701,14 +4706,15 @@ qemuProcessSetupIOThread(virDomainObjPtr vm, static int -qemuProcessSetupIOThreads(virDomainObjPtr vm) +qemuProcessSetupIOThreads(virQEMUDriverPtr driver, + virDomainObjPtr vm) { size_t i; for (i = 0; i < vm->def->niothreadids; i++) { virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i]; - if (qemuProcessSetupIOThread(vm, info) < 0) + if (qemuProcessSetupIOThread(driver, vm, info) < 0) return -1; } @@ -5134,7 +5140,7 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; VIR_DEBUG("Setting emulator tuning/settings"); - if (qemuProcessSetupEmulator(vm) < 0) + if (qemuProcessSetupEmulator(driver, vm) < 0) goto cleanup; VIR_DEBUG("Setting domain security labels"); @@ -5215,11 +5221,11 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; VIR_DEBUG("Setting vCPU tuning/settings"); - if (qemuProcessSetupVcpus(vm) < 0) + if (qemuProcessSetupVcpus(driver, vm) < 0) goto cleanup; VIR_DEBUG("Setting IOThread tuning/settings"); - if (qemuProcessSetupIOThreads(vm) < 0) + if (qemuProcessSetupIOThreads(driver, 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 21f3b0ccaa3c..6ada8cadd8b7 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -174,9 +174,11 @@ virDomainDiskDefPtr qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm, int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm); -int qemuProcessSetupVcpu(virDomainObjPtr vm, +int qemuProcessSetupVcpu(virQEMUDriverPtr driver, + virDomainObjPtr vm, unsigned int vcpuid); -int qemuProcessSetupIOThread(virDomainObjPtr vm, +int qemuProcessSetupIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainIOThreadIDDefPtr iothread); int qemuRefreshVirtioChannelState(virQEMUDriverPtr driver, -- 2.9.0
Some settings may be confusing and in case users use numad placement in combination with static placement we could warn them as it might not be wanted (but it's not forbidden). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1254402 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0aab01fd4d50..c012b6efcab6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2304,6 +2304,76 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, } +static int +qemuProcessCheckCpusMemsAlignment(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virBitmapPtr cpumask, + const char *mem_mask) +{ + int ret = -1; + int hostnodes = 0; + char *cpumask_str = NULL; + char *tmpmask_str = NULL; + char *mem_cpus_str = NULL; + virCapsPtr caps = NULL; + virBitmapPtr tmpmask = NULL; + virBitmapPtr mem_cpus = NULL; + virBitmapPtr mem_nodes = NULL; + virDomainNumatuneMemMode mem_mode; + + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) != 0) + return 0; + + if (mem_mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) + return 0; + + if (!mem_mask || !cpumask) + return 0; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(tmpmask = virBitmapNewCopy(cpumask))) + goto cleanup; + + if ((hostnodes = virNumaGetMaxNode()) < 0) + goto cleanup; + + if (virBitmapParse(mem_mask, &mem_nodes, hostnodes) < 0) + goto cleanup; + + if (!(mem_cpus = virCapabilitiesGetCpusForNodemask(caps, mem_nodes))) + goto cleanup; + + virBitmapSubtract(tmpmask, mem_cpus); + if (!virBitmapIsAllClear(tmpmask)) { + if (!(cpumask_str = virBitmapFormat(cpumask))) + goto cleanup; + + if (!(tmpmask_str = virBitmapFormat(tmpmask))) + goto cleanup; + + if (!(mem_cpus_str = virBitmapFormat(mem_cpus))) + goto cleanup; + + VIR_WARN("CPUs '%s' in cpumask '%s' might not have access to any NUMA " + "node in memory's nodeset '%s' which consists of CPUs: '%s'.", + tmpmask_str, cpumask_str, mem_mask, mem_cpus_str); + } + + ret = 0; + cleanup: + VIR_FREE(mem_cpus_str); + VIR_FREE(tmpmask_str); + VIR_FREE(cpumask_str); + virBitmapFree(mem_nodes); + virBitmapFree(mem_cpus); + virBitmapFree(tmpmask); + virObjectUnref(caps); + return ret; +} + + /** * qemuProcessSetupPid: * @@ -2315,7 +2385,7 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, * Returns 0 on success, -1 on error. */ static int -qemuProcessSetupPid(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuProcessSetupPid(virQEMUDriverPtr driver, virDomainObjPtr vm, pid_t pid, virCgroupThreadName nameval, @@ -2389,6 +2459,9 @@ qemuProcessSetupPid(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, if (virCgroupAddTask(cgroup, pid) < 0) goto cleanup; + if (qemuProcessCheckCpusMemsAlignment(driver, vm, + use_cpumask, mem_mask) < 0) + goto cleanup; } /* Setup legacy affinity. */ -- 2.9.0
On Thu, Jul 07, 2016 at 02:12:32PM +0200, Martin Kletzander wrote:
Some settings may be confusing and in case users use numad placement in combination with static placement we could warn them as it might not be wanted (but it's not forbidden).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1254402
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0aab01fd4d50..c012b6efcab6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2304,6 +2304,76 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, }
+static int +qemuProcessCheckCpusMemsAlignment(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virBitmapPtr cpumask, + const char *mem_mask) +{ + int ret = -1; + int hostnodes = 0; + char *cpumask_str = NULL; + char *tmpmask_str = NULL; + char *mem_cpus_str = NULL; + virCapsPtr caps = NULL; + virBitmapPtr tmpmask = NULL; + virBitmapPtr mem_cpus = NULL; + virBitmapPtr mem_nodes = NULL; + virDomainNumatuneMemMode mem_mode; + + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) != 0) + return 0; + + if (mem_mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) + return 0; + + if (!mem_mask || !cpumask) + return 0; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(tmpmask = virBitmapNewCopy(cpumask))) + goto cleanup; + + if ((hostnodes = virNumaGetMaxNode()) < 0) + goto cleanup; + + if (virBitmapParse(mem_mask, &mem_nodes, hostnodes) < 0) + goto cleanup; + + if (!(mem_cpus = virCapabilitiesGetCpusForNodemask(caps, mem_nodes))) + goto cleanup; + + virBitmapSubtract(tmpmask, mem_cpus); + if (!virBitmapIsAllClear(tmpmask)) { + if (!(cpumask_str = virBitmapFormat(cpumask))) + goto cleanup; + + if (!(tmpmask_str = virBitmapFormat(tmpmask))) + goto cleanup; + + if (!(mem_cpus_str = virBitmapFormat(mem_cpus))) + goto cleanup; + + VIR_WARN("CPUs '%s' in cpumask '%s' might not have access to any NUMA " + "node in memory's nodeset '%s' which consists of CPUs: '%s'.", + tmpmask_str, cpumask_str, mem_mask, mem_cpus_str);
We've had a general principle that we don't use VIR_WARN for this kind of thing, because libvirtd logs are genrally invisible to the person who is making the mistake. Meanwhile if this is intentional, we're spamming the logs for a situation the user explicitly chose. So NACK to the entire patch, as it doesn't do anything useful IMHO. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Jul 07, 2016 at 01:42:33PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 07, 2016 at 02:12:32PM +0200, Martin Kletzander wrote:
Some settings may be confusing and in case users use numad placement in combination with static placement we could warn them as it might not be wanted (but it's not forbidden).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1254402
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0aab01fd4d50..c012b6efcab6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2304,6 +2304,76 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, }
+static int +qemuProcessCheckCpusMemsAlignment(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virBitmapPtr cpumask, + const char *mem_mask) +{ + int ret = -1; + int hostnodes = 0; + char *cpumask_str = NULL; + char *tmpmask_str = NULL; + char *mem_cpus_str = NULL; + virCapsPtr caps = NULL; + virBitmapPtr tmpmask = NULL; + virBitmapPtr mem_cpus = NULL; + virBitmapPtr mem_nodes = NULL; + virDomainNumatuneMemMode mem_mode; + + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) != 0) + return 0; + + if (mem_mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) + return 0; + + if (!mem_mask || !cpumask) + return 0; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(tmpmask = virBitmapNewCopy(cpumask))) + goto cleanup; + + if ((hostnodes = virNumaGetMaxNode()) < 0) + goto cleanup; + + if (virBitmapParse(mem_mask, &mem_nodes, hostnodes) < 0) + goto cleanup; + + if (!(mem_cpus = virCapabilitiesGetCpusForNodemask(caps, mem_nodes))) + goto cleanup; + + virBitmapSubtract(tmpmask, mem_cpus); + if (!virBitmapIsAllClear(tmpmask)) { + if (!(cpumask_str = virBitmapFormat(cpumask))) + goto cleanup; + + if (!(tmpmask_str = virBitmapFormat(tmpmask))) + goto cleanup; + + if (!(mem_cpus_str = virBitmapFormat(mem_cpus))) + goto cleanup; + + VIR_WARN("CPUs '%s' in cpumask '%s' might not have access to any NUMA " + "node in memory's nodeset '%s' which consists of CPUs: '%s'.", + tmpmask_str, cpumask_str, mem_mask, mem_cpus_str);
We've had a general principle that we don't use VIR_WARN for this kind of thing, because libvirtd logs are genrally invisible to the person who is making the mistake. Meanwhile if this is intentional, we're spamming the logs for a situation the user explicitly chose.
So NACK to the entire patch, as it doesn't do anything useful IMHO.
BTW, I'd suggest NOTABUG on this Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Jul 07, 2016 at 01:43:32PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 07, 2016 at 01:42:33PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 07, 2016 at 02:12:32PM +0200, Martin Kletzander wrote:
Some settings may be confusing and in case users use numad placement in combination with static placement we could warn them as it might not be wanted (but it's not forbidden).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1254402
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0aab01fd4d50..c012b6efcab6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2304,6 +2304,76 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, }
+static int +qemuProcessCheckCpusMemsAlignment(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virBitmapPtr cpumask, + const char *mem_mask) +{ + int ret = -1; + int hostnodes = 0; + char *cpumask_str = NULL; + char *tmpmask_str = NULL; + char *mem_cpus_str = NULL; + virCapsPtr caps = NULL; + virBitmapPtr tmpmask = NULL; + virBitmapPtr mem_cpus = NULL; + virBitmapPtr mem_nodes = NULL; + virDomainNumatuneMemMode mem_mode; + + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) != 0) + return 0; + + if (mem_mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) + return 0; + + if (!mem_mask || !cpumask) + return 0; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(tmpmask = virBitmapNewCopy(cpumask))) + goto cleanup; + + if ((hostnodes = virNumaGetMaxNode()) < 0) + goto cleanup; + + if (virBitmapParse(mem_mask, &mem_nodes, hostnodes) < 0) + goto cleanup; + + if (!(mem_cpus = virCapabilitiesGetCpusForNodemask(caps, mem_nodes))) + goto cleanup; + + virBitmapSubtract(tmpmask, mem_cpus); + if (!virBitmapIsAllClear(tmpmask)) { + if (!(cpumask_str = virBitmapFormat(cpumask))) + goto cleanup; + + if (!(tmpmask_str = virBitmapFormat(tmpmask))) + goto cleanup; + + if (!(mem_cpus_str = virBitmapFormat(mem_cpus))) + goto cleanup; + + VIR_WARN("CPUs '%s' in cpumask '%s' might not have access to any NUMA " + "node in memory's nodeset '%s' which consists of CPUs: '%s'.", + tmpmask_str, cpumask_str, mem_mask, mem_cpus_str);
We've had a general principle that we don't use VIR_WARN for this kind of thing, because libvirtd logs are genrally invisible to the person who is making the mistake. Meanwhile if this is intentional, we're spamming the logs for a situation the user explicitly chose.
So NACK to the entire patch, as it doesn't do anything useful IMHO.
BTW, I'd suggest NOTABUG on this
OK, I posted the first three patches as a clean-up separately so at least we shave off some duplicated code ;)
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 07, 2016 at 03:28:44PM +0200, Martin Kletzander wrote:
On Thu, Jul 07, 2016 at 01:43:32PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 07, 2016 at 01:42:33PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 07, 2016 at 02:12:32PM +0200, Martin Kletzander wrote:
Some settings may be confusing and in case users use numad placement in combination with static placement we could warn them as it might not be wanted (but it's not forbidden).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1254402
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_process.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0aab01fd4d50..c012b6efcab6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2304,6 +2304,76 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, }
+static int +qemuProcessCheckCpusMemsAlignment(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virBitmapPtr cpumask, + const char *mem_mask) +{ + int ret = -1; + int hostnodes = 0; + char *cpumask_str = NULL; + char *tmpmask_str = NULL; + char *mem_cpus_str = NULL; + virCapsPtr caps = NULL; + virBitmapPtr tmpmask = NULL; + virBitmapPtr mem_cpus = NULL; + virBitmapPtr mem_nodes = NULL; + virDomainNumatuneMemMode mem_mode; + + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) != 0) + return 0; + + if (mem_mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) + return 0; + + if (!mem_mask || !cpumask) + return 0; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(tmpmask = virBitmapNewCopy(cpumask))) + goto cleanup; + + if ((hostnodes = virNumaGetMaxNode()) < 0) + goto cleanup; + + if (virBitmapParse(mem_mask, &mem_nodes, hostnodes) < 0) + goto cleanup; + + if (!(mem_cpus = virCapabilitiesGetCpusForNodemask(caps, mem_nodes))) + goto cleanup; + + virBitmapSubtract(tmpmask, mem_cpus); + if (!virBitmapIsAllClear(tmpmask)) { + if (!(cpumask_str = virBitmapFormat(cpumask))) + goto cleanup; + + if (!(tmpmask_str = virBitmapFormat(tmpmask))) + goto cleanup; + + if (!(mem_cpus_str = virBitmapFormat(mem_cpus))) + goto cleanup; + + VIR_WARN("CPUs '%s' in cpumask '%s' might not have access to any NUMA " + "node in memory's nodeset '%s' which consists of CPUs: '%s'.", + tmpmask_str, cpumask_str, mem_mask, mem_cpus_str);
We've had a general principle that we don't use VIR_WARN for this kind of thing, because libvirtd logs are genrally invisible to the person who is making the mistake. Meanwhile if this is intentional, we're spamming the logs for a situation the user explicitly chose.
So NACK to the entire patch, as it doesn't do anything useful IMHO.
BTW, I'd suggest NOTABUG on this
OK, I posted the first three patches as a clean-up separately so at least we shave off some duplicated code ;)
Oh sure, no objection to the general cleanup patches going in, jsut this last one. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Martin Kletzander