[libvirt] [PATCH 0/2] Remove old buggy check when pinning iothreads/emulators
Peter Krempa (2): qemu: process: Move emulator thread setting code into one function qemu: Allow setting pinning of emulator/iohtread with automatic placement src/qemu/qemu_cgroup.c | 66 ----------------------------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_driver.c | 14 --------- src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++---------- 4 files changed, 62 insertions(+), 97 deletions(-) -- 2.6.2
Similarly to the refactors to iothreads and vcpus, move the code that initializes the emulator thread settings into single function. --- src/qemu/qemu_cgroup.c | 66 ----------------------------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 62 insertions(+), 83 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f3c5fbb..a294bb0 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1062,72 +1062,6 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, int -qemuSetupCgroupForEmulator(virDomainObjPtr vm) -{ - virBitmapPtr cpumask = NULL; - virCgroupPtr cgroup_emulator = NULL; - virDomainDefPtr def = vm->def; - qemuDomainObjPrivatePtr priv = vm->privateData; - unsigned long long period = vm->def->cputune.emulator_period; - long long quota = vm->def->cputune.emulator_quota; - - if ((period || quota) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cgroup cpu is required for scheduler tuning")); - return -1; - } - - /* - * If CPU cgroup controller is not initialized here, then we need - * neither period nor quota settings. And if CPUSET controller is - * not initialized either, then there's nothing to do anyway. - */ - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) - return 0; - - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, - true, &cgroup_emulator) < 0) - goto cleanup; - - if (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 0) - goto cleanup; - - if (def->cputune.emulatorpin) - cpumask = def->cputune.emulatorpin; - else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - cpumask = priv->autoCpuset; - else if (def->cpumask) - cpumask = def->cpumask; - - 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; - } - - virCgroupFree(&cgroup_emulator); - return 0; - - cleanup: - if (cgroup_emulator) { - virCgroupRemove(cgroup_emulator); - virCgroupFree(&cgroup_emulator); - } - - return -1; -} - - -int qemuRemoveCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index a8b8e1b..7a9d10a 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -54,7 +54,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, unsigned long long period, long long quota); int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); -int qemuSetupCgroupForEmulator(virDomainObjPtr vm); int qemuRemoveCgroup(virDomainObjPtr vm); #endif /* __QEMU_CGROUP_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e760182..07b7d63 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2185,22 +2185,72 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, } -/* Set CPU affinities for emulator threads. */ static int -qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) +qemuProcessSetupEmulator(virDomainObjPtr vm) { - virBitmapPtr cpumask; - virDomainDefPtr def = vm->def; + 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 (def->cputune.emulatorpin) - cpumask = def->cputune.emulatorpin; - else if (def->cpumask) - cpumask = def->cpumask; + 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 - return 0; + 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 (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 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); + } - ret = virProcessSetAffinity(vm->pid, cpumask); return ret; } @@ -5135,12 +5185,8 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup; - VIR_DEBUG("Setting cgroup for emulator (if required)"); - if (qemuSetupCgroupForEmulator(vm) < 0) - goto cleanup; - - VIR_DEBUG("Setting affinity of emulator threads"); - if (qemuProcessSetEmulatorAffinity(vm) < 0) + VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0) goto cleanup; VIR_DEBUG("Waiting for monitor to show up"); -- 2.6.2
On 02/24/2016 08:45 AM, Peter Krempa wrote:
Similarly to the refactors to iothreads and vcpus, move the code that initializes the emulator thread settings into single function. --- src/qemu/qemu_cgroup.c | 66 ----------------------------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 62 insertions(+), 83 deletions(-)
This is going to conflict with Henning Schild's series: http://www.redhat.com/archives/libvir-list/2016-February/msg01157.html In particular: http://www.redhat.com/archives/libvir-list/2016-February/msg01166.html
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f3c5fbb..a294bb0 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1062,72 +1062,6 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup,
int -qemuSetupCgroupForEmulator(virDomainObjPtr vm) -{ - virBitmapPtr cpumask = NULL; - virCgroupPtr cgroup_emulator = NULL; - virDomainDefPtr def = vm->def; - qemuDomainObjPrivatePtr priv = vm->privateData; - unsigned long long period = vm->def->cputune.emulator_period; - long long quota = vm->def->cputune.emulator_quota; - - if ((period || quota) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cgroup cpu is required for scheduler tuning")); - return -1; - } - - /* - * If CPU cgroup controller is not initialized here, then we need - * neither period nor quota settings. And if CPUSET controller is - * not initialized either, then there's nothing to do anyway. - */ - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && - !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) - return 0; - - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, - true, &cgroup_emulator) < 0) - goto cleanup; - - if (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 0) - goto cleanup; - - if (def->cputune.emulatorpin) - cpumask = def->cputune.emulatorpin; - else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - cpumask = priv->autoCpuset; - else if (def->cpumask) - cpumask = def->cpumask; - - 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; - } - - virCgroupFree(&cgroup_emulator); - return 0; - - cleanup: - if (cgroup_emulator) { - virCgroupRemove(cgroup_emulator); - virCgroupFree(&cgroup_emulator); - } - - return -1; -} - - -int qemuRemoveCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index a8b8e1b..7a9d10a 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -54,7 +54,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, unsigned long long period, long long quota); int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); -int qemuSetupCgroupForEmulator(virDomainObjPtr vm); int qemuRemoveCgroup(virDomainObjPtr vm);
#endif /* __QEMU_CGROUP_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e760182..07b7d63 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2185,22 +2185,72 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, }
-/* Set CPU affinities for emulator threads. */ static int -qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) +qemuProcessSetupEmulator(virDomainObjPtr vm) { - virBitmapPtr cpumask; - virDomainDefPtr def = vm->def; + 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 (def->cputune.emulatorpin) - cpumask = def->cputune.emulatorpin; - else if (def->cpumask) - cpumask = def->cpumask; + 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 - return 0; + 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 (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 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); + }
- ret = virProcessSetAffinity(vm->pid, cpumask); return ret; }
@@ -5135,12 +5185,8 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup;
- VIR_DEBUG("Setting cgroup for emulator (if required)"); - if (qemuSetupCgroupForEmulator(vm) < 0) - goto cleanup; - - VIR_DEBUG("Setting affinity of emulator threads"); - if (qemuProcessSetEmulatorAffinity(vm) < 0) + VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0)
Interesting to note in the other series the SetupCgroupForEmulator was moved to right after qemuSetupCgroup - whether that's exactly right, I'm not sure; however, is it reasonable to move the new call to below qemuProcessInitCpuAffinity? Before the labeling starts? There perhaps is even more overlap between those two since the InitCpuAffinity will virProcessSetAffinity for vm->pid just like the new qemuProcessSetupEmulator. Hopefully you, Dan, and Henning can work all this out... John
goto cleanup;
VIR_DEBUG("Waiting for monitor to show up");
On Thu, Feb 25, 2016 at 18:18:09 -0500, John Ferlan wrote:
On 02/24/2016 08:45 AM, Peter Krempa wrote:
Similarly to the refactors to iothreads and vcpus, move the code that initializes the emulator thread settings into single function. --- src/qemu/qemu_cgroup.c | 66 ----------------------------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 62 insertions(+), 83 deletions(-)
This is going to conflict with Henning Schild's series:
http://www.redhat.com/archives/libvir-list/2016-February/msg01157.html
In particular:
http://www.redhat.com/archives/libvir-list/2016-February/msg01166.html
Well, that one is trivial enough, but I'll redo this change after that patch goes in, so that the bug gets fixed and then I'll refactor it. Peter
On 02/24/2016 08:45 AM, Peter Krempa wrote:
Similarly to the refactors to iothreads and vcpus, move the code that initializes the emulator thread settings into single function. --- src/qemu/qemu_cgroup.c | 66 ----------------------------------------- src/qemu/qemu_cgroup.h | 1 - src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 62 insertions(+), 83 deletions(-)
Since it seems Henning's series is waiting for this... At the time I wasn't sure which would go first, but wanted to be sure we had the right adjustments. My main hangup was when the call would be made (after the exec or in between fork/exec as was done in the other series). Since the other patch will move the call into the fork/exec window, this is fine... ACK John
We honour the placement bitmaps when starting up, so there's no point in having this check. Additionally the check was buggy since it checked vm->def all the time even if the user requested to modify the persistent definition which had different configuration. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1308317 --- src/qemu/qemu_driver.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45ff3c0..ae75694 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5240,13 +5240,6 @@ qemuDomainPinEmulator(virDomainPtr dom, if (virDomainPinEmulatorEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Changing affinity for emulator thread dynamically " - "is not allowed when CPU placement is 'auto'")); - goto cleanup; - } - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -5714,13 +5707,6 @@ qemuDomainPinIOThread(virDomainPtr dom, if (virDomainPinIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Changing affinity for IOThread dynamically is " - "not allowed when CPU placement is 'auto'")); - goto cleanup; - } - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; -- 2.6.2
$SUBJ "emulator/iothread" On 02/24/2016 08:45 AM, Peter Krempa wrote:
We honour the placement bitmaps when starting up, so there's no point in having this check. Additionally the check was buggy since it checked vm->def all the time even if the user requested to modify the persistent definition which had different configuration.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1308317 --- src/qemu/qemu_driver.c | 14 -------------- 1 file changed, 14 deletions(-)
Seem reasonable ACK John
participants (2)
-
John Ferlan -
Peter Krempa