Moving tasks to cgroups implied sched_setaffinity. Changing the cpus in a set implies the same for all tasks in the group. The old code put the the thread into the cpuset inherited from the machine cgroup, which allowed it to run outside of vcpupin for a short while. Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; --- src/qemu/qemu_cgroup.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)

On 11/13/2015 11:56 AM, Henning Schild wrote: > Hi, > > i already explained some of the cgroup problems in some detail so i > will not do that again. > https://www.redhat.com/archives/libvir-list/2015-October/msg00876.html > > I managed to solve some of the problems in the current codebase, and > am now sharing the patches. But they are really just half of what i > had to change to get libvirt to behave in a system with isolated > cpus. > > Other changes/hacks i am not sending here because they do not work > for the general case: > - create machine.slice before starting libvirtd (smaller than root) > ... and hope it wont grow > - disabling cpuset.cpus inheritance in libvirtd > - allowing only xml with fully specified cputune > - set machine cpuset to (vcpupins | emulatorpin) > > I am not sure how useful the individual fixes are, i am sending them > as concrete examples for the problems i described earlier. And i am > hoping that will start a discussion. > > Henning > > Henning Schild (3): > util: cgroups do not implicitly add task to new machine cgroup > qemu: do not put a task into machine cgroup > qemu cgroups: move new threads to new cgroup after cpuset is set > up > > src/lxc/lxc_cgroup.c | 6 ++++++ > src/qemu/qemu_cgroup.c | 23 ++++++++++++++--------- > src/util/vircgroup.c | 22 ---------------------- > 3 files changed, 20 insertions(+), 31 deletions(-) > The updated code looks fine to me - although it didn't directly git am -3 to top of tree - I was able to make a few adjustments to get things merged... Since no one has objected to this ordering change - I've pushed.

On 12/21/2015 03:36 AM, Henning Schild wrote: > On Mon, 14 Dec 2015 16:27:54 -0500 > John Ferlan <jferlan(a)redhat.com&gt; wrote: > >> >> >> On 11/13/2015 11:56 AM, Henning Schild wrote: >>> Hi, >>> >>> i already explained some of the cgroup problems in some detail so >>> i will not do that again. >>> https://www.redhat.com/archives/libvir-list/2015-October/msg00876.html >>> >>> I managed to solve some of the problems in the current codebase, >>> and am now sharing the patches. But they are really just half of >>> what i had to change to get libvirt to behave in a system with >>> isolated cpus. >>> >>> Other changes/hacks i am not sending here because they do not work >>> for the general case: >>> - create machine.slice before starting libvirtd (smaller than >>> root) ... and hope it wont grow >>> - disabling cpuset.cpus inheritance in libvirtd >>> - allowing only xml with fully specified cputune >>> - set machine cpuset to (vcpupins | emulatorpin) >>> >>> I am not sure how useful the individual fixes are, i am sending >>> them as concrete examples for the problems i described earlier. >>> And i am hoping that will start a discussion. >>> >>> Henning >>> >>> Henning Schild (3): >>> util: cgroups do not implicitly add task to new machine cgroup >>> qemu: do not put a task into machine cgroup >>> qemu cgroups: move new threads to new cgroup after cpuset is set >>> up >>> >>> src/lxc/lxc_cgroup.c | 6 ++++++ >>> src/qemu/qemu_cgroup.c | 23 ++++++++++++++--------- >>> src/util/vircgroup.c | 22 ---------------------- >>> 3 files changed, 20 insertions(+), 31 deletions(-) >>> >> >> >> The updated code looks fine to me - although it didn't directly >> git am -3 to top of tree - I was able to make a few adjustments to >> get things merged... Since no one has objected to this ordering >> change - I've pushed. > > Sorry the patches where still based on v1.2.19. Thanks for the merge > and accepting them! > No problem - although it seems they've generated a regression in the virttest memtune test suite. I'm 'technically' on vacation for the next couple of weeks; however, I think/perhaps the problem is a result of this patch and the change to adding the task to the cgroup at the end of the for loop, but perhaps the following code causes the control to jump back to the top of the loop: if (!cpumap) continue; if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) goto cleanup; not allowing the /* move the thread for vcpu to sub dir */ if (virCgroupAddTask(cgroup_vcpu, qemuDomainGetVcpuPid(vm, i)) < 0) goto cleanup; to be executed. The code should probably change to be (like IOThreads): if (cpumap && qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) goto cleanup; As for the rest, I suspect things will be quite quiet around here over the next couple of weeks. A discussion to perhaps start in the new year.

