On Wed, Jan 13, 2016 at 07:29:49AM -0500, John Ferlan wrote: > This reverts commit a41c00b472efaa192d2deae51ab732e65903238f. > > The patch was causing erroneous updates to the /proc/$pid/cgroup file. > This resulted in some unexpected behavoirs. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; ACK, Please also revert commit 71ce475967b3523882b0e1a0f649fdbea5abf9d5 Author: Henning Schild <henning.schild(a)siemens.com&gt; Date: Wed Dec 9 17:58:10 2015 +0100 util: cgroups do not implicitly add task to new machine cgroup Regards, Daniel

Reposting my cgroup fixes series: http://www.redhat.com/archives/libvir-list/2016-January/msg00236.html partially because I originally forgot to CC the author (Henning Schild) of the original series for which these patch fix a couple of issues discovered during regression testing (virt-test memtune failures in Red Hat regression environment), but also to bring them up to date with the top of libvirt git. NB: I did send Henning the changes after the fact, but my resend using the same message-id skills so that replies are left in the onlist series are lacking. Henning has looked at the first patch - with a response here: http://www.redhat.com/archives/libvir-list/2016-January/msg00443.html Finally, I think these changes should go into 1.3.1 since that's when the regression was introduced.

John Ferlan (4): cgroup: Fix possible bug as a result of code motion for vcpu cgroup setup qemu: Add check for NULL cgroup return from virCgroupNewMachine Revert "qemu: do not put a task into machine cgroup" qemu: Put the emulator cgroup pid into the right task file src/qemu/qemu_cgroup.c | 18 +++++++++++++----- src/qemu/qemu_process.c | 12 ++++++------ 2 files changed, 19 insertions(+), 11 deletions(-) -- 2.5.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 01/13/2016 11:51 AM, Martin Kletzander wrote: > On Wed, Jan 13, 2016 at 07:29:46AM -0500, John Ferlan wrote: >> Reposting my cgroup fixes series: >> >> http://www.redhat.com/archives/libvir-list/2016-January/msg00236.html >> >> partially because I originally forgot to CC the author (Henning Schild) >> of the original series for which these patch fix a couple of issues >> discovered during regression testing (virt-test memtune failures in >> Red Hat regression environment), but also to bring them up to date >> with the top of libvirt git. >> >> NB: I did send Henning the changes after the fact, but my resend using >> the same message-id skills so that replies are left in the onlist series >> are lacking. Henning has looked at the first patch - with a response >> here: >> >> http://www.redhat.com/archives/libvir-list/2016-January/msg00443.html >> >> Finally, I think these changes should go into 1.3.1 since that's when the >> regression was introduced. >> > > It would be nice to have them in, I really tried reviewing them, but I > can't wrap my head around last two of them. Maybe because I'm already > late for an appointment I have. > What I found happening is that by moving the virCgroupAddTask from qemuInitCgroup until later in qemuSetupCgroupForEmulator is that for "some reason" on (at least in the test environment) an older Fedora release (f20) that the /proc/$pid/cgroup file was updated "strangely". On a more recent Fedora release (f23) I didn't see the same phenomena. And by strangely - what I saw was even though the *SetupEmulator path was 'supposed to be' modifying only cpuset and cpu,cpuacct - other entries for memory, blkio, and devices were also updated - thus pointing at the wrong place (rather than the machine specific place). This caused the memtune test to fail. What was even stranger is that the code was updating the machine specific area when changing the values (e.g., internally we had the right path), but the test cannot see that so it uses the /proc/$pid/cgroup file to find the path.

On Wed, Jan 13, 2016 at 05:51:34PM +0100, Martin Kletzander wrote: > On Wed, Jan 13, 2016 at 07:29:46AM -0500, John Ferlan wrote: > >Reposting my cgroup fixes series: > > > >http://www.redhat.com/archives/libvir-list/2016-January/msg00236.html > > > >partially because I originally forgot to CC the author (Henning > >Schild) of the original series for which these patch fix a couple > >of issues discovered during regression testing (virt-test memtune > >failures in Red Hat regression environment), but also to bring > >them up to date with the top of libvirt git. > > > >NB: I did send Henning the changes after the fact, but my resend > >using the same message-id skills so that replies are left in the > >onlist series are lacking. Henning has looked at the first patch > >- with a response here: > > > >http://www.redhat.com/archives/libvir-list/2016-January/msg00443.html > > > >Finally, I think these changes should go into 1.3.1 since that's > >when the regression was introduced. > > > > It would be nice to have them in, I really tried reviewing them, > but I can't wrap my head around last two of them. Maybe because > I'm already late for an appointment I have. > > So unfortunately I have to leave you without the review for those > two as I really need to go, but anyone else feel free to continue. > And even re-check my reviews for 1 and 2 if you want. It would be > a pity not to fix a regression when we could. I agree we need to get these into the next release, but please hold off from merging them. I want to re-examine Henning's original patches in more detail, as I have a bad feeling we might need to simply revert all of them and start again.

