[libvirt] [PATCH v2 0/4] Adjustment to recent cgroup/cpuset changes (for 1.3.1)
v1: http://www.redhat.com/archives/libvir-list/2016-January/msg00511.html As discussed during the replies of the v1 - revert Henning's first two patches, plus the one I made as a result of those. Patch 4/4 is already ACK'd John Ferlan (4): Revert "qemu: do not put a task into machine cgroup" Revert "util: cgroups do not implicitly add task to new machine cgroup" Revert "lxc_cgroup: Add check for NULL cgroup before AddTask call" cgroup: Fix possible bug as a result of code motion for vcpu cgroup setup src/lxc/lxc_cgroup.c | 13 +------------ src/qemu/qemu_cgroup.c | 9 +-------- src/qemu/qemu_process.c | 12 ++++++------ src/util/vircgroup.c | 22 ++++++++++++++++++++++ 4 files changed, 30 insertions(+), 26 deletions(-) -- 2.5.0
This reverts commit a41c00b472efaa192d2deae51ab732e65903238f. After much testing and upstream discussion this has been deemed to be the incorrect operation since it means we no longer have any guarantee about which resource controllers the QEMU processes in general are in. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 15 +++++++++++---- src/qemu/qemu_process.c | 12 ++++++------ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1c406ce..88c1ce2 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -789,6 +789,17 @@ qemuInitCgroup(virQEMUDriverPtr driver, goto cleanup; } + if (virCgroupAddTask(priv->cgroup, vm->pid) < 0) { + virErrorPtr saved = virSaveLastError(); + virCgroupRemove(priv->cgroup); + virCgroupFree(&priv->cgroup); + if (saved) { + virSetError(saved); + virFreeError(saved); + } + goto cleanup; + } + done: ret = 0; cleanup: @@ -1160,10 +1171,6 @@ qemuSetupCgroupForEmulator(virDomainObjPtr vm) goto cleanup; } - /* consider the first thread an emulator-thread */ - if (virCgroupAddTask(cgroup_emulator, vm->pid) < 0) - goto cleanup; - virCgroupFree(&cgroup_emulator); return 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f083f3f..05cbda2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4895,6 +4895,12 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuSetupCgroup(driver, vm, nnicindexes, nicindexes) < 0) goto cleanup; + /* This must be done after cgroup placement to avoid resetting CPU + * affinity */ + if (!vm->def->cputune.emulatorpin && + qemuProcessInitCpuAffinity(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, @@ -4941,12 +4947,6 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuSetupCgroupForEmulator(vm) < 0) goto cleanup; - /* This must be done after cgroup placement to avoid resetting CPU - * affinity */ - if (!vm->def->cputune.emulatorpin && - qemuProcessInitCpuAffinity(vm) < 0) - goto cleanup; - VIR_DEBUG("Setting affinity of emulator threads"); if (qemuProcessSetEmulatorAffinity(vm) < 0) goto cleanup; -- 2.5.0
This reverts commit 71ce475967b3523882b0e1a0f649fdbea5abf9d5. Since commit id 'a41c00b47' has been reverted, this no longer is necessary Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_cgroup.c | 11 ----------- src/qemu/qemu_cgroup.c | 11 ----------- src/util/vircgroup.c | 22 ++++++++++++++++++++++ 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index ef332e3..8f78d24 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -504,17 +504,6 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, &cgroup) < 0 || !cgroup) goto cleanup; - if (virCgroupAddTask(cgroup, initpid) < 0) { - virErrorPtr saved = virSaveLastError(); - virCgroupRemove(cgroup); - virCgroupFree(&cgroup); - if (saved) { - virSetError(saved); - virFreeError(saved); - } - goto cleanup; - } - /* setup control group permissions for user namespace */ if (def->idmap.uidmap) { if (virCgroupSetOwner(cgroup, diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 88c1ce2..94b931f 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -789,17 +789,6 @@ qemuInitCgroup(virQEMUDriverPtr driver, goto cleanup; } - if (virCgroupAddTask(priv->cgroup, vm->pid) < 0) { - virErrorPtr saved = virSaveLastError(); - virCgroupRemove(priv->cgroup); - virCgroupFree(&priv->cgroup); - if (saved) { - virSetError(saved); - virFreeError(saved); - } - goto cleanup; - } - done: ret = 0; cleanup: diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 78f519c..7584ee4 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1669,6 +1669,16 @@ virCgroupNewMachineSystemd(const char *name, } } + if (virCgroupAddTask(*group, pidleader) < 0) { + virErrorPtr saved = virSaveLastError(); + virCgroupRemove(*group); + virCgroupFree(group); + if (saved) { + virSetError(saved); + virFreeError(saved); + } + } + ret = 0; cleanup: virCgroupFree(&parent); @@ -1691,6 +1701,7 @@ int virCgroupTerminateMachine(const char *name, static int virCgroupNewMachineManual(const char *name, const char *drivername, + pid_t pidleader, const char *partition, int controllers, virCgroupPtr *group) @@ -1716,6 +1727,16 @@ virCgroupNewMachineManual(const char *name, group) < 0) goto cleanup; + if (virCgroupAddTask(*group, pidleader) < 0) { + virErrorPtr saved = virSaveLastError(); + virCgroupRemove(*group); + virCgroupFree(group); + if (saved) { + virSetError(saved); + virFreeError(saved); + } + } + done: ret = 0; @@ -1762,6 +1783,7 @@ virCgroupNewMachine(const char *name, return virCgroupNewMachineManual(name, drivername, + pidleader, partition, controllers, group); -- 2.5.0
This reverts commit ae09988eb787df63d3bb298f713a3bbd77275901. Since commit id '71ce4759' has been reverted, this one is no longer necessary. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 8f78d24..ad254e4 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -501,7 +501,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, nnicindexes, nicindexes, def->resource->partition, -1, - &cgroup) < 0 || !cgroup) + &cgroup) < 0) goto cleanup; /* setup control group permissions for user namespace */ -- 2.5.0
Commit id '90b721e43' moved where the virCgroupAddTask was made until after the check for the vcpupin checks. However, in doing so it missed an option where if the cpumap didn't exist, then the code would continue back to the top of the current vcpu loop. The results was that the virCgroupAddTask wouldn't be called. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 94b931f..e41f461 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1079,10 +1079,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) } } - if (!cpumap) - continue; - - if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) + if (cpumap && qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) goto cleanup; } -- 2.5.0
On Thu, Jan 14, 2016 at 11:21:25AM -0500, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-January/msg00511.html
As discussed during the replies of the v1 - revert Henning's first two patches, plus the one I made as a result of those.
Patch 4/4 is already ACK'd
John Ferlan (4): Revert "qemu: do not put a task into machine cgroup" Revert "util: cgroups do not implicitly add task to new machine cgroup" Revert "lxc_cgroup: Add check for NULL cgroup before AddTask call" cgroup: Fix possible bug as a result of code motion for vcpu cgroup setup
ACK to all 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, 14 Jan 2016 16:42:12 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 14, 2016 at 11:21:25AM -0500, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-January/msg00511.html
As discussed during the replies of the v1 - revert Henning's first two patches, plus the one I made as a result of those.
Patch 4/4 is already ACK'd
John Ferlan (4): Revert "qemu: do not put a task into machine cgroup" Revert "util: cgroups do not implicitly add task to new machine cgroup" Revert "lxc_cgroup: Add check for NULL cgroup before AddTask call" cgroup: Fix possible bug as a result of code motion for vcpu cgroup setup
ACK to all
Same here! Daniel do you want to fix the "first qemu thread is no emulator" issue, or should i give it another try? Henning
On Thu, Jan 14, 2016 at 06:14:45PM +0100, Henning Schild wrote:
On Thu, 14 Jan 2016 16:42:12 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 14, 2016 at 11:21:25AM -0500, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-January/msg00511.html
As discussed during the replies of the v1 - revert Henning's first two patches, plus the one I made as a result of those.
Patch 4/4 is already ACK'd
John Ferlan (4): Revert "qemu: do not put a task into machine cgroup" Revert "util: cgroups do not implicitly add task to new machine cgroup" Revert "lxc_cgroup: Add check for NULL cgroup before AddTask call" cgroup: Fix possible bug as a result of code motion for vcpu cgroup setup
ACK to all
Same here!
Daniel do you want to fix the "first qemu thread is no emulator" issue, or should i give it another try?
If you send another patch I'll review it. As mentioned before, I think i'd suggest something as simple as calling qemuCgroupSetupEmulator from qemuInitCgroup will probably work ok 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, 14 Jan 2016 17:20:04 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 14, 2016 at 06:14:45PM +0100, Henning Schild wrote:
On Thu, 14 Jan 2016 16:42:12 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 14, 2016 at 11:21:25AM -0500, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-January/msg00511.html
As discussed during the replies of the v1 - revert Henning's first two patches, plus the one I made as a result of those.
Patch 4/4 is already ACK'd
John Ferlan (4): Revert "qemu: do not put a task into machine cgroup" Revert "util: cgroups do not implicitly add task to new machine cgroup" Revert "lxc_cgroup: Add check for NULL cgroup before AddTask call" cgroup: Fix possible bug as a result of code motion for vcpu cgroup setup
ACK to all
Same here!
Daniel do you want to fix the "first qemu thread is no emulator" issue, or should i give it another try?
If you send another patch I'll review it. As mentioned before, I think i'd suggest something as simple as calling qemuCgroupSetupEmulator from qemuInitCgroup will probably work ok
I will send one, but bare with me .. Other projects and vacation ;). Henning
participants (3)
-
Daniel P. Berrange -
Henning Schild -
John Ferlan