John > Wrong operation ordering within libvirt cgroups (like the ones > fixed by the patches) could still push tasks onto dedicated cpus. > And more importantly other cgroups users can still grab the > dedicated cpus as well. The only reliable solution to prevent that > seems to be making use of the "exclusive" feature of cpusets. And > that would imply changing the cgroups layout of libvirt again. > Because sets can not be partially exclusive and libvirt deals with > dedicated cpus and shared ones. > How to deal with these problems is a discussion that i wanted to get > started with this patch-series. It would be nice to receive general > comments on that. How should we proceed here? I could maybe write an > RFC mail describing the problems again and suggesting changes to > libvirt on a conceptual basis. > > But until then maybe people responsible for cgroups in libvirt (Paul > and Martin?) can again look at > https://www.redhat.com/archives/libvir-list/2015-October/msg00876.html > There i described how naive use of cgoups can place tasks on cpus > that are supposed to be isolated/dedicated/exclusive. Even if > libvirt does not make these mistakes it should protect itself > against docker, systemd, ... > > Henning > >

[...] >> No problem - although it seems they've generated a regression in >> the virttest memtune test suite. I'm 'technically' on vacation >> for the next couple of weeks; however, I think/perhaps the problem >> is a result of this patch and the change to adding the task to the >> cgroup at the end of the for loop, but perhaps the following code >> causes the control to jump back to the top of the loop: >> >> if (!cpumap) >> continue; >> >> if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < >> 0) goto cleanup; >> >> not allowing the >> >> >> /* move the thread for vcpu to sub dir */ >> if (virCgroupAddTask(cgroup_vcpu, >> qemuDomainGetVcpuPid(vm, i)) < 0) >> goto cleanup; >> >> to be executed. >> >> The code should probably change to be (like IOThreads): >> >> if (cpumap && >> qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < >> 0) goto cleanup; >> >> >> As for the rest, I suspect things will be quite quiet around here >> over the next couple of weeks. A discussion to perhaps start in >> the new year. > > Same here. I will have a look at that regression after my vacation, > should it still be there. > > Henning > More data from the issue... While the above mentioned path is an issue, I don't believe it's what's causing the test failure. I haven't quite figured out why yet, but it seems the /proc/#/cgroup file isn't getting the proper path for the 'memory' slice and thus the test fails because it's looking at the: /sys/fs/cgroup/memory/machine.slice/memory.* files instead of the /sys/fs/cgroup/memory/machine.slice/$path/memory.*

Where $path is "machine-qemu\x2dvirt\x2dtests\x2dvm1.scope" This affects the virsh memtune $dom command test suite which uses the /proc/$pid/cgroup file in order to find the path for the 'memory' or 'cpuset' or 'cpu,cpuacct' cgroup paths. Seems to be some interaction with systemd that I have quite figured out. I'm assuming this is essentially the issue you were trying to fix - that is changes to values should be done to the machine-qemu* specific files rather than the machine.slice files. The good news is I can see the changes occurring in the machine-qemu* specific files, so it seems libvirt is doing the right thing. However, there's something strange with perhaps previously existing/running domains where that /proc/$pid/cgroup file doesn't get the $path for the memory entry, thus causing the test validation to look in the wrong place. Hopefully this makes sense. What's really strange (for me at least) is that it's only occurring on one test system. I can set up the same test on another system and things work just fine. I'm not quite sure what interaction generates that /proc/$pid/cgroup file - hopefully someone else understands it and help me make sense of it.

