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"); > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list