The function has no argument named @name rather than @path instead. The comment is, however, referring to @name while it should have been referring to @path really. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 0a2e729..660f840 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1290,7 +1290,7 @@ virCgroupSetPartitionSuffix(const char *path, char **res) * @controllers: mask of controllers to create * * Creates a new cgroup to represent the resource - * partition path identified by @name. + * partition path identified by @path. * * Returns 0 on success, -1 on failure */ -- 2.0.5

Throughout our code, the virCgroupController enum is used in two ways. First as an index to an array of cgroup controllers: struct virCgroup { char *path; struct virCgroupController controllers[VIR_CGROUP_CONTROLLER_LAST]; }; Second way is that when calling virCgroupNew() a bitmask of the enum items can be passed to selectively detect only some controllers. For instance: int virCgroupNewVcpu(virCgroupPtr domain, int vcpuid, bool create, virCgroupPtr *group) { ... controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) | (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | (1 << VIR_CGROUP_CONTROLLER_CPUSET)); if (virCgroupNew(-1, name, domain, controllers, group) < 0) goto cleanup; } Even though it's highly unlikely that so many new controllers will be invented so that we would overflow when constructing the bitmask, it doesn't hurt to check at compile time either. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/vircgroup.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index bfb3a9b..eee15ca 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -46,6 +46,11 @@ enum { }; VIR_ENUM_DECL(virCgroupController); +/* Items of this enum are used later in virCgroupNew to create + * bit array stored in int. Like this: + * 1 << VIR_CGROUP_CONTROLLER_CPU + * Make sure we will not overflow */ +verify(VIR_CGROUP_CONTROLLER_LAST < 8 * sizeof(int)); bool virCgroupAvailable(void); -- 2.0.5

Once upon a time, there was a little domain. And the domain was pinned onto a NUMA node and hasn't fully allocated its memory: <memory unit='KiB'>2355200</memory> <currentMemory unit='KiB'>1048576</currentMemory> <numatune> <memory mode='strict' nodeset='0'/> </numatune> Oh little me, said the domain, what will I do with so few memory. If I only had a few megabytes more. But the old admin noticed whimpering, barely audible to untrained human ear. And good admin he was, he gave the domain yet more memory. But the old NUMA topology witch forbidden to allocate more memory on the node zero. So he decided to allocate it on a different node: virsh # numatune little_domain --nodeset 0-1 virsh # setmem little_domain 2355200 The little domain was happy. For a while. Until bad, sharp teeth shaped creature came. Every process in the system was afraid of him. The OOM Killer they called him. Oh no, he's after the little domain. There's no escape. Do you kids know why? Because when the little domain was born, her father, Libvirt, called numa_set_membind(). So even if the admin allowed her to allocate memory from other nodes in the cgroups, the membind() forbid it. So what's the lesson? Libvirt should rely on cgroups, whenever possible and use numa_set_membind() as the last ditch effort. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 79f763e..cba042d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3154,6 +3154,7 @@ static int qemuProcessHook(void *data) int fd; virBitmapPtr nodeset = NULL; virDomainNumatuneMemMode mode; + bool doNuma = true; /* This method cannot use any mutexes, which are not * protected across fork() @@ -3185,11 +3186,34 @@ static int qemuProcessHook(void *data) goto cleanup; mode = virDomainNumatuneGetMode(h->vm->def->numa, -1); - nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa, - priv->autoNodeset, -1); - if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) - goto cleanup; + if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) { + virCgroupPtr cgroup = NULL; + + /* Create dummy cgroup ... */ + if (virCgroupNewSelf(&cgroup) < 0) + goto cleanup; + + /* ... just to detect if cpuset cgroup is present */ + if (virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + /* Because if it's not, we must use virNuma* APIs to bind + * memory onto desired nodes. CGroup way is preferred, as + * it allows runtime tuning, while virNuma - well, once + * set and child (qemu) is exec()-ed, we can't do + * anything about the settings. virNuma* does not take + * any PID argument after all. */ + doNuma = false; + } + virCgroupFree(&cgroup); + } + + if (doNuma) { + nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa, + priv->autoNodeset, -1); + + if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) + goto cleanup; + } ret = 0; -- 2.0.5

On 30.03.2015 15:15, Ján Tomko wrote: Good point, I though that CGroups are created later in the process. But due to handshaking with child, we are guaranteed that we can access priv->cgroup. Will send v2 in a while. Michal