10:21 a.m.
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
10:21 a.m.
New subject: [libvirt] [PATCH v2 1/4] Revert "qemu: do not put a task into machine cgroup"
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(a)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
10:21 a.m.
New subject: [libvirt] [PATCH v2 2/4] Revert "util: cgroups do not implicitly add task to new machine cgroup"
This reverts commit 71ce475967b3523882b0e1a0f649fdbea5abf9d5.
Since commit id 'a41c00b47' has been reverted, this no longer is
necessary
Signed-off-by: John Ferlan <jferlan(a)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
10:21 a.m.
New subject: [libvirt] [PATCH v2 3/4] Revert "lxc_cgroup: Add check for NULL cgroup before AddTask call"
This reverts commit ae09988eb787df63d3bb298f713a3bbd77275901.
Since commit id '71ce4759' has been reverted, this one is no
longer necessary.
Signed-off-by: John Ferlan <jferlan(a)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
10:21 a.m.
New subject: [libvirt] [PATCH v2 4/4] cgroup: Fix possible bug as a result of code motion for vcpu cgroup setup
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(a)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
10:42 a.m.
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 :|
11:14 a.m.
On Thu, 14 Jan 2016 16:42:12 +0000
"Daniel P. Berrange" <berrange(a)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
11:20 a.m.
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(a)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 :|
11:37 a.m.
On Thu, 14 Jan 2016 17:20:04 +0000
"Daniel P. Berrange" <berrange(a)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(a)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
3264
days inactive
3265
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Henning Schild -
John Ferlan