On Wed, Jan 13, 2016 at 07:29:46AM -0500, John Ferlan wrote: > Reposting my cgroup fixes series: > > http://www.redhat.com/archives/libvir-list/2016-January/msg00236.html > > partially because I originally forgot to CC the author (Henning > Schild) of the original series for which these patch fix a couple > of issues discovered during regression testing (virt-test memtune > failures in Red Hat regression environment), but also to bring them > up to date with the top of libvirt git. > > NB: I did send Henning the changes after the fact, but my resend > using the same message-id skills so that replies are left in the > onlist series are lacking. Henning has looked at the first patch - > with a response here: > > http://www.redhat.com/archives/libvir-list/2016-January/msg00443.html > > Finally, I think these changes should go into 1.3.1 since that's > when the regression was introduced. Since this has been puzzelling us for a while, let me recap on the cgroup setup in general. First, I'll describe how it used to work *before* Henning's patches were merged, on a systemd based host. - The QEMU driver forks a child process, but does *not* exec QEMU yet The cgroup placement at this point is inherited from libvirtd. It may look like this: 10:freezer:/ 9:cpuset:/ 8:perf_event:/ 7:hugetlb:/ 6:blkio:/system.slice 5:memory:/system.slice 4:net_cls,net_prio:/ 3:devices:/system.slice/libvirtd.service 2:cpu,cpuacct:/system.slice 1:name=systemd:/system.slice/libvirtd.service - The QEMU driver calls virCgroupNewMachine() - We calll virSystemdCreateMachine with pidleader=$child - Systemd creates the initial machine scope unit under the machine slice unit, for the "systemd" controller. It may also add the PID to *zero* or more other resource controllers. So at this point the cgroup placement may look like this: 10:freezer:/ 9:cpuset:/ 8:perf_event:/ 7:hugetlb:/ 6:blkio:/ 5:memory:/ 4:net_cls,net_prio:/ 3:devices:/ 2:cpu,cpuacct:/ 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope Or may look like this: 10:freezer:/machine.slice/machine-qemu\x2dserial.scope 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope 6:blkio:/machine.slice/machine-qemu\x2dserial.scope 5:memory:/machine.slice/machine-qemu\x2dserial.scope 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope 3:devices:/machine.slice/machine-qemu\x2dserial.scope 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope Or anywhere in between. We have *ZERO* guarantee about what other resource controllers we may have been placed in by systemd. There is some fairly complex logic that determines this, based on what other tasks current exist in sibling cgroups, and what tasks have *previously* existed in the cgroups. IOW, you should consider the list of etra resource controllers essentially non-deterministic - We call virCgroupAddTask with pid=$child This places the pid in any resource controllers we need, which systemd has not already setup. IOW, it guarantees that we now have placement that should look like this, regardless of what systemd has done: 10:freezer:/machine.slice/machine-qemu\x2dserial.scope 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope 6:blkio:/machine.slice/machine-qemu\x2dserial.scope 5:memory:/machine.slice/machine-qemu\x2dserial.scope 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope 3:devices:/machine.slice/machine-qemu\x2dserial.scope 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope - The QEMU driver now lets the child process exec QEMU. QEMU creates its vCPU threads at this point. All QEMU threads (emulator, vcpu and I/O threads) now have the cgroup placement shown above. - We create the emulator cgroup for the cpuset, cpu, cpuacct controllers move all threads into this new cgroup. All threads (emulator, vcpu and I/O threads) thus now have placement of: 10:freezer:/machine.slice/machine-qemu\x2dserial.scope 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope/emulator 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope 6:blkio:/machine.slice/machine-qemu\x2dserial.scope 5:memory:/machine.slice/machine-qemu\x2dserial.scope 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope 3:devices:/machine.slice/machine-qemu\x2dserial.scope 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope/emulator 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope Yes, we really did move the vcpu threads into the emulator group... - We now ask QEMU which are the vCPU & I/O threads. - Foreach CPU thread we new vCPU cgroups and move them into this place 10:freezer:/machine.slice/machine-qemu\x2dserial.scope 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope/vcpuN 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope 6:blkio:/machine.slice/machine-qemu\x2dserial.scope 5:memory:/machine.slice/machine-qemu\x2dserial.scope 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope 3:devices:/machine.slice/machine-qemu\x2dserial.scope 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope/vpuN 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope - Foreach I/O thread we new vCPU cgroups and move them into this place 10:freezer:/machine.slice/machine-qemu\x2dserial.scope 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope/iothreadN 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope 6:blkio:/machine.slice/machine-qemu\x2dserial.scope 5:memory:/machine.slice/machine-qemu\x2dserial.scope 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope 3:devices:/machine.slice/machine-qemu\x2dserial.scope 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope/iothreadN 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope Now, lets Henning's three patches commit 71ce475967b3523882b0e1a0f649fdbea5abf9d5 Author: Henning Schild <henning.schild(a)siemens.com&gt; Date: Wed Dec 9 17:58:10 2015 +0100 util: cgroups do not implicitly add task to new machine cgroup This alters virCgroupNewMachine so that it no longer calls virCgroupAddTask. instead the callers are responsible for doing it. This is a functional no-opp change, so ostensibly harmless at this point. The second patch commit a41c00b472efaa192d2deae51ab732e65903238f Author: Henning Schild <henning.schild(a)siemens.com&gt; Date: Mon Dec 14 15:48:05 2015 -0500 qemu: do not put a task into machine cgroup This stops qemuInitCgroup from calling virCgroupAddTask. Instead it has added a call to vriCgroupAddTask in qemuSetupCgroupForEmulator(), which only affects the cpu, cpuset & cpuacct controllers. This means we no longer have any guarantee about which resource controllers the QEMU processes in general are in. All we can say is that they are in the 'systemd' controller, and the cpu, cpuset & cpuacct controllers. Systemd may have also placed it into *all* the other resource controllers, or it may have placed it into none of the them. As such, after this change, we are potentially not in the correct cgroup for the memory, blkio, netcls, devices controllers. For added fun, the qemuSetupCgroupForEmulator() still has a call to virCgroupMoveTask, so the addition of virCgroupAddTask to this method was useless. The final patch commit 90b721e43ec9232b5b218e891437bed04548e841 Author: Henning Schild <henning.schild(a)siemens.com&gt; Date: Mon Dec 14 15:58:05 2015 -0500 qemu cgroups: move new threads to new cgroup after cpuset is set up This delays the point at which we call virCgroupAddTask for the vCPU and I/O thread cgroups, until after we have configured properties on those cgroups. This change is fine So I think we need to revert the first 2 of Hennings patches - the 2nd one is the real serious problem, but once we revert that, the 1st change becomes pointless and so should also be reverted. Going back to the key problem Henning was trying to address.... The issue is that we only setup the emulator cgroup /after/ QEMU has already started running, so there's a period of time where threads are not confined by the cgroup affinity. I think we can solve that more simply by just moving the call to qemuSetupCgroupForEmulator(), into qemuSetupCgroup(). That way we place the emulator thread into the correct cgroup *before* we even exec QEMU. So we never have the window where we run in the unconfined cpuset controller.

