[libvirt] [PATCH v2 0/5] Warn users with weird thread/memory alignment

[libvirt] [libvirt-php PATCH 0/3]...

[libvirt] [PATCH] qemu: caps:...

Martin Kletzander

Thursday, 7 July 2016 Thu, 7 Jul '16

7:12 a.m.

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

Show replies by date

Martin Kletzander

Thursday, 7 July Thu, 7 Jul

7:12 a.m.

New subject: [libvirt] [PATCH v2 1/5] qemu: Add qemuProcessSetupPid() and use it in qemuProcessSetupIOThread()

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(a)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

Martin Kletzander

7:12 a.m.

New subject: [libvirt] [PATCH v2 2/5] qemu: Use qemuProcessSetupPid() in qemuProcessSetupEmulator()

Signed-off-by: Martin Kletzander <mkletzan(a)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

Martin Kletzander

7:12 a.m.

New subject: [libvirt] [PATCH v2 3/5] qemu: Use qemuProcessSetupPid() in qemuProcessSetupVcpu()

Signed-off-by: Martin Kletzander <mkletzan(a)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

Martin Kletzander

7:12 a.m.

New subject: [libvirt] [PATCH v2 4/5] qemu: Add driver parameter to qemuProcessSetupPid() and callers

It will be used later. Signed-off-by: Martin Kletzander <mkletzan(a)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

Martin Kletzander

7:12 a.m.

New subject: [libvirt] [PATCH v2 5/5] qemu: Check for thread <=> memory alignment

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(a)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

Daniel P. Berrange

7:42 a.m.

New subject: [libvirt] [PATCH v2 5/5] qemu: Check for thread <=> memory alignment

On Thu, Jul 07, 2016 at 02:12:32PM +0200, Martin Kletzander wrote:

...

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 :|

Daniel P. Berrange

7:43 a.m.

New subject: [libvirt] [PATCH v2 5/5] qemu: Check for thread <=> memory alignment

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(a)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 :|

Martin Kletzander

8:28 a.m.

New subject: [libvirt] [PATCH v2 5/5] qemu: Check for thread <=> memory alignment

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(a)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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Daniel P. Berrange

8:57 a.m.

New subject: [libvirt] [PATCH v2 5/5] qemu: Check for thread <=> memory alignment

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(a)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 :|

3063

days inactive

3063

days old

devel@lists.libvirt.org

Manage subscription

9 comments

2 participants

tags (0)

participants (2)

Daniel P. Berrange
Martin Kletzander

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH v2 0/5] Warn users with weird thread/memory alignment