On 01/07/2016 02:01 PM, Henning Schild wrote: > On Thu, 7 Jan 2016 11:20:23 -0500 > John Ferlan <jferlan(a)redhat.com&gt; wrote: > >> >> [...] >> >>>> No problem - although it seems they've generated a regression in >>>> the virttest memtune test suite. I'm 'technically' on vacation >>>> for the next couple of weeks; however, I think/perhaps the >>>> problem is a result of this patch and the change to adding the >>>> task to the cgroup at the end of the for loop, but perhaps the >>>> following code causes the control to jump back to the top of the >>>> loop: >>>> >>>> if (!cpumap) >>>> continue; >>>> >>>> if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) >>>> < 0) goto cleanup; >>>> >>>> not allowing the >>>> >>>> >>>> /* move the thread for vcpu to sub dir */ >>>> if (virCgroupAddTask(cgroup_vcpu, >>>> qemuDomainGetVcpuPid(vm, i)) < 0) >>>> goto cleanup; >>>> >>>> to be executed. >>>> >>>> The code should probably change to be (like IOThreads): >>>> >>>> if (cpumap && >>>> qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < >>>> 0) goto cleanup; >>>> >>>> >>>> As for the rest, I suspect things will be quite quiet around here >>>> over the next couple of weeks. A discussion to perhaps start in >>>> the new year. >>> >>> Same here. I will have a look at that regression after my >>> vacation, should it still be there. >>> >>> Henning >>> >> >> More data from the issue... While the above mentioned path is an >> issue, I don't believe it's what's causing the test failure. >> >> I haven't quite figured out why yet, but it seems >> the /proc/#/cgroup file isn't getting the proper path for the >> 'memory' slice and thus the test fails because it's looking at the: >> >> /sys/fs/cgroup/memory/machine.slice/memory.* >> >> files instead of the >> >> /sys/fs/cgroup/memory/machine.slice/$path/memory.* > > To be honest i did just look at the cgroup/cpuset/ hierarchy, but i > just browsed cgroup/memory/ as well. > > The target of my patch series was to get > cgroup/cpuset/machine.slice/tasks to be emtpy, all tasks should be > in their sub-cgroup under the machine.slice. And the ordering > patches make sure the file is always empty. > > In the memory cgroups all tasks are in the parent group (all in > machine.slice/tasks). machine.slice/*/tasks are empty. I am not sure > whether that is intended, i can just assume it is a bug in the > memory cgroup subsystem. Why are the groups created and tuned when > the tasks stay in the big superset? TBH - there's quite a bit of this that mystifies me... Use of cgroups is not something I've spent a whole lot of time looking at... I guess I've been working under the assumption that when the machine.slice/$path is created, the domain would use that for all cgroup specific file adjustments for that domain. Not sure how the /proc/$pid/cgroup is related to this. My f23 system seems to generate the /proc/$pid/cgroup with the machine.slice/$path/ for each of the cgroups libvirt cares about while the f20 system with the test only has that path for cpuset and cpu,cpuacct. Since that's what the test uses for to find the memory path for validation that's why it fails. I've been looking through the libvirtd debug logs to see if anything jumps out at me, but it seems both the systems I've looked at will build the path for the domain using the machine.slice/$path as seen during domain startup. Very odd - perhaps looking at it too long right now though! > /proc/#/cgroup is showing the correct path, libvirt seems to fail to > migrate tasks into memory subgroups. (i am talking about a patched > 1.2.19 where vms do not have any special memory tuning) I'm using latest upstream 1.3.1 - it seems to set the machine.slice/$path for blkio, cpu,cpuacct, cpuset, memory, and devices entries. > > Without my patches the first qemu thread was in > "2:cpuset:/machine.slice" and the name did match > "4:memory:/machine.slice". Now if the test wants matching names the > test might just be wrong. Or as indicated before there might be a > bug in the memory cgroups. > I'm leaning towards something in the test. I'll check if reverting these changes alters the results. I don't imagine it will.