On Thu, Jan 14, 2016 at 11:57:44AM +0000, Daniel P. Berrange wrote: > Since this has been puzzelling us for a while, let me recap on the > cgroup setup in general. > > First, I'll describe how it used to work *before* Henning's patches > were merged, on a systemd based host. > > - The QEMU driver forks a child process, but does *not* exec QEMU > yet The cgroup placement at this point is inherited from libvirtd. > It may look like this: > > 10:freezer:/ > 9:cpuset:/ > 8:perf_event:/ > 7:hugetlb:/ > 6:blkio:/system.slice > 5:memory:/system.slice > 4:net_cls,net_prio:/ > 3:devices:/system.slice/libvirtd.service > 2:cpu,cpuacct:/system.slice > 1:name=systemd:/system.slice/libvirtd.service > > - The QEMU driver calls virCgroupNewMachine() > > - We calll virSystemdCreateMachine with pidleader=$child > > - Systemd creates the initial machine scope unit under > the machine slice unit, for the "systemd" controller. > It may also add the PID to *zero* or more other > resource controllers. So at this point the cgroup > placement may look like this: > > 10:freezer:/ > 9:cpuset:/ > 8:perf_event:/ > 7:hugetlb:/ > 6:blkio:/ > 5:memory:/ > 4:net_cls,net_prio:/ > 3:devices:/ > 2:cpu,cpuacct:/ > 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope > > Or may look like this: > > 10:freezer:/machine.slice/machine-qemu\x2dserial.scope > 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope > 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope > 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope > 6:blkio:/machine.slice/machine-qemu\x2dserial.scope > 5:memory:/machine.slice/machine-qemu\x2dserial.scope > 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope > 3:devices:/machine.slice/machine-qemu\x2dserial.scope > 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope > 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope > > Or anywhere in between. We have *ZERO* guarantee about > what other resource controllers we may have been placed in by > systemd. There is some fairly complex logic that > determines this, based on what other tasks current exist in sibling > cgroups, and what tasks have *previously* existed in > the cgroups. IOW, you should consider the list of etra resource > controllers essentially non-deterministic > > - We call virCgroupAddTask with pid=$child > > This places the pid in any resource controllers we need, > which systemd has not already setup. IOW, it guarantees that we now > have placement that should look like this, regardless of > what systemd has done: > > 10:freezer:/machine.slice/machine-qemu\x2dserial.scope > 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope > 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope > 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope > 6:blkio:/machine.slice/machine-qemu\x2dserial.scope > 5:memory:/machine.slice/machine-qemu\x2dserial.scope > 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope > 3:devices:/machine.slice/machine-qemu\x2dserial.scope > 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope > 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope > > - The QEMU driver now lets the child process exec QEMU. QEMU > creates its vCPU threads at this point. All QEMU threads (emulator, > vcpu and I/O threads) now have the cgroup placement shown above. > > - We create the emulator cgroup for the cpuset, cpu, cpuacct > controllers move all threads into this new cgroup. All threads > (emulator, vcpu and I/O threads) thus now have placement of: > > 10:freezer:/machine.slice/machine-qemu\x2dserial.scope > 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope/emulator > 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope > 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope > 6:blkio:/machine.slice/machine-qemu\x2dserial.scope > 5:memory:/machine.slice/machine-qemu\x2dserial.scope > 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope > 3:devices:/machine.slice/machine-qemu\x2dserial.scope > 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope/emulator > 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope > > Yes, we really did move the vcpu threads into the emulator > group... > > - We now ask QEMU which are the vCPU & I/O threads. > > - Foreach CPU thread we new vCPU cgroups and move them into this > place > > 10:freezer:/machine.slice/machine-qemu\x2dserial.scope > 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope/vcpuN > 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope > 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope > 6:blkio:/machine.slice/machine-qemu\x2dserial.scope > 5:memory:/machine.slice/machine-qemu\x2dserial.scope > 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope > 3:devices:/machine.slice/machine-qemu\x2dserial.scope > 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope/vpuN > 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope > > - Foreach I/O thread we new vCPU cgroups and move them into this > place > > 10:freezer:/machine.slice/machine-qemu\x2dserial.scope > 9:cpuset:/machine.slice/machine-qemu\x2dserial.scope/iothreadN > 8:perf_event:/machine.slice/machine-qemu\x2dserial.scope > 7:hugetlb:/machine.slice/machine-qemu\x2dserial.scope > 6:blkio:/machine.slice/machine-qemu\x2dserial.scope > 5:memory:/machine.slice/machine-qemu\x2dserial.scope > 4:net_cls,net_prio:/machine.slice/machine-qemu\x2dserial.scope > 3:devices:/machine.slice/machine-qemu\x2dserial.scope > 2:cpu,cpuacct:/machine.slice/machine-qemu\x2dserial.scope/iothreadN > 1:name=systemd:/machine.slice/machine-qemu\x2dserial.scope BTW, on a slight tangent, the kernel is throwing a spanner in the works in the near future. They have just accepted cgroupv2 into mainline. Broadly speaking this is very nice because they got rid of the idea of separate mount point for each controller, and instead have a single filesystem tree. The problem is that they decided the granularity of placement is at a *process* level, not a *thread* level. So it will no longer be possible for us to have the cgroups for emulator, vcpus & i/o threads. Everything will have to live in the same cgroup :-( For cpu accounting and cpu affinity I think we can still achieve what we need by using a combination of cgroups and sched_setaffinity and /proc. I'm not sure what we'll do about per-thread schedular policies for period + quota though - not sure if there's an API for setting those or not ?!?! https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Docu...

Regards, Daniel