[libvirt] [PATCH 0/8] Fix cgroups regresion when default cpuset is specified
Since commit a39f69d2b libvirt would fail to start a VM if the default cpu set was specified and individual vcpus were pinned to cpus outside of that cpuset. Peter Krempa (8): qemu: cgroup: Store auto cpuset instead of re-creating it on demand qemu: cgroup: Refactor setup for IOThread cgroups qemu: cgroup: Properly set up vcpu pinning qemu: cgroup: Use priv->autoCpuset instead of using qemuPrepareCpumap() qemu: cgroup: Rename qemuSetupCgroupEmulatorPin to qemuSetupCgroupCpusetCpus qemu: cgroup: Kill qemuSetupCgroupIOThreadsPin() qemu: cgroup: Kill qemuSetupCgroupVcpuPin() qemu: Copy bitmap in a sane way src/qemu/qemu_cgroup.c | 150 +++++++++++++++--------------------------------- src/qemu/qemu_cgroup.h | 13 +---- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 21 +++---- src/qemu/qemu_process.c | 93 +++++++++--------------------- src/qemu/qemu_process.h | 2 - 7 files changed, 85 insertions(+), 198 deletions(-) -- 2.2.2
The automatic cpuset can be stored along with automatic nodeset and it does not have to be recreated when used. --- src/qemu/qemu_cgroup.c | 21 +++++---------------- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 11 +++++++---- 4 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index a422fbc..bc7632f 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -642,8 +642,7 @@ qemuSetupCpusetMems(virDomainObjPtr vm) static int -qemuSetupCpusetCgroup(virDomainObjPtr vm, - virCapsPtr caps) +qemuSetupCpusetCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; char *cpu_mask = NULL; @@ -658,15 +657,10 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm, if (vm->def->cpumask || (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)) { - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { - virBitmapPtr cpumap; - if (!(cpumap = virCapabilitiesGetCpusForNodemask(caps, priv->autoNodeset))) - goto cleanup; - cpu_mask = virBitmapFormat(cpumap); - virBitmapFree(cpumap); - } else { + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + cpu_mask = virBitmapFormat(priv->autoCpuset); + else cpu_mask = virBitmapFormat(vm->def->cpumask); - } if (!cpu_mask) goto cleanup; @@ -896,7 +890,6 @@ qemuSetupCgroup(virQEMUDriverPtr driver, int *nicindexes) { qemuDomainObjPrivatePtr priv = vm->privateData; - virCapsPtr caps = NULL; int ret = -1; if (!vm->pid) { @@ -911,9 +904,6 @@ qemuSetupCgroup(virQEMUDriverPtr driver, if (!priv->cgroup) return 0; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; - if (qemuSetupDevicesCgroup(driver, vm) < 0) goto cleanup; @@ -926,12 +916,11 @@ qemuSetupCgroup(virQEMUDriverPtr driver, if (qemuSetupCpuCgroup(driver, vm) < 0) goto cleanup; - if (qemuSetupCpusetCgroup(vm, caps) < 0) + if (qemuSetupCpusetCgroup(vm) < 0) goto cleanup; ret = 0; cleanup: - virObjectUnref(caps); return ret; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 758fcd9..d1be66e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -458,6 +458,7 @@ qemuDomainObjPrivateFree(void *data) } VIR_FREE(priv->cleanupCallbacks); virBitmapFree(priv->autoNodeset); + virBitmapFree(priv->autoCpuset); VIR_FREE(priv); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b854b54..e4140d8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -191,7 +191,10 @@ struct _qemuDomainObjPrivate { char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */ bool hookRun; /* true if there was a hook run over this domain */ + + /* Bitmaps below hold data from the auto NUMA feature */ virBitmapPtr autoNodeset; + virBitmapPtr autoCpuset; }; typedef enum { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 79f763e..4a786b1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4321,7 +4321,6 @@ int qemuProcessStart(virConnectPtr conn, size_t i; bool rawio_set = false; char *nodeset = NULL; - virBitmapPtr nodemask = NULL; unsigned int stop_flags; virQEMUDriverConfigPtr cfg; virCapsPtr caps = NULL; @@ -4582,10 +4581,14 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Nodeset returned from numad: %s", nodeset); - if (virBitmapParse(nodeset, 0, &nodemask, VIR_DOMAIN_CPUMASK_LEN) < 0) + if (virBitmapParse(nodeset, 0, &priv->autoNodeset, + VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + + if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, + priv->autoNodeset))) goto cleanup; } - priv->autoNodeset = nodemask; /* "volume" type disk's source must be translated before * cgroup and security setting. @@ -4652,7 +4655,7 @@ int qemuProcessStart(virConnectPtr conn, migrateFrom, stdin_fd, snapshot, vmop, &buildCommandLineCallbacks, false, qemuCheckFips(), - nodemask, + priv->autoNodeset, &nnicindexes, &nicindexes))) goto cleanup; -- 2.2.2
Use the default or auto cpuset if they are provided for IOThreads. --- src/qemu/qemu_cgroup.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index bc7632f..be02ede 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1255,21 +1255,26 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* Set iothreadpin in cgroup if iothreadpin xml is provided */ if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - /* find the right CPU to pin, otherwise - * qemuSetupCgroupIOThreadsPin will fail. */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - /* IOThreads are numbered/named 1..n */ - if (def->cputune.iothreadspin[j]->id != i + 1) - continue; + virBitmapPtr cpumask = NULL; - if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, - def->cputune.iothreadspin, - def->cputune.niothreadspin, - i + 1) < 0) - goto cleanup; + /* default cpu masks */ + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + cpumask = priv->autoCpuset; + else + cpumask = def->cpumask; - break; + /* specific cpu mask */ + for (j = 0; j < def->cputune.niothreadspin; j++) { + /* IOThreads are numbered/named 1..n */ + if (def->cputune.iothreadspin[j]->id == i + 1) { + cpumask = def->cputune.iothreadspin[j]->cpumask; + break; + } } + + if (cpumask && + qemuSetupCgroupEmulatorPin(cgroup_iothread, cpumask) < 0) + goto cleanup; } virCgroupFree(&cgroup_iothread); -- 2.2.2
When the default cpuset or automatic numa placement is used libvirt would place the whole parent cgroup in the specified cpuset. This then disallowed to re-pin the vcpus to a different cpu. This patch pins only the vcpu threads to the default cpuset and thus allows to re-pin them later. The following config would fail to start: <domain type='kvm'> ... <vcpu placement='static' cpuset='0-1' current='2'>4</vcpu> <cputune> <vcpupin vcpu='0' cpuset='2-3'/> ... This is a regression since a39f69d2b. --- src/qemu/qemu_cgroup.c | 51 +++++++++++++++++++------------------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index be02ede..8674ab8 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -645,8 +645,6 @@ static int qemuSetupCpusetCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - char *cpu_mask = NULL; - int ret = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) return 0; @@ -654,25 +652,7 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm) if (virCgroupSetCpusetMemoryMigrate(priv->cgroup, true) < 0) return -1; - if (vm->def->cpumask || - (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)) { - - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - cpu_mask = virBitmapFormat(priv->autoCpuset); - else - cpu_mask = virBitmapFormat(vm->def->cpumask); - - if (!cpu_mask) - goto cleanup; - - if (virCgroupSetCpusetCpus(priv->cgroup, cpu_mask) < 0) - goto cleanup; - } - - ret = 0; - cleanup: - VIR_FREE(cpu_mask); - return ret; + return 0; } @@ -1079,20 +1059,27 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) /* Set vcpupin in cgroup if vcpupin xml is provided */ if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - /* find the right CPU to pin, otherwise - * qemuSetupCgroupVcpuPin will fail. */ - for (j = 0; j < def->cputune.nvcpupin; j++) { - if (def->cputune.vcpupin[j]->id != i) - continue; + virBitmapPtr cpumap = NULL; - if (qemuSetupCgroupVcpuPin(cgroup_vcpu, - def->cputune.vcpupin, - def->cputune.nvcpupin, - i) < 0) - goto cleanup; + /* try to use the default cpu maps */ + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + cpumap = priv->autoCpuset; + else + cpumap = vm->def->cpumask; - break; + /* lookup a more specific pinning info */ + for (j = 0; j < def->cputune.nvcpupin; j++) { + if (def->cputune.vcpupin[j]->id == i) { + cpumap = def->cputune.vcpupin[j]->cpumask; + break; + } } + + if (!cpumap) + continue; + + if (qemuSetupCgroupEmulatorPin(cgroup_vcpu, cpumap) < 0) + goto cleanup; } virCgroupFree(&cgroup_vcpu); -- 2.2.2
Two places would call to qemuPrepareCpumap() with priv->autoNodeset to convert it to a cpuset. Remove the function and use the prepared cpuset automatically. --- src/qemu/qemu_cgroup.c | 18 +++-------- src/qemu/qemu_cgroup.h | 3 +- src/qemu/qemu_process.c | 82 ++++++++++++------------------------------------- src/qemu/qemu_process.h | 2 -- 4 files changed, 25 insertions(+), 80 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 8674ab8..7ba3059 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1099,11 +1099,9 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) } int -qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, - virDomainObjPtr vm) +qemuSetupCgroupForEmulator(virDomainObjPtr vm) { virBitmapPtr cpumask = NULL; - virBitmapPtr cpumap = NULL; virCgroupPtr cgroup_emulator = NULL; virDomainDefPtr def = vm->def; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1135,15 +1133,12 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, if (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 0) goto cleanup; - if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { - if (!(cpumap = qemuPrepareCpumap(driver, priv->autoNodeset))) - goto cleanup; - cpumask = cpumap; - } else if (def->cputune.emulatorpin) { + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + cpumask = priv->autoCpuset; + else if (def->cputune.emulatorpin) cpumask = def->cputune.emulatorpin->cpumask; - } else if (def->cpumask) { + else if (def->cpumask) cpumask = def->cpumask; - } if (cpumask) { if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET) && @@ -1159,12 +1154,9 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, } virCgroupFree(&cgroup_emulator); - virBitmapFree(cpumap); return 0; cleanup: - virBitmapFree(cpumap); - if (cgroup_emulator) { virCgroupRemove(cgroup_emulator); virCgroupFree(&cgroup_emulator); diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index b311af3..11893ef 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -63,8 +63,7 @@ int qemuSetupCgroupIOThreadsPin(virCgroupPtr cgroup, int iothreadid); int qemuSetupCgroupForVcpu(virDomainObjPtr vm); int qemuSetupCgroupForIOThreads(virDomainObjPtr vm); -int qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, - 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 4a786b1..69c44d9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2273,67 +2273,12 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, return ret; } -/* Helper to prepare cpumap for affinity setting, convert - * NUMA nodeset into cpuset if @nodemask is not NULL, otherwise - * just return a new allocated bitmap. - */ -virBitmapPtr -qemuPrepareCpumap(virQEMUDriverPtr driver, - virBitmapPtr nodemask) -{ - size_t i; - int hostcpus, maxcpu = QEMUD_CPUMASK_LEN; - virBitmapPtr cpumap = NULL; - virCapsPtr caps = NULL; - - /* setaffinity fails if you set bits for CPUs which - * aren't present, so we have to limit ourselves */ - if ((hostcpus = nodeGetCPUCount()) < 0) - return NULL; - - if (maxcpu > hostcpus) - maxcpu = hostcpus; - - if (!(cpumap = virBitmapNew(maxcpu))) - return NULL; - - if (nodemask) { - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) { - virBitmapFree(cpumap); - cpumap = NULL; - goto cleanup; - } - - for (i = 0; i < caps->host.nnumaCell; i++) { - size_t j; - int cur_ncpus = caps->host.numaCell[i]->ncpus; - bool result; - if (virBitmapGetBit(nodemask, caps->host.numaCell[i]->num, &result) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to convert nodeset to cpuset")); - virBitmapFree(cpumap); - cpumap = NULL; - goto cleanup; - } - if (result) { - for (j = 0; j < cur_ncpus; j++) - ignore_value(virBitmapSetBit(cpumap, - caps->host.numaCell[i]->cpus[j].id)); - } - } - } - - cleanup: - virObjectUnref(caps); - return cpumap; -} /* * To be run between fork/exec of QEMU only */ static int -qemuProcessInitCpuAffinity(virQEMUDriverPtr driver, - virDomainObjPtr vm) +qemuProcessInitCpuAffinity(virDomainObjPtr vm) { int ret = -1; virBitmapPtr cpumap = NULL; @@ -2346,23 +2291,34 @@ qemuProcessInitCpuAffinity(virQEMUDriverPtr driver, return -1; } - if (!(cpumap = qemuPrepareCpumap(driver, priv->autoNodeset))) - return -1; - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { VIR_DEBUG("Set CPU affinity with advisory nodeset from numad"); - cpumapToSet = cpumap; + cpumapToSet = priv->autoCpuset; } else { VIR_DEBUG("Set CPU affinity with specified cpuset"); if (vm->def->cpumask) { cpumapToSet = vm->def->cpumask; } else { - cpumapToSet = cpumap; /* You may think this is redundant, but we can't assume libvirtd * itself is running on all pCPUs, so we need to explicitly set * the spawned QEMU instance to all pCPUs if no map is given in * its config file */ + int hostcpus; + + /* setaffinity fails if you set bits for CPUs which + * aren't present, so we have to limit ourselves */ + if ((hostcpus = nodeGetCPUCount()) < 0) + goto cleanup; + + if (hostcpus > QEMUD_CPUMASK_LEN) + hostcpus = QEMUD_CPUMASK_LEN; + + if (!(cpumap = virBitmapNew(hostcpus))) + goto cleanup; + virBitmapSetAll(cpumap); + + cpumapToSet = cpumap; } } @@ -4787,7 +4743,7 @@ int qemuProcessStart(virConnectPtr conn, /* This must be done after cgroup placement to avoid resetting CPU * affinity */ if (!vm->def->cputune.emulatorpin && - qemuProcessInitCpuAffinity(driver, vm) < 0) + qemuProcessInitCpuAffinity(vm) < 0) goto cleanup; VIR_DEBUG("Setting domain security labels"); @@ -4834,7 +4790,7 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; VIR_DEBUG("Setting cgroup for emulator (if required)"); - if (qemuSetupCgroupForEmulator(driver, vm) < 0) + if (qemuSetupCgroupForEmulator(vm) < 0) goto cleanup; VIR_DEBUG("Setting affinity of emulator threads"); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 3c04179..c67b0cb 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -99,8 +99,6 @@ int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver, virDomainObjPtr vm); bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver, virDomainObjPtr vm); -virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver, - virBitmapPtr nodemask); int qemuProcessReadLog(int fd, char *buf, int buflen, int off, bool skipchar); -- 2.2.2
The function is used to set cpuset.cpus in various other helpers. --- src/qemu/qemu_cgroup.c | 14 +++++++------- src/qemu/qemu_cgroup.h | 2 +- src/qemu/qemu_driver.c | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 7ba3059..fad7003 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -952,7 +952,7 @@ qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, for (i = 0; i < nvcpupin; i++) { if (vcpuid == vcpupin[i]->id) - return qemuSetupCgroupEmulatorPin(cgroup, vcpupin[i]->cpumask); + return qemuSetupCgroupCpusetCpus(cgroup, vcpupin[i]->cpumask); } return -1; @@ -968,15 +968,15 @@ qemuSetupCgroupIOThreadsPin(virCgroupPtr cgroup, for (i = 0; i < niothreadspin; i++) { if (iothreadid == iothreadspin[i]->id) - return qemuSetupCgroupEmulatorPin(cgroup, iothreadspin[i]->cpumask); + return qemuSetupCgroupCpusetCpus(cgroup, iothreadspin[i]->cpumask); } return -1; } int -qemuSetupCgroupEmulatorPin(virCgroupPtr cgroup, - virBitmapPtr cpumask) +qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, + virBitmapPtr cpumask) { int ret = -1; char *new_cpus = NULL; @@ -1078,7 +1078,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) if (!cpumap) continue; - if (qemuSetupCgroupEmulatorPin(cgroup_vcpu, cpumap) < 0) + if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) goto cleanup; } @@ -1142,7 +1142,7 @@ qemuSetupCgroupForEmulator(virDomainObjPtr vm) if (cpumask) { if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET) && - qemuSetupCgroupEmulatorPin(cgroup_emulator, cpumask) < 0) + qemuSetupCgroupCpusetCpus(cgroup_emulator, cpumask) < 0) goto cleanup; } @@ -1252,7 +1252,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) } if (cpumask && - qemuSetupCgroupEmulatorPin(cgroup_iothread, cpumask) < 0) + qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 11893ef..0f7be7e 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -56,7 +56,7 @@ int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, virDomainPinDefPtr *vcpupin, int nvcpupin, int vcpuid); -int qemuSetupCgroupEmulatorPin(virCgroupPtr cgroup, virBitmapPtr cpumask); +int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); int qemuSetupCgroupIOThreadsPin(virCgroupPtr cgroup, virDomainPinDefPtr *iothreadspin, int niothreadspin, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index db4f0b4..949ba44 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5420,8 +5420,8 @@ qemuDomainPinEmulator(virDomainPtr dom, */ if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_emulator) < 0) goto endjob; - if (qemuSetupCgroupEmulatorPin(cgroup_emulator, - newVcpuPin[0]->cpumask) < 0) { + if (qemuSetupCgroupCpusetCpus(cgroup_emulator, + newVcpuPin[0]->cpumask) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("failed to set cpuset.cpus in cgroup" " for emulator threads")); -- 2.2.2
The function doesn't make sense. There's a simpler way to achieve the same. --- src/qemu/qemu_cgroup.c | 15 --------------- src/qemu/qemu_cgroup.h | 4 ---- src/qemu/qemu_driver.c | 5 +---- 3 files changed, 1 insertion(+), 23 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index fad7003..bd768ef 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -958,21 +958,6 @@ qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, return -1; } -int -qemuSetupCgroupIOThreadsPin(virCgroupPtr cgroup, - virDomainPinDefPtr *iothreadspin, - int niothreadspin, - int iothreadid) -{ - size_t i; - - for (i = 0; i < niothreadspin; i++) { - if (iothreadid == iothreadspin[i]->id) - return qemuSetupCgroupCpusetCpus(cgroup, iothreadspin[i]->cpumask); - } - - return -1; -} int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 0f7be7e..cdeb307 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -57,10 +57,6 @@ int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, int nvcpupin, int vcpuid); int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); -int qemuSetupCgroupIOThreadsPin(virCgroupPtr cgroup, - virDomainPinDefPtr *iothreadspin, - int niothreadspin, - int iothreadid); int qemuSetupCgroupForVcpu(virDomainObjPtr vm); int qemuSetupCgroupForIOThreads(virDomainObjPtr vm); int qemuSetupCgroupForEmulator(virDomainObjPtr vm); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 949ba44..fc44ef1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6016,10 +6016,7 @@ qemuDomainPinIOThread(virDomainPtr dom, if (virCgroupNewIOThread(priv->cgroup, iothread_id, false, &cgroup_iothread) < 0) goto endjob; - if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, - newIOThreadsPin, - newIOThreadsPinNum, - iothread_id) < 0) { + if (qemuSetupCgroupCpusetCpus(cgroup_iothread, pcpumap) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, _("failed to set cpuset.cpus in cgroup" " for iothread %d"), iothread_id); -- 2.2.2
The function doesn't make sense. There's a simpler way to achieve the same. --- src/qemu/qemu_cgroup.c | 16 ---------------- src/qemu/qemu_cgroup.h | 4 ---- src/qemu/qemu_driver.c | 8 +++----- 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index bd768ef..50546a1 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -942,22 +942,6 @@ qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, return -1; } -int -qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, - virDomainPinDefPtr *vcpupin, - int nvcpupin, - int vcpuid) -{ - size_t i; - - for (i = 0; i < nvcpupin; i++) { - if (vcpuid == vcpupin[i]->id) - return qemuSetupCgroupCpusetCpus(cgroup, vcpupin[i]->cpumask); - } - - return -1; -} - int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index cdeb307..711a6de 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -52,10 +52,6 @@ int qemuSetupCpusetMems(virDomainObjPtr vm); int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, unsigned long long period, long long quota); -int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, - virDomainPinDefPtr *vcpupin, - int nvcpupin, - int vcpuid); int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); int qemuSetupCgroupForVcpu(virDomainObjPtr vm); int qemuSetupCgroupForIOThreads(virDomainObjPtr vm); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fc44ef1..58fa3c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4768,9 +4768,8 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, } if (cgroup_vcpu) { - if (qemuSetupCgroupVcpuPin(cgroup_vcpu, - vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, i) < 0) { + if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, + vcpupin->cpumask) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, _("failed to set cpuset.cpus in cgroup" " for vcpu %zu"), i); @@ -5136,8 +5135,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { if (virCgroupNewVcpu(priv->cgroup, vcpu, false, &cgroup_vcpu) < 0) goto endjob; - if (qemuSetupCgroupVcpuPin(cgroup_vcpu, newVcpuPin, newVcpuPinNum, - vcpu) < 0) { + if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, _("failed to set cpuset.cpus in cgroup" " for vcpu %d"), vcpu); -- 2.2.2
Use virBitmapNewCopy instead of a combination of virBitmapNew and virBitmapCopy. --- src/qemu/qemu_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 58fa3c6..71bbf6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4752,12 +4752,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, if (VIR_ALLOC(vcpupin) < 0) goto cleanup; - if (!(vcpupin->cpumask = - virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) { + if (!(vcpupin->cpumask = virBitmapNewCopy(vm->def->cpumask))) { VIR_FREE(vcpupin); goto cleanup; } - virBitmapCopy(vcpupin->cpumask, vm->def->cpumask); vcpupin->id = i; if (VIR_APPEND_ELEMENT_COPY(vm->def->cputune.vcpupin, vm->def->cputune.nvcpupin, vcpupin) < 0) { -- 2.2.2
On 03/27/2015 07:12 AM, Peter Krempa wrote:
Since commit a39f69d2b libvirt would fail to start a VM if the default cpu set was specified and individual vcpus were pinned to cpus outside of that cpuset.
Peter Krempa (8): qemu: cgroup: Store auto cpuset instead of re-creating it on demand qemu: cgroup: Refactor setup for IOThread cgroups qemu: cgroup: Properly set up vcpu pinning qemu: cgroup: Use priv->autoCpuset instead of using qemuPrepareCpumap() qemu: cgroup: Rename qemuSetupCgroupEmulatorPin to qemuSetupCgroupCpusetCpus qemu: cgroup: Kill qemuSetupCgroupIOThreadsPin() qemu: cgroup: Kill qemuSetupCgroupVcpuPin() qemu: Copy bitmap in a sane way
Not a review of this series, but related to cgroups so also needs fixing: Running ./autobuild.sh currently fails on the mingw-cross-compilation section, due to: CCLD libvirt.la Cannot export virCgroupDetectMountsFromFile: symbol not defined Cannot export virCgroupGetCpusetMemoryMigrate: symbol not defined Cannot export virCgroupSetCpusetMemoryMigrate: symbol not defined -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/27/2015 11:13 AM, Eric Blake wrote:
On 03/27/2015 07:12 AM, Peter Krempa wrote:
Since commit a39f69d2b libvirt would fail to start a VM if the default cpu set was specified and individual vcpus were pinned to cpus outside of that cpuset.
Peter Krempa (8): qemu: cgroup: Store auto cpuset instead of re-creating it on demand qemu: cgroup: Refactor setup for IOThread cgroups qemu: cgroup: Properly set up vcpu pinning qemu: cgroup: Use priv->autoCpuset instead of using qemuPrepareCpumap() qemu: cgroup: Rename qemuSetupCgroupEmulatorPin to qemuSetupCgroupCpusetCpus qemu: cgroup: Kill qemuSetupCgroupIOThreadsPin() qemu: cgroup: Kill qemuSetupCgroupVcpuPin() qemu: Copy bitmap in a sane way
Not a review of this series, but related to cgroups so also needs fixing:
Running ./autobuild.sh currently fails on the mingw-cross-compilation section, due to:
CCLD libvirt.la Cannot export virCgroupDetectMountsFromFile: symbol not defined Cannot export virCgroupGetCpusetMemoryMigrate: symbol not defined Cannot export virCgroupSetCpusetMemoryMigrate: symbol not defined
I "think" the following will resolve that (but I have no empirical evidence since I don't have a cross-mingw environment to test this on)... http://www.redhat.com/archives/libvir-list/2015-March/msg01480.html John
On Fri, Mar 27, 2015 at 02:12:04PM +0100, Peter Krempa wrote:
Since commit a39f69d2b libvirt would fail to start a VM if the default cpu set was specified and individual vcpus were pinned to cpus outside of that cpuset.
Peter Krempa (8): qemu: cgroup: Store auto cpuset instead of re-creating it on demand qemu: cgroup: Refactor setup for IOThread cgroups qemu: cgroup: Properly set up vcpu pinning qemu: cgroup: Use priv->autoCpuset instead of using qemuPrepareCpumap() qemu: cgroup: Rename qemuSetupCgroupEmulatorPin to qemuSetupCgroupCpusetCpus qemu: cgroup: Kill qemuSetupCgroupIOThreadsPin() qemu: cgroup: Kill qemuSetupCgroupVcpuPin() qemu: Copy bitmap in a sane way
src/qemu/qemu_cgroup.c | 150 +++++++++++++++--------------------------------- src/qemu/qemu_cgroup.h | 13 +---- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 21 +++---- src/qemu/qemu_process.c | 93 +++++++++--------------------- src/qemu/qemu_process.h | 2 - 7 files changed, 85 insertions(+), 198 deletions(-)
ACK series. Nice cleanup, that will show the NUMA topology witch! [1] Jan [1] https://www.redhat.com/archives/libvir-list/2015-March/msg01581.html
On Tue, Mar 31, 2015 at 16:14:53 +0200, Ján Tomko wrote:
On Fri, Mar 27, 2015 at 02:12:04PM +0100, Peter Krempa wrote:
Since commit a39f69d2b libvirt would fail to start a VM if the default cpu set was specified and individual vcpus were pinned to cpus outside of that cpuset.
Peter Krempa (8): qemu: cgroup: Store auto cpuset instead of re-creating it on demand qemu: cgroup: Refactor setup for IOThread cgroups qemu: cgroup: Properly set up vcpu pinning qemu: cgroup: Use priv->autoCpuset instead of using qemuPrepareCpumap() qemu: cgroup: Rename qemuSetupCgroupEmulatorPin to qemuSetupCgroupCpusetCpus qemu: cgroup: Kill qemuSetupCgroupIOThreadsPin() qemu: cgroup: Kill qemuSetupCgroupVcpuPin() qemu: Copy bitmap in a sane way
src/qemu/qemu_cgroup.c | 150 +++++++++++++++--------------------------------- src/qemu/qemu_cgroup.h | 13 +---- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 21 +++---- src/qemu/qemu_process.c | 93 +++++++++--------------------- src/qemu/qemu_process.h | 2 - 7 files changed, 85 insertions(+), 198 deletions(-)
ACK series.
Thanks. Now that the release is out I've pushed the patches. Peter
participants (4)
-
Eric Blake -
John Ferlan -
Ján Tomko -
Peter Krempa