Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; --- make-kpkg | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/make-kpkg b/make-kpkg index 5cb8ec3..ba663c9 100755 --- a/make-kpkg +++ b/make-kpkg @@ -662,9 +662,10 @@ sub main () { } if ( $config_target - !~ /^(?:|silentold|old|menu|n|x|g|rand|def|all(mod|yes|no))(config)?$/ ) { + !~ /^(?:|silentold|old|olddef|menu|n|x|g|rand|def|all(mod|yes|no))(config)?$/ ) { print - "Config type must be one of {config,silentoldconfig,oldconfig,menuconfig,xconfig,\n"; + "Config type must be one of {config,silentoldconfig,oldconfig,olddefconfig,menuconfig," + . "xconfig,\n"; print " nconfig,gconfig,randconfig,defconfig,allmodconfigallyesconfig,allnoconfig}.\n"; print "use --help to display command line syntax help.\n";

Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; --- make-kpkg | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/make-kpkg b/make-kpkg index 5cb8ec3..ba663c9 100755 --- a/make-kpkg +++ b/make-kpkg @@ -662,9 +662,10 @@ sub main () { } if ( $config_target - !~ /^(?:|silentold|old|menu|n|x|g|rand|def|all(mod|yes|no))(config)?$/ ) { + !~ /^(?:|silentold|old|olddef|menu|n|x|g|rand|def|all(mod|yes|no))(config)?$/ ) { print - "Config type must be one of {config,silentoldconfig,oldconfig,menuconfig,xconfig,\n"; + "Config type must be one of {config,silentoldconfig,oldconfig,olddefconfig,menuconfig," + . "xconfig,\n"; print " nconfig,gconfig,randconfig,defconfig,allmodconfigallyesconfig,allnoconfig}.\n"; print "use --help to display command line syntax help.\n";

On 02/23/2016 10:58 AM, Henning Schild wrote: > When using a hierarchy of cgroups we might want to add tasks just to > the children cgroups but never to the parent. To make sure we do not > use a parent cgroup by accident add a mechanism that lets us assert > a correct implementation in cases we want such a hierarchy. > > i.e. for qemu cpusets we want all tasks in /vcpuX or /emulator, not > in /. > > Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; > --- > src/libvirt_private.syms | 2 ++ > src/util/vircgroup.c | 19 +++++++++++++++++++ > src/util/vircgroup.h | 3 +++ > src/util/vircgrouppriv.h | 1 + > 4 files changed, 25 insertions(+) > These aren't used until patch 9 - I think this should be closer to that patch... That is introduce it just before you use it.. > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 83f6e2c..cf93d06 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1201,6 +1201,7 @@ virCgroupDenyDeviceMajor; > virCgroupDenyDevicePath; > virCgroupDetectMountsFromFile; > virCgroupFree; > +virCgroupGetAssertEmpty; > virCgroupGetBlkioDeviceReadBps; > virCgroupGetBlkioDeviceReadIops; > virCgroupGetBlkioDeviceWeight; > @@ -1245,6 +1246,7 @@ virCgroupNewThread; > virCgroupPathOfController; > virCgroupRemove; > virCgroupRemoveRecursively; > +virCgroupSetAssertEmpty; > virCgroupSetBlkioDeviceReadBps; > virCgroupSetBlkioDeviceReadIops; > virCgroupSetBlkioDeviceWeight; > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 0b65238..ad46dfc 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -1197,6 +1197,15 @@ virCgroupAddTaskController(virCgroupPtr > group, pid_t pid, int controller) return -1; > } > > + if(group->assert_empty & (1 << controller)) { should be : if (group... > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Controller '%s' is not supposed to > contain any" > + " tasks. group=%s pid=%d\n"), > + virCgroupControllerTypeToString(controller), > + group->path, pid); > + return -1; > + } > + > return virCgroupSetValueU64(group, controller, "tasks", > (unsigned long long)pid); > } > @@ -1246,6 +1255,16 @@ virCgroupAddTaskStrController(virCgroupPtr > group, return rc; > } > Need to have some code comments regarding input and what these do... > +void > +virCgroupSetAssertEmpty(virCgroupPtr group, int mask) { > + group->assert_empty = mask; > +} > + > +int > +virCgroupGetAssertEmpty(virCgroupPtr group) { > + return group->assert_empty; > +} > + > You'll need to add the corresponding API in the "#else /* !VIR_CGROUP_SUPPORTED */" area... Search on virCgroupAddTaskController - you'll find a second entry in the module which reports a system error. That's what you'll need to add for these John > /** > * virCgroupMoveTask: > diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h > index 63a9e1c..f244c24 100644 > --- a/src/util/vircgroup.h > +++ b/src/util/vircgroup.h > @@ -131,6 +131,9 @@ int virCgroupAddTaskController(virCgroupPtr > group, pid_t pid, > int controller); > > +void virCgroupSetAssertEmpty(virCgroupPtr group, int mask); > +int virCgroupGetAssertEmpty(virCgroupPtr group); > + > int virCgroupMoveTask(virCgroupPtr src_group, > virCgroupPtr dest_group); > > diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h > index 722863e..944d6ae 100644 > --- a/src/util/vircgrouppriv.h > +++ b/src/util/vircgrouppriv.h > @@ -44,6 +44,7 @@ struct virCgroupController { > > struct virCgroup { > char *path; > + int assert_empty; > > struct virCgroupController > controllers[VIR_CGROUP_CONTROLLER_LAST]; }; >

On 02/23/2016 10:58 AM, Henning Schild wrote: > When using a cgroups hierarchy threads have child cgroups for > certain controllers. Introduce an enum for later reuse. > > Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; > --- > src/util/vircgroup.c | 12 +++--------- > src/util/vircgroup.h | 7 +++++++ > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index ad46dfc..11f33ab 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -277,9 +277,7 @@ virCgroupValidateMachineGroup(virCgroupPtr > group, goto cleanup; > > if (stripEmulatorSuffix && > - (i == VIR_CGROUP_CONTROLLER_CPU || > - i == VIR_CGROUP_CONTROLLER_CPUACCT || > - i == VIR_CGROUP_CONTROLLER_CPUSET)) { > + (i & VIR_CGROUP_THREAD_CONTROLLER_MASK)) { Not sure this works as expected because 'i' is not a mask - it's just an int... On entry, VIR_CGROUP_THREAD_CONTROLLER_MASK is 7... The loop goes from 0 to VIR_CGROUP_CONTROLLER_LAST (11). If you logically go through the values of 'i': 0 & 7 1 & 7 2 & 7 3 & 7 4 & 7 ... etc You'd find 0 & 8 fail, but 1 -> 7, 9, & 10 succeed So what "would" work is : mask = (1 << i); if (stripEmulatorSuffix && mask & VIR_CGROUP_THREAD_CONTROLLER_MASK))

John > if (STREQ(tmp, "/emulator")) > *tmp = '\0'; > tmp = strrchr(group->controllers[i].placement, '/'); > @@ -1518,7 +1516,6 @@ virCgroupNewThread(virCgroupPtr domain, > { > int ret = -1; > char *name = NULL; > - int controllers; > > switch (nameval) { > case VIR_CGROUP_THREAD_VCPU: > @@ -1539,11 +1536,8 @@ virCgroupNewThread(virCgroupPtr domain, > goto cleanup; > } > > - controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) | > - (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | > - (1 << VIR_CGROUP_CONTROLLER_CPUSET)); > - > - if (virCgroupNew(-1, name, domain, controllers, group) < 0) > + if (virCgroupNew(-1, name, domain, > VIR_CGROUP_THREAD_CONTROLLER_MASK, > + group) < 0) > goto cleanup; > > if (virCgroupMakeGroup(domain, *group, create, > VIR_CGROUP_NONE) < 0) { diff --git a/src/util/vircgroup.h > b/src/util/vircgroup.h index f244c24..f71aed5 100644 > --- a/src/util/vircgroup.h > +++ b/src/util/vircgroup.h > @@ -52,6 +52,13 @@ VIR_ENUM_DECL(virCgroupController); > * Make sure we will not overflow */ > verify(VIR_CGROUP_CONTROLLER_LAST < 8 * sizeof(int)); > > +enum { > + VIR_CGROUP_THREAD_CONTROLLER_MASK = > + ((1 << VIR_CGROUP_CONTROLLER_CPU) | > + (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | > + (1 << VIR_CGROUP_CONTROLLER_CPUSET)) > +}; > + > typedef enum { > VIR_CGROUP_THREAD_VCPU = 0, > VIR_CGROUP_THREAD_EMULATOR, >

virCgroupNewMachine used to add the pidleader to the newly created machine cgroup. Do not do this implicit anymore. Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; --- 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 ad254e4..609e9ea 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -504,6 +504,17 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, &cgroup) < 0)

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 e41f461..66dc782 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: diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 11f33ab..aef8e8c 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1682,16 +1682,6 @@ 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); @@ -1714,7 +1704,6 @@ int virCgroupTerminateMachine(const char *name, static int virCgroupNewMachineManual(const char *name, const char *drivername, - pid_t pidleader, const char *partition, int controllers, virCgroupPtr *group) @@ -1740,16 +1729,6 @@ 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; @@ -1796,7 +1775,6 @@ virCgroupNewMachine(const char *name, return virCgroupNewMachineManual(name, drivername, - pidleader, partition, controllers, group);

On Tue, Feb 23, 2016 at 04:58:39PM +0100, Henning Schild wrote: > virCgroupNewMachine used to add the pidleader to the newly created > machine cgroup. Do not do this implicit anymore. > > Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; > --- > src/lxc/lxc_cgroup.c | 11 +++++++++++ > src/qemu/qemu_cgroup.c | 11 +++++++++++ > src/util/vircgroup.c | 22 ---------------------- > 3 files changed, 22 insertions(+), 22 deletions(-) NACK to this patch once again. This does not actually work as you think it does. > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 11f33ab..aef8e8c 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -1682,16 +1682,6 @@ virCgroupNewMachineSystemd(const char *name, > } > } > > - if (virCgroupAddTask(*group, pidleader) < 0) { > - virErrorPtr saved = virSaveLastError(); > - virCgroupRemove(*group); > - virCgroupFree(group); > - if (saved) { > - virSetError(saved); > - virFreeError(saved); > - } > - } Just above this we called virSystemdCreateMachine. Systemd will create the cgroup and add the pidleader to those cgroups. Systemd may add the pidleader to just the 'systemd' controller, or it may add the pidleader to *ALL* controllers. We have no way of knowing. This virCgroupAddTask call deals with whatever systemd chose not todo, so we can guarantee consistent behaviour with the pidleader in all cgroups. By removing this you make this method non-deterministic - the pid may or may not be in the cpu controller now. THis is bad because it can lead to QEMU/LXC driver code working in some cases but failing in other cases. Furthermore, this existing does not cause any problems for the scenario you care about. THis cgroup placement is being set in between the time libvirtd calls fork() and exec(). With your later patch 5, we ensure that the PID is moved across into the emulator cgroup, before we call exec(). When we call exec all memory mappings will be replaced, so QEMU will stil start with the correct vCPU placement and memory allocation placement.

Just just drop this patch please. Regards, Daniel

On Fri, Feb 26, 2016 at 01:16:15PM +0100, Henning Schild wrote: > On Fri, 26 Feb 2016 11:13:13 +0000 > "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > > > On Tue, Feb 23, 2016 at 04:58:39PM +0100, Henning Schild wrote: > > > virCgroupNewMachine used to add the pidleader to the newly > > > created machine cgroup. Do not do this implicit anymore. > > > > > > Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; > > > --- > > > src/lxc/lxc_cgroup.c | 11 +++++++++++ > > > src/qemu/qemu_cgroup.c | 11 +++++++++++ > > > src/util/vircgroup.c | 22 ---------------------- > > > 3 files changed, 22 insertions(+), 22 deletions(-) > > > > NACK to this patch once again. > > > > This does not actually work as you think it does. > > > > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > > > index 11f33ab..aef8e8c 100644 > > > --- a/src/util/vircgroup.c > > > +++ b/src/util/vircgroup.c > > > @@ -1682,16 +1682,6 @@ virCgroupNewMachineSystemd(const char > > > *name, } > > > } > > > > > > - if (virCgroupAddTask(*group, pidleader) < 0) { > > > - virErrorPtr saved = virSaveLastError(); > > > - virCgroupRemove(*group); > > > - virCgroupFree(group); > > > - if (saved) { > > > - virSetError(saved); > > > - virFreeError(saved); > > > - } > > > - } > > > > Just above this we called virSystemdCreateMachine. Systemd will > > create the cgroup and add the pidleader to those cgroups. Systemd > > may add the pidleader to just the 'systemd' controller, or it may > > add the pidleader to *ALL* controllers. We have no way of knowing. > > > > This virCgroupAddTask call deals with whatever systemd chose not > > todo, so we can guarantee consistent behaviour with the pidleader > > in all cgroups. > > > > By removing this you make this method non-deterministic - the pid > > may or may not be in the cpu controller now. THis is bad because > > it can lead to QEMU/LXC driver code working in some cases but > > failing in other cases. > > > > Furthermore, this existing does not cause any problems for the > > scenario you care about. THis cgroup placement is being set > > in between the time libvirtd calls fork() and exec(). With your > > later patch 5, we ensure that the PID is moved across into the > > emulator cgroup, before we call exec(). When we call exec all > > memory mappings will be replaced, so QEMU will stil start with > > the correct vCPU placement and memory allocation placement. > > I agree having the task in the wrong cgroup before the exec() seems > harmless. But i am not sure all the fiddling with cgroups is indeed > harmless and does not cause i.e. kernel work on cores that should be > left alone. I have the feeling allowing the task in the parent > cgroup is a bad idea, no matter how short the window seems to be. > > Right now the parent cgroup contains all cpus found in > machine.slice, which for pinned VMs is too much. How about we > calculate the size of the child cgroups before and make the parent > the union of them. Or give the parent the emulator pinning and > extend it for the vcpus later. But that might turn out pretty > complicated as well, getting the order right with the mix of > cpusets and sched_setaffinity(). > > Just just drop this patch please. The point is though that we have *no* choice. Systemd can put the task in the cpu controller and we've no way to prevent that. So the code *has* to be able to cope with that happening. Therefore this patch is wrong it just makes behaviour non-deterministic increasing the chances that we don't correctly handle the case where systemd adds the task to the cpu controllers

Regards, Daniel

On Fri, Feb 26, 2016 at 01:43:05PM +0100, Henning Schild wrote: > On Fri, 26 Feb 2016 12:21:02 +0000 > "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > > > On Fri, Feb 26, 2016 at 01:16:15PM +0100, Henning Schild wrote: > > > On Fri, 26 Feb 2016 11:13:13 +0000 > > > "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > > > > > > > On Tue, Feb 23, 2016 at 04:58:39PM +0100, Henning Schild > > > > wrote: > > > > > virCgroupNewMachine used to add the pidleader to the newly > > > > > created machine cgroup. Do not do this implicit anymore. > > > > > > > > > > Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; > > > > > --- > > > > > src/lxc/lxc_cgroup.c | 11 +++++++++++ > > > > > src/qemu/qemu_cgroup.c | 11 +++++++++++ > > > > > src/util/vircgroup.c | 22 ---------------------- > > > > > 3 files changed, 22 insertions(+), 22 deletions(-) > > > > > > > > NACK to this patch once again. > > > > > > > > This does not actually work as you think it does. > > > > > > > > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > > > > > index 11f33ab..aef8e8c 100644 > > > > > --- a/src/util/vircgroup.c > > > > > +++ b/src/util/vircgroup.c > > > > > @@ -1682,16 +1682,6 @@ virCgroupNewMachineSystemd(const char > > > > > *name, } > > > > > } > > > > > > > > > > - if (virCgroupAddTask(*group, pidleader) < 0) { > > > > > - virErrorPtr saved = virSaveLastError(); > > > > > - virCgroupRemove(*group); > > > > > - virCgroupFree(group); > > > > > - if (saved) { > > > > > - virSetError(saved); > > > > > - virFreeError(saved); > > > > > - } > > > > > - } > > > > > > > > Just above this we called virSystemdCreateMachine. Systemd > > > > will create the cgroup and add the pidleader to those > > > > cgroups. Systemd may add the pidleader to just the 'systemd' > > > > controller, or it may add the pidleader to *ALL* controllers. > > > > We have no way of knowing. > > > > > > > > This virCgroupAddTask call deals with whatever systemd chose > > > > not todo, so we can guarantee consistent behaviour with the > > > > pidleader in all cgroups. > > > > > > > > By removing this you make this method non-deterministic - the > > > > pid may or may not be in the cpu controller now. THis is bad > > > > because it can lead to QEMU/LXC driver code working in some > > > > cases but failing in other cases. > > > > > > > > Furthermore, this existing does not cause any problems for the > > > > scenario you care about. THis cgroup placement is being set > > > > in between the time libvirtd calls fork() and exec(). With > > > > your later patch 5, we ensure that the PID is moved across > > > > into the emulator cgroup, before we call exec(). When we call > > > > exec all memory mappings will be replaced, so QEMU will stil > > > > start with the correct vCPU placement and memory allocation > > > > placement. > > > > > > I agree having the task in the wrong cgroup before the exec() > > > seems harmless. But i am not sure all the fiddling with cgroups > > > is indeed harmless and does not cause i.e. kernel work on cores > > > that should be left alone. I have the feeling allowing the task > > > in the parent cgroup is a bad idea, no matter how short the > > > window seems to be. > > > > > > Right now the parent cgroup contains all cpus found in > > > machine.slice, which for pinned VMs is too much. How about we > > > calculate the size of the child cgroups before and make the > > > parent the union of them. Or give the parent the emulator > > > pinning and extend it for the vcpus later. But that might turn > > > out pretty complicated as well, getting the order right with > > > the mix of cpusets and sched_setaffinity(). > > > > Just just drop this patch please. > > > > The point is though that we have *no* choice. Systemd can put the > > task in the cpu controller and we've no way to prevent that. So > > the code *has* to be able to cope with that happening. Therefore > > this patch is wrong it just makes behaviour non-deterministic > > increasing the chances that we don't correctly handle the case > > where systemd adds the task to the cpu controllers > > Understood! I was suggesting a "growing on demand" policy instead of > "shrinking after inheriting all". > > If we can not control what systemd does we have to give it harmless > cpus to mess around with. That assumes we can control the size of > the cpuset before systemd puts anything in. Once back in control we > grow the parent group before deriving more child groups. > > Would that be possible? > > I have no objections to keep using the shrinking approach. > Especially since the controlled growing is harder to implement in > the given codebase. It just feels like it should be the other way > around. I don't see what actual problem you are trying to solve here. IIUC, the original problem you wanted to address was that vCPU pids could run the wrong CPU for a short time. ie The original code did 1. Libvirtd forks child pid ... this pid runs on whatever pCPUs that libvirt is permitted to use... 2. Libvirtd creates root cgroup for VM ... this pid runs on whatever pCPUs the root cgroup inherited... 3. Child pid execs QEMU ... QEMU pid runs on whatever pCPUs the root cgroup inherited... 4. QEMU creates vCPU pids ... vCPU pids run on whatever pCPUs the root cgroup inherited... 4. Libvirtd moves emulator PIDs and vCPU PIDs ... emulator PIDs are running on assigned pCPUs for emulator... ... vCPU PIDs are running on assigned pCPUs for vCPUs.... With the important change in patch 5 this now looks like 1. Libvirtd forks child pid ... this pid runs on whatever pCPUs that libvirt is permitted to use... 2. Libvirtd creates root cgroup for VM

... this pid runs on whatever pCPUs the root cgroup inherited...

... this pid runs on whatever pCPUs the root cgroup inherited...

On 02/23/2016 10:58 AM, Henning Schild wrote: > Move qemuSetupCgroupForEmulator up under qemuSetupCgroup. That way > we move the one main thread right into the emulator cgroup, instead > of moving multiple threads later on. And we do not actually want any > threads running in the parent cgroups (cpu cpuacct cpuset). > > Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; > --- > src/qemu/qemu_process.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > This is where things are going to get messy. Peter Krempa posted a series after yours: http://www.redhat.com/archives/libvir-list/2016-February/msg01211.html which conflicts with this and the followup patch. Hopefully between you, Peter, and Dan something can be worked out.

Also, it seems starting at patch 7 there's more conflicts with the top of the upstream, so I couldn't 'git am -3' them into a local branch. John > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 05cbda2..65f718c 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4895,6 +4895,10 @@ qemuProcessLaunch(virConnectPtr conn, > if (qemuSetupCgroup(driver, vm, nnicindexes, nicindexes) < 0) > goto cleanup; > > + VIR_DEBUG("Setting cgroup for emulator (if required)"); > + if (qemuSetupCgroupForEmulator(vm) < 0) > + goto cleanup; > + > /* This must be done after cgroup placement to avoid resetting > CPU > * affinity */ > if (!vm->def->cputune.emulatorpin && > @@ -4943,10 +4947,6 @@ 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) > goto cleanup; >

Move qemuSetupCgroupForEmulator up under qemuSetupCgroup. That way we move the one main thread right into the emulator cgroup, instead of moving multiple threads later on. And we do not actually want any threads running in the parent cgroups (cpu cpuacct cpuset). Signed-off-by: Henning Schild <henning.schild(a)siemens.com&gt; --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 05cbda2..65f718c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4895,6 +4895,10 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuSetupCgroup(driver, vm, nnicindexes, nicindexes) < 0) goto cleanup; + VIR_DEBUG("Setting cgroup for emulator (if required)"); + if (qemuSetupCgroupForEmulator(vm) < 0) + goto cleanup; + /* This must be done after cgroup placement to avoid resetting CPU * affinity */ if (!vm->def->cputune.emulatorpin && @@ -4943,10 +4947,6 @@ 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) goto cleanup; -- 2.4.10