John >> Where $path is "machine-qemu\x2dvirt\x2dtests\x2dvm1.scope" >> >> This affects the virsh memtune $dom command test suite which uses >> the /proc/$pid/cgroup file in order to find the path for the >> 'memory' or 'cpuset' or 'cpu,cpuacct' cgroup paths. >> >> Seems to be some interaction with systemd that I have quite figured >> out. >> >> I'm assuming this is essentially the issue you were trying to fix - >> that is changes to values should be done to the machine-qemu* >> specific files rather than the machine.slice files. >> >> The good news is I can see the changes occurring in the >> machine-qemu* specific files, so it seems libvirt is doing the >> right thing. >> >> However, there's something strange with perhaps previously >> existing/running domains where that /proc/$pid/cgroup file doesn't >> get the $path for the memory entry, thus causing the test >> validation to look in the wrong place. >> >> Hopefully this makes sense. What's really strange (for me at >> least) is that it's only occurring on one test system. I can set >> up the same test on another system and things work just fine. I'm >> not quite sure what interaction generates that /proc/$pid/cgroup >> file - hopefully someone else understands it and help me make >> sense of it. >

> > I'm leaning towards something in the test. I'll check if reverting > these changes alters the results. I don't imagine it will. The real question is which thread it fails on and at what point in time. My patches only changed the order of operations where threads enter the cpuset cgroups at a slightly different time. And the qemu main thread never enters the parent group, it becomes an emulator-thread. Maybe you can point to exactly the assertion that fails. Including a link to the test code. And yes if you can confirm that the patches are to blame that would be a good first step ;). Thanks, Henning

On Fri, 8 Jan 2016 11:05:59 -0500 John Ferlan <jferlan(a)redhat.com&gt; wrote: > >>> >>> I'm leaning towards something in the test. I'll check if reverting >>> these changes alters the results. I don't imagine it will. >> >> The real question is which thread it fails on and at what point in >> time. My patches only changed the order of operations where threads >> enter the cpuset cgroups at a slightly different time. And the qemu >> main thread never enters the parent group, it becomes an >> emulator-thread. Maybe you can point to exactly the assertion that >> fails. Including a link to the test code. And yes if you can >> confirm that the patches are to blame that would be a good first >> step ;). >> >> Thanks, >> Henning >> > > Update: > > I have found that if I revert patch 2... > > Then modify qemuInitCgroup() to modify the virCgroupNewMachine check > to also ensure "|| !priv->cgroup) I see the check for the parent cgroup should probably go back into virCgroupNewMachine, including the cleanup stuff in case of failure.

> Then modify qemuSetupCgroupForEmulator() to make the > virCgroupAddTask() call like was in patch 2 > > Then modify patch 3 (qemuSetupCgroupForVcpu) to change the call: > > > if (!cpumap) > continue; > > if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) > goto cleanup; > > to > > if (cpumap && > qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) > goto cleanup; > Well that is not a syntactical change, maybe easier to read and in line with the other places where qemuSetupCgroupCpusetCpus is called. > Then retest and the test passes again. > > Note that taking this route, I found that when I start the guest, I > have the following in 'tasks': > > # cat /sys/fs/cgroup/memory/machine.slice/tasks > # cat /sys/fs/cgroup/memory/machine.slice/*/tasks > 15007 > 15008 > 15010 > 15011 > 15013 > # > > Where '15007' is the virt-tests-vm1 process (eg, /proc/$pid/cgroup). > If I read the intentions you had, this follows that... > > I'll post a couple of patches in a bit... > > John