1:58 a.m.
Deatils are in the patches themselves, but the basic idea is this:
Setup:
$ grep DMA32 /proc/zoneinfo
Node 0, zone DMA32
$ virsh dumpxml domain | grep -C1 strict
<numatune>
<memory mode='strict' nodeset='1'/>
</numatune>
$ virsh start domain
Domain domain started
Before:
$ virsh setvcpus domain 2
error: Unable to read from monitor: Connection reset by peer
# Domain died
After:
$ virsh setvcpus domain 2
# hotplug successful
Martin
Martin Kletzander (6):
util: Add function virCgroupHasEmptyTasks
util: Add virNumaGetHostNodeset
qemu: Remove unnecessary qemuSetupCgroupPostInit function
qemu: Save numad advice into qemuDomainObjPrivate
qemu: Leave cpuset.mems in parent cgroup alone
qemu: Fix hotplugging cpus with strict memory pinning
src/libvirt_private.syms | 2 ++
src/qemu/qemu_cgroup.c | 94 +++++++++++++++++++++++++++++++++++++-----------
src/qemu/qemu_cgroup.h | 9 ++---
src/qemu/qemu_domain.c | 1 +
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 88 +++++++++++++++++++++++++--------------------
src/qemu/qemu_process.c | 21 ++++++-----
src/util/vircgroup.c | 23 ++++++++++++
src/util/vircgroup.h | 4 ++-
src/util/virnuma.c | 28 +++++++++++++++
src/util/virnuma.h | 1 +
11 files changed, 194 insertions(+), 78 deletions(-)
--
2.2.0
1:58 a.m.
New subject: [libvirt] [PATCH 1/6] util: Add function virCgroupHasEmptyTasks
That function helps checking whether there's a task in that cgroup.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/vircgroup.c | 23 +++++++++++++++++++++++
src/util/vircgroup.h | 4 +++-
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6df2784..60979f8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1096,6 +1096,7 @@ virCgroupGetMemSwapHardLimit;
virCgroupGetMemSwapUsage;
virCgroupGetPercpuStats;
virCgroupHasController;
+virCgroupHasEmptyTasks;
virCgroupIsolateMount;
virCgroupKill;
virCgroupKillPainfully;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 3995477..4857ef3 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3926,6 +3926,20 @@ virCgroupSupportsCpuBW(virCgroupPtr cgroup)
return ret;
}
+int
+virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller)
+{
+ int ret = -1;
+ char *content = NULL;
+
+ ret = virCgroupGetValueStr(cgroup, controller, "tasks", &content);
+
+ if (ret == 0 && content[0] == '\0')
+ ret = 1;
+
+ VIR_FREE(content);
+ return ret;
+}
#else /* !VIR_CGROUP_SUPPORTED */
@@ -4655,4 +4669,13 @@ virCgroupSetOwner(virCgroupPtr cgroup ATTRIBUTE_UNUSED,
return -1;
}
+int
+virCgroupHasEmptyTasks(virCgroupPtr cgroup ATTRIBUTE_UNUSED,
+ int controller ATTRIBUTE_UNUSED)
+{
+ virReportSystemError(ENOSYS, "%s",
+ _("Control groups not supported on this platform"));
+ return -1;
+}
+
#endif /* !VIR_CGROUP_SUPPORTED */
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index 7718a07..f07c1a7 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -1,7 +1,7 @@
/*
* vircgroup.h: methods for managing control cgroups
*
- * Copyright (C) 2011-2013 Red Hat, Inc.
+ * Copyright (C) 2011-2014 Red Hat, Inc.
* Copyright IBM Corp. 2008
*
* This library is free software; you can redistribute it and/or
@@ -270,4 +270,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup,
gid_t gid,
int controllers);
+int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller);
+
#endif /* __VIR_CGROUP_H__ */
--
2.2.0
1:58 a.m.
New subject: [libvirt] [PATCH 2/6] util: Add virNumaGetHostNodeset
That function tries its best to create a bitmap of host NUMA nodes.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virnuma.c | 28 ++++++++++++++++++++++++++++
src/util/virnuma.h | 1 +
3 files changed, 30 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 60979f8..b21f5fb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1748,6 +1748,7 @@ virNodeSuspendGetTargetMask;
# util/virnuma.h
virNumaGetAutoPlacementAdvice;
virNumaGetDistances;
+virNumaGetHostNodeset;
virNumaGetMaxNode;
virNumaGetNodeMemory;
virNumaGetPageInfo;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index b8d76f4..86564d4 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -983,3 +983,31 @@ virNumaNodesetIsAvailable(virBitmapPtr nodeset)
}
return true;
}
+
+virBitmapPtr
+virNumaGetHostNodeset(void)
+{
+ int maxnode = virNumaGetMaxNode();
+ size_t i = 0;
+ virBitmapPtr nodeset = NULL;
+
+ if (maxnode < 0)
+ return NULL;
+
+ if (!(nodeset = virBitmapNew(maxnode + 1)))
+ return NULL;
+
+ for (i = 0; i <= maxnode; i++) {
+ if (!virNumaNodeIsAvailable(i))
+ continue;
+
+ if (virBitmapSetBit(nodeset, i) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Problem setting bit in bitmap"));
+ virBitmapFree(nodeset);
+ return NULL;
+ }
+ }
+
+ return nodeset;
+}
diff --git a/src/util/virnuma.h b/src/util/virnuma.h
index 09034a2..1f3c0ad 100644
--- a/src/util/virnuma.h
+++ b/src/util/virnuma.h
@@ -33,6 +33,7 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups,
int virNumaSetupMemoryPolicy(virDomainNumatuneMemMode mode,
virBitmapPtr nodeset);
+virBitmapPtr virNumaGetHostNodeset(void);
bool virNumaNodesetIsAvailable(virBitmapPtr nodeset);
bool virNumaIsAvailable(void);
int virNumaGetMaxNode(void);
--
2.2.0
1:58 a.m.
New subject: [libvirt] [PATCH 3/6] qemu: Remove unnecessary qemuSetupCgroupPostInit function
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 9 +--------
src/qemu/qemu_cgroup.h | 4 ++--
src/qemu/qemu_process.c | 2 +-
3 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 0e94cae..ed5416b 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -607,7 +607,7 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
}
-static int
+int
qemuSetupCpusetMems(virDomainObjPtr vm,
virBitmapPtr nodemask)
{
@@ -867,13 +867,6 @@ qemuSetupCgroup(virQEMUDriverPtr driver,
}
int
-qemuSetupCgroupPostInit(virDomainObjPtr vm,
- virBitmapPtr nodemask)
-{
- return qemuSetupCpusetMems(vm, nodemask);
-}
-
-int
qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
unsigned long long period,
long long quota)
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index 4a4f22c..72c28f7 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -47,8 +47,8 @@ int qemuConnectCgroup(virQEMUDriverPtr driver,
int qemuSetupCgroup(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virBitmapPtr nodemask);
-int qemuSetupCgroupPostInit(virDomainObjPtr vm,
- virBitmapPtr nodemask);
+int qemuSetupCpusetMems(virDomainObjPtr vm,
+ virBitmapPtr nodemask);
int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
unsigned long long period,
long long quota);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a19e71a..9781e44 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4710,7 +4710,7 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
VIR_DEBUG("Setting up post-init cgroup restrictions");
- if (qemuSetupCgroupPostInit(vm, nodemask) < 0)
+ if (qemuSetupCpusetMems(vm, nodemask) < 0)
goto cleanup;
VIR_DEBUG("Detecting VCPU PIDs");
--
2.2.0
1:58 a.m.
New subject: [libvirt] [PATCH 4/6] qemu: Save numad advice into qemuDomainObjPrivate
Thanks to that we don't need to drag the pointer everywhere and future
code will get cleaner.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 18 +++++++-----------
src/qemu/qemu_cgroup.h | 9 +++------
src/qemu/qemu_domain.c | 1 +
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_process.c | 21 ++++++++++-----------
5 files changed, 22 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index ed5416b..a0370bc 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -608,8 +608,7 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
int
-qemuSetupCpusetMems(virDomainObjPtr vm,
- virBitmapPtr nodemask)
+qemuSetupCpusetMems(virDomainObjPtr vm)
{
virCgroupPtr cgroup_temp = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -624,7 +623,7 @@ qemuSetupCpusetMems(virDomainObjPtr vm,
return 0;
if (virDomainNumatuneMaybeFormatNodeset(vm->def->numatune,
- nodemask,
+ priv->autoNodeset,
&mem_mask, -1) < 0)
goto cleanup;
@@ -644,7 +643,6 @@ qemuSetupCpusetMems(virDomainObjPtr vm,
static int
qemuSetupCpusetCgroup(virDomainObjPtr vm,
- virBitmapPtr nodemask,
virCapsPtr caps)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -659,7 +657,7 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm,
if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
virBitmapPtr cpumap;
- if (!(cpumap = virCapabilitiesGetCpusForNodemask(caps, nodemask)))
+ if (!(cpumap = virCapabilitiesGetCpusForNodemask(caps,
priv->autoNodeset)))
goto cleanup;
cpu_mask = virBitmapFormat(cpumap);
virBitmapFree(cpumap);
@@ -823,8 +821,7 @@ qemuConnectCgroup(virQEMUDriverPtr driver,
int
qemuSetupCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virBitmapPtr nodemask)
+ virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virCapsPtr caps = NULL;
@@ -857,7 +854,7 @@ qemuSetupCgroup(virQEMUDriverPtr driver,
if (qemuSetupCpuCgroup(driver, vm) < 0)
goto cleanup;
- if (qemuSetupCpusetCgroup(vm, nodemask, caps) < 0)
+ if (qemuSetupCpusetCgroup(vm, caps) < 0)
goto cleanup;
ret = 0;
@@ -1042,8 +1039,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
int
qemuSetupCgroupForEmulator(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virBitmapPtr nodemask)
+ virDomainObjPtr vm)
{
virBitmapPtr cpumask = NULL;
virBitmapPtr cpumap = NULL;
@@ -1079,7 +1075,7 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver,
goto cleanup;
if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
- if (!(cpumap = qemuPrepareCpumap(driver, nodemask)))
+ if (!(cpumap = qemuPrepareCpumap(driver, priv->autoNodeset)))
goto cleanup;
cpumask = cpumap;
} else if (def->cputune.emulatorpin) {
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index 72c28f7..df7e3a1 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -45,10 +45,8 @@ int qemuTeardownHostdevCgroup(virDomainObjPtr vm,
int qemuConnectCgroup(virQEMUDriverPtr driver,
virDomainObjPtr vm);
int qemuSetupCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virBitmapPtr nodemask);
-int qemuSetupCpusetMems(virDomainObjPtr vm,
- virBitmapPtr nodemask);
+ virDomainObjPtr vm);
+int qemuSetupCpusetMems(virDomainObjPtr vm);
int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
unsigned long long period,
long long quota);
@@ -64,8 +62,7 @@ int qemuSetupCgroupIOThreadsPin(virCgroupPtr cgroup,
int qemuSetupCgroupForVcpu(virDomainObjPtr vm);
int qemuSetupCgroupForIOThreads(virDomainObjPtr vm);
int qemuSetupCgroupForEmulator(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virBitmapPtr nodemask);
+ virDomainObjPtr vm);
int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm);
int qemuAddToCgroup(virDomainObjPtr vm);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 220304f..8dd427a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -451,6 +451,7 @@ qemuDomainObjPrivateFree(void *data)
qemuAgentClose(priv->agent);
}
VIR_FREE(priv->cleanupCallbacks);
+ virBitmapFree(priv->autoNodeset);
VIR_FREE(priv);
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index efabd82..ce5eba3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -187,6 +187,7 @@ struct _qemuDomainObjPrivate {
char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */
bool hookRun; /* true if there was a hook run over this domain */
+ virBitmapPtr autoNodeset;
};
typedef enum {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9781e44..d683918 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2377,12 +2377,12 @@ qemuPrepareCpumap(virQEMUDriverPtr driver,
*/
static int
qemuProcessInitCpuAffinity(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virBitmapPtr nodemask)
+ virDomainObjPtr vm)
{
int ret = -1;
virBitmapPtr cpumap = NULL;
virBitmapPtr cpumapToSet = NULL;
+ qemuDomainObjPrivatePtr priv = vm->privateData;
if (!vm->pid) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -2390,7 +2390,7 @@ qemuProcessInitCpuAffinity(virQEMUDriverPtr driver,
return -1;
}
- if (!(cpumap = qemuPrepareCpumap(driver, nodemask)))
+ if (!(cpumap = qemuPrepareCpumap(driver, priv->autoNodeset)))
return -1;
if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
@@ -3041,13 +3041,13 @@ struct qemuProcessHookData {
virConnectPtr conn;
virDomainObjPtr vm;
virQEMUDriverPtr driver;
- virBitmapPtr nodemask;
virQEMUDriverConfigPtr cfg;
};
static int qemuProcessHook(void *data)
{
struct qemuProcessHookData *h = data;
+ qemuDomainObjPrivatePtr priv = h->vm->privateData;
int ret = -1;
int fd;
virBitmapPtr nodeset = NULL;
@@ -3084,7 +3084,7 @@ static int qemuProcessHook(void *data)
mode = virDomainNumatuneGetMode(h->vm->def->numatune, -1);
nodeset = virDomainNumatuneGetNodeset(h->vm->def->numatune,
- h->nodemask, -1);
+ priv->autoNodeset, -1);
if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
goto cleanup;
@@ -4426,7 +4426,7 @@ int qemuProcessStart(virConnectPtr conn,
if (virBitmapParse(nodeset, 0, &nodemask, VIR_DOMAIN_CPUMASK_LEN) < 0)
goto cleanup;
}
- hookData.nodemask = nodemask;
+ priv->autoNodeset = nodemask;
/* "volume" type disk's source must be translated before
* cgroup and security setting.
@@ -4630,13 +4630,13 @@ int qemuProcessStart(virConnectPtr conn,
}
VIR_DEBUG("Setting up domain cgroup (if required)");
- if (qemuSetupCgroup(driver, vm, nodemask) < 0)
+ if (qemuSetupCgroup(driver, vm) < 0)
goto cleanup;
/* This must be done after cgroup placement to avoid resetting CPU
* affinity */
if (!vm->def->cputune.emulatorpin &&
- qemuProcessInitCpuAffinity(driver, vm, nodemask) < 0)
+ qemuProcessInitCpuAffinity(driver, vm) < 0)
goto cleanup;
VIR_DEBUG("Setting domain security labels");
@@ -4683,7 +4683,7 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
VIR_DEBUG("Setting cgroup for emulator (if required)");
- if (qemuSetupCgroupForEmulator(driver, vm, nodemask) < 0)
+ if (qemuSetupCgroupForEmulator(driver, vm) < 0)
goto cleanup;
VIR_DEBUG("Setting affinity of emulator threads");
@@ -4710,7 +4710,7 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
VIR_DEBUG("Setting up post-init cgroup restrictions");
- if (qemuSetupCpusetMems(vm, nodemask) < 0)
+ if (qemuSetupCpusetMems(vm) < 0)
goto cleanup;
VIR_DEBUG("Detecting VCPU PIDs");
@@ -4848,7 +4848,6 @@ int qemuProcessStart(virConnectPtr conn,
* if we failed to initialize the now running VM. kill it off and
* pretend we never started it */
VIR_FREE(nodeset);
- virBitmapFree(nodemask);
virCommandFree(cmd);
VIR_FORCE_CLOSE(logfile);
if (priv->mon)
--
2.2.0
1:58 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: Leave cpuset.mems in parent cgroup alone
Instead of setting the value of cpuset.mems once when the domain starts
and then re-calculating the value every time we need to change the child
cgroup values, leave the cgroup alone and rather set the child data
every time there is new cgroup created. We don't leave any task in the
parent group anyway. This will ease both current and future code.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++--
src/qemu/qemu_driver.c | 59 +++++++++++++++-----------------------------
2 files changed, 85 insertions(+), 41 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a0370bc..1e383c4 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -35,6 +35,7 @@
#include "virstring.h"
#include "virfile.h"
#include "virtypedparam.h"
+#include "virnuma.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -629,8 +630,7 @@ qemuSetupCpusetMems(virDomainObjPtr vm)
if (mem_mask)
if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0 ||
- virCgroupSetCpusetMems(cgroup_temp, mem_mask) < 0 ||
- virCgroupSetCpusetMems(priv->cgroup, mem_mask) < 0)
+ virCgroupSetCpusetMems(cgroup_temp, mem_mask) < 0)
goto cleanup;
ret = 0;
@@ -785,6 +785,39 @@ qemuInitCgroup(virQEMUDriverPtr driver,
return ret;
}
+static void
+qemuRestoreCgroupState(virDomainObjPtr vm)
+{
+ char *mem_mask;
+ int empty = -1;
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ virBitmapPtr all_nodes;
+
+ if (!(all_nodes = virNumaGetHostNodeset()))
+ goto error;
+
+ if (!(mem_mask = virBitmapFormat(all_nodes)))
+ goto error;
+
+ if ((empty = virCgroupHasEmptyTasks(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPUSET)) < 0)
+
+ if (!empty)
+ goto error;
+
+ if (virCgroupSetCpusetMems(priv->cgroup, mem_mask) < 0)
+ goto error;
+
+ cleanup:
+ VIR_FREE(mem_mask);
+ virBitmapFree(all_nodes);
+ return;
+
+ error:
+ virResetLastError();
+ VIR_DEBUG("Couldn't restore cgroups to meaningful state");
+ goto cleanup;
+}
int
qemuConnectCgroup(virQEMUDriverPtr driver,
@@ -812,6 +845,8 @@ qemuConnectCgroup(virQEMUDriverPtr driver,
&priv->cgroup) < 0)
goto cleanup;
+ qemuRestoreCgroupState(vm);
+
done:
ret = 0;
cleanup:
@@ -961,6 +996,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
size_t i, j;
unsigned long long period = vm->def->cputune.period;
long long quota = vm->def->cputune.quota;
+ char *mem_mask = NULL;
if ((period || quota) &&
!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
@@ -992,6 +1028,13 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
return 0;
}
+ if (virDomainNumatuneGetMode(vm->def->numatune, -1) ==
+ VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+ virDomainNumatuneMaybeFormatNodeset(vm->def->numatune,
+ priv->autoNodeset,
+ &mem_mask, -1) < 0)
+ goto cleanup;
+
for (i = 0; i < priv->nvcpupids; i++) {
if (virCgroupNewVcpu(priv->cgroup, i, true, &cgroup_vcpu) < 0)
goto cleanup;
@@ -1000,6 +1043,10 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
if (virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]) < 0)
goto cleanup;
+ if (mem_mask &&
+ virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0)
+ goto cleanup;
+
if (period || quota) {
if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
goto cleanup;
@@ -1025,6 +1072,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
virCgroupFree(&cgroup_vcpu);
}
+ VIR_FREE(mem_mask);
return 0;
@@ -1033,6 +1081,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
virCgroupRemove(cgroup_vcpu);
virCgroupFree(&cgroup_vcpu);
}
+ VIR_FREE(mem_mask);
return -1;
}
@@ -1121,6 +1170,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
size_t i, j;
unsigned long long period = vm->def->cputune.period;
long long quota = vm->def->cputune.quota;
+ char *mem_mask = NULL;
if ((period || quota) &&
!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
@@ -1149,6 +1199,13 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
return 0;
}
+ if (virDomainNumatuneGetMode(vm->def->numatune, -1) ==
+ VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+ virDomainNumatuneMaybeFormatNodeset(vm->def->numatune,
+ priv->autoNodeset,
+ &mem_mask, -1) < 0)
+ goto cleanup;
+
for (i = 0; i < priv->niothreadpids; i++) {
/* IOThreads are numbered 1..n, although the array is 0..n-1,
* so we will account for that here
@@ -1166,6 +1223,10 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
goto cleanup;
}
+ if (mem_mask &&
+ virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0)
+ goto cleanup;
+
/* Set iothreadpin in cgroup if iothreadpin xml is provided */
if (virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_CPUSET)) {
@@ -1188,6 +1249,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
virCgroupFree(&cgroup_iothread);
}
+ VIR_FREE(mem_mask);
return 0;
@@ -1196,6 +1258,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
virCgroupRemove(cgroup_iothread);
virCgroupFree(&cgroup_iothread);
}
+ VIR_FREE(mem_mask);
return -1;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index df3ba6d..bfa62c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4427,6 +4427,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
pid_t *cpupids = NULL;
int ncpupids;
virCgroupPtr cgroup_vcpu = NULL;
+ char *mem_mask = NULL;
qemuDomainObjEnterMonitor(driver, vm);
@@ -4490,6 +4491,13 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
goto cleanup;
}
+ if (virDomainNumatuneGetMode(vm->def->numatune, -1) ==
+ VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+ virDomainNumatuneMaybeFormatNodeset(vm->def->numatune,
+ priv->autoNodeset,
+ &mem_mask, -1) < 0)
+ goto cleanup;
+
if (nvcpus > oldvcpus) {
for (i = oldvcpus; i < nvcpus; i++) {
if (priv->cgroup) {
@@ -4498,6 +4506,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
if (virCgroupNewVcpu(priv->cgroup, i, true, &cgroup_vcpu) < 0)
goto cleanup;
+ if (mem_mask &&
+ virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0)
+ goto cleanup;
+
/* Add vcpu thread to the cgroup */
rv = virCgroupAddTask(cgroup_vcpu, cpupids[i]);
if (rv < 0) {
@@ -4507,6 +4519,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
virCgroupRemove(cgroup_vcpu);
goto cleanup;
}
+
}
/* Inherit def->cpuset */
@@ -4579,6 +4592,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
qemuDomainObjExitMonitor(driver, vm);
vm->def->vcpus = vcpus;
VIR_FREE(cpupids);
+ VIR_FREE(mem_mask);
virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
if (cgroup_vcpu)
virCgroupFree(&cgroup_vcpu);
@@ -9215,11 +9229,9 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
static int
qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
- virCapsPtr caps,
virBitmapPtr nodeset)
{
virCgroupPtr cgroup_temp = NULL;
- virBitmapPtr temp_nodeset = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
char *nodeset_str = NULL;
size_t i = 0;
@@ -9233,39 +9245,15 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
goto cleanup;
}
- /* Get existing nodeset values */
- if (virCgroupGetCpusetMems(priv->cgroup, &nodeset_str) < 0 ||
- virBitmapParse(nodeset_str, 0, &temp_nodeset,
- VIR_DOMAIN_CPUMASK_LEN) < 0)
- goto cleanup;
- VIR_FREE(nodeset_str);
-
- for (i = 0; i < caps->host.nnumaCell; i++) {
- bool result;
- virCapsHostNUMACellPtr cell = caps->host.numaCell[i];
- if (virBitmapGetBit(nodeset, cell->num, &result) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Failed to get cpuset bit values"));
- goto cleanup;
- }
- if (result && (virBitmapSetBit(temp_nodeset, cell->num) < 0)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Failed to set temporary cpuset bit values"));
- goto cleanup;
- }
- }
-
- if (!(nodeset_str = virBitmapFormat(temp_nodeset)))
- goto cleanup;
-
- if (virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0)
- goto cleanup;
- VIR_FREE(nodeset_str);
-
/* Ensure the cpuset string is formatted before passing to cgroup */
if (!(nodeset_str = virBitmapFormat(nodeset)))
goto cleanup;
+ if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0 ||
+ virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
+ goto cleanup;
+ virCgroupFree(&cgroup_temp);
+
for (i = 0; i < priv->nvcpupids; i++) {
if (virCgroupNewVcpu(priv->cgroup, i, false, &cgroup_temp) < 0 ||
virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
@@ -9273,11 +9261,6 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
virCgroupFree(&cgroup_temp);
}
- if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0 ||
- virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0 ||
- virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0)
- goto cleanup;
-
for (i = 0; i < priv->niothreadpids; i++) {
if (virCgroupNewIOThread(priv->cgroup, i + 1, false,
&cgroup_temp) < 0 ||
@@ -9286,11 +9269,9 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
virCgroupFree(&cgroup_temp);
}
-
ret = 0;
cleanup:
VIR_FREE(nodeset_str);
- virBitmapFree(temp_nodeset);
virCgroupFree(&cgroup_temp);
return ret;
@@ -9392,7 +9373,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
}
if (nodeset &&
- qemuDomainSetNumaParamsLive(vm, caps, nodeset) < 0)
+ qemuDomainSetNumaParamsLive(vm, nodeset) < 0)
goto endjob;
if (virDomainNumatuneSet(&vm->def->numatune,
--
2.2.0
12:51 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: Leave cpuset.mems in parent cgroup alone
On 12/15/2014 12:58 AM, Martin Kletzander wrote:
Instead of setting the value of cpuset.mems once when the domain
starts
and then re-calculating the value every time we need to change the child
cgroup values, leave the cgroup alone and rather set the child data
every time there is new cgroup created. We don't leave any task in the
parent group anyway. This will ease both current and future code.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++--
src/qemu/qemu_driver.c | 59 +++++++++++++++-----------------------------
2 files changed, 85 insertions(+), 41 deletions(-)
This patch causes libvirtd to segfault on startup:
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffda116700 (LWP 15311)]
0x00007ffff7400890 in virCgroupPathOfController (group=0x0, controller=2,
key=0x7ffff768a019 "tasks", path=0x7fffda115b30) at
util/vircgroup.c:1867
1867 if (group->controllers[controller].mountPoint == NULL) {
(gdb) bt
#0 0x00007ffff7400890 in virCgroupPathOfController (group=0x0,
controller=2,
key=0x7ffff768a019 "tasks", path=0x7fffda115b30) at
util/vircgroup.c:1867
#1 0x00007ffff73fe4db in virCgroupGetValueStr (group=0x0, controller=2,
key=0x7ffff768a019 "tasks", value=0x7fffda115b80) at
util/vircgroup.c:751
#2 0x00007ffff7405673 in virCgroupHasEmptyTasks (cgroup=0x0, controller=2)
at util/vircgroup.c:3935
#3 0x00007fffdc79c687 in qemuRestoreCgroupState (vm=0x7fffd41dd8d0)
at qemu/qemu_cgroup.c:802
#4 0x00007fffdc79c80c in qemuConnectCgroup (driver=0x7fffd4152ae0,
vm=0x7fffd41dd8d0) at qemu/qemu_cgroup.c:848
#5 0x00007fffdc7b8553 in qemuProcessReconnect (opaque=0x7fffd41da150)
at qemu/qemu_process.c:3616
#6 0x00007ffff7469830 in virThreadHelper (data=0x7fffd41c5b00)
at util/virthread.c:197
#7 0x00007ffff3e65ee5 in start_thread (arg=0x7fffda116700)
at pthread_create.c:309
#8 0x00007ffff3b94b8d in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: Leave cpuset.mems in parent cgroup alone
On 12/16/2014 11:51 PM, Eric Blake wrote:
On 12/15/2014 12:58 AM, Martin Kletzander wrote:
> Instead of setting the value of cpuset.mems once when the domain starts
> and then re-calculating the value every time we need to change the child
> cgroup values, leave the cgroup alone and rather set the child data
> every time there is new cgroup created. We don't leave any task in the
> parent group anyway. This will ease both current and future code.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_cgroup.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_driver.c | 59 +++++++++++++++-----------------------------
> 2 files changed, 85 insertions(+), 41 deletions(-)
This patch causes libvirtd to segfault on startup:
More particularly, I'm doing an upgrade from older libvirtd to this
version, while leaving a transient domain running that was started by
the older libvirtd. Hope that helps you narrow in on the problem.
Reverting 86759e and af2a1f0 was sufficient to get me going again
locally, but I'm not going to push my reversions until you've first had
a chance to address the regression.
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffda116700 (LWP 15311)]
0x00007ffff7400890 in virCgroupPathOfController (group=0x0, controller=2,
key=0x7ffff768a019 "tasks", path=0x7fffda115b30) at
util/vircgroup.c:1867
1867 if (group->controllers[controller].mountPoint == NULL) {
(gdb) bt
#0 0x00007ffff7400890 in virCgroupPathOfController (group=0x0,
controller=2,
key=0x7ffff768a019 "tasks", path=0x7fffda115b30) at
util/vircgroup.c:1867
#1 0x00007ffff73fe4db in virCgroupGetValueStr (group=0x0, controller=2,
key=0x7ffff768a019 "tasks", value=0x7fffda115b80) at
util/vircgroup.c:751
#2 0x00007ffff7405673 in virCgroupHasEmptyTasks (cgroup=0x0, controller=2)
at util/vircgroup.c:3935
#3 0x00007fffdc79c687 in qemuRestoreCgroupState (vm=0x7fffd41dd8d0)
at qemu/qemu_cgroup.c:802
#4 0x00007fffdc79c80c in qemuConnectCgroup (driver=0x7fffd4152ae0,
vm=0x7fffd41dd8d0) at qemu/qemu_cgroup.c:848
#5 0x00007fffdc7b8553 in qemuProcessReconnect (opaque=0x7fffd41da150)
at qemu/qemu_process.c:3616
#6 0x00007ffff7469830 in virThreadHelper (data=0x7fffd41c5b00)
at util/virthread.c:197
#7 0x00007ffff3e65ee5 in start_thread (arg=0x7fffda116700)
at pthread_create.c:309
#8 0x00007ffff3b94b8d in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:06 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: Leave cpuset.mems in parent cgroup alone
On Wed, Dec 17, 2014 at 12:00:36AM -0700, Eric Blake wrote:
On 12/16/2014 11:51 PM, Eric Blake wrote:
> On 12/15/2014 12:58 AM, Martin Kletzander wrote:
>> Instead of setting the value of cpuset.mems once when the domain starts
>> and then re-calculating the value every time we need to change the child
>> cgroup values, leave the cgroup alone and rather set the child data
>> every time there is new cgroup created. We don't leave any task in the
>> parent group anyway. This will ease both current and future code.
>>
>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
>> ---
>> src/qemu/qemu_cgroup.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++--
>> src/qemu/qemu_driver.c | 59 +++++++++++++++-----------------------------
>> 2 files changed, 85 insertions(+), 41 deletions(-)
>
> This patch causes libvirtd to segfault on startup:
More particularly, I'm doing an upgrade from older libvirtd to this
version, while leaving a transient domain running that was started by
the older libvirtd. Hope that helps you narrow in on the problem.
I tried that and it works for me. And I tried various domains, both
heavily cgroup dependent and simple ones.
Reverting 86759e and af2a1f0 was sufficient to get me going again
locally, but I'm not going to push my reversions until you've first had
a chance to address the regression.
>
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffda116700 (LWP 15311)]
> 0x00007ffff7400890 in virCgroupPathOfController (group=0x0, controller=2,
> key=0x7ffff768a019 "tasks", path=0x7fffda115b30) at
> util/vircgroup.c:1867
> 1867 if (group->controllers[controller].mountPoint == NULL) {
> (gdb) bt
> #0 0x00007ffff7400890 in virCgroupPathOfController (group=0x0,
> controller=2,
> key=0x7ffff768a019 "tasks", path=0x7fffda115b30) at
> util/vircgroup.c:1867
> #1 0x00007ffff73fe4db in virCgroupGetValueStr (group=0x0, controller=2,
> key=0x7ffff768a019 "tasks", value=0x7fffda115b80) at
> util/vircgroup.c:751
> #2 0x00007ffff7405673 in virCgroupHasEmptyTasks (cgroup=0x0, controller=2)
> at util/vircgroup.c:3935
From this line it looks like priv->cgroup was not initialized. I did
not add a check for that, so that may be the cause. I'll send a patch
soon.
But I wonder how did you manage to do that, is that a session libvirtd
you restarted? Otherwise how come virCgroupNewDetectMachine() didn't
fill the cgroup?
> #3 0x00007fffdc79c687 in qemuRestoreCgroupState
(vm=0x7fffd41dd8d0)
> at qemu/qemu_cgroup.c:802
> #4 0x00007fffdc79c80c in qemuConnectCgroup (driver=0x7fffd4152ae0,
> vm=0x7fffd41dd8d0) at qemu/qemu_cgroup.c:848
> #5 0x00007fffdc7b8553 in qemuProcessReconnect (opaque=0x7fffd41da150)
> at qemu/qemu_process.c:3616
> #6 0x00007ffff7469830 in virThreadHelper (data=0x7fffd41c5b00)
> at util/virthread.c:197
> #7 0x00007ffff3e65ee5 in start_thread (arg=0x7fffda116700)
> at pthread_create.c:309
> #8 0x00007ffff3b94b8d in clone ()
> at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:59 p.m.
New subject: [libvirt] [PATCH 5/6] qemu: Leave cpuset.mems in parent cgroup alone
On 12/17/2014 08:06 AM, Martin Kletzander wrote:
On Wed, Dec 17, 2014 at 12:00:36AM -0700, Eric Blake wrote:
> On 12/16/2014 11:51 PM, Eric Blake wrote:
>> On 12/15/2014 12:58 AM, Martin Kletzander wrote:
>>> Instead of setting the value of cpuset.mems once when the domain starts
>>> and then re-calculating the value every time we need to change the
>>> child
>>> cgroup values, leave the cgroup alone and rather set the child data
>>> every time there is new cgroup created. We don't leave any task in the
>>> parent group anyway. This will ease both current and future code.
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
>>> ---
>>> src/qemu/qemu_cgroup.c | 67
>>> ++++++++++++++++++++++++++++++++++++++++++++++++--
>>> src/qemu/qemu_driver.c | 59
>>> +++++++++++++++-----------------------------
>>> 2 files changed, 85 insertions(+), 41 deletions(-)
>>
>> This patch causes libvirtd to segfault on startup:
>
> More particularly, I'm doing an upgrade from older libvirtd to this
> version, while leaving a transient domain running that was started by
> the older libvirtd. Hope that helps you narrow in on the problem.
Weird - At the time I made the report, I ran 'git bisect' and reliably
reproduced the crash across multiple libvirtd restarts (a restart for
each new build while trying to nail down the culprit commit), as long as
I left that transient domain running.
>
I tried that and it works for me. And I tried various domains, both
heavily cgroup dependent and simple ones.
But now that I've rebooted, and then proceeded to do incremental builds
from both before and after the patch, I can't reproduce the crash.
Although my formula for creating my transient domain was the same both
yesterday and today, there may be some difference in the version of
libvirtd that was running at the time I created the domain that then
affected the XML affecting the libvirtd restarts.
> Reverting 86759e and af2a1f0 was sufficient to get me going again
> locally, but I'm not going to push my reversions until you've first had
> a chance to address the regression.
>
>> #2 0x00007ffff7405673 in virCgroupHasEmptyTasks
(cgroup=0x0,
>> controller=2)
>> at util/vircgroup.c:3935
From this line it looks like priv->cgroup was not initialized. I did
not add a check for that, so that may be the cause. I'll send a patch
soon.
But I wonder how did you manage to do that, is that a session libvirtd
you restarted? Otherwise how come virCgroupNewDetectMachine() didn't
fill the cgroup?
I'm not spotting anything obvious why it wasn't initialized at the time
I reproduced the crash, nor why I can't reproduce it now. Hopefully
it's not a lurking time bomb.
If it happens again, I'll definitely report it; but for now, without a
reliable reproduction, it could easily have been something caused on my
end (since it is my dev machine, it may have been caused by a half-baked
patch on my end that I was testing at the time I created the transient
domain, where using only upstream patches wouldn't see the issue). So
for now, don't worry too hard if you can't find it either.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:45 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: Leave cpuset.mems in parent cgroup alone
On Wed, Dec 17, 2014 at 04:59:30PM -0700, Eric Blake wrote:
On 12/17/2014 08:06 AM, Martin Kletzander wrote:
> On Wed, Dec 17, 2014 at 12:00:36AM -0700, Eric Blake wrote:
>> On 12/16/2014 11:51 PM, Eric Blake wrote:
>>> On 12/15/2014 12:58 AM, Martin Kletzander wrote:
>>>> Instead of setting the value of cpuset.mems once when the domain starts
>>>> and then re-calculating the value every time we need to change the
>>>> child
>>>> cgroup values, leave the cgroup alone and rather set the child data
>>>> every time there is new cgroup created. We don't leave any task in
the
>>>> parent group anyway. This will ease both current and future code.
>>>>
>>>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
>>>> ---
>>>> src/qemu/qemu_cgroup.c | 67
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++--
>>>> src/qemu/qemu_driver.c | 59
>>>> +++++++++++++++-----------------------------
>>>> 2 files changed, 85 insertions(+), 41 deletions(-)
>>>
>>> This patch causes libvirtd to segfault on startup:
>>
>> More particularly, I'm doing an upgrade from older libvirtd to this
>> version, while leaving a transient domain running that was started by
>> the older libvirtd. Hope that helps you narrow in on the problem.
Weird - At the time I made the report, I ran 'git bisect' and reliably
reproduced the crash across multiple libvirtd restarts (a restart for
each new build while trying to nail down the culprit commit), as long as
I left that transient domain running.
>>
>
> I tried that and it works for me. And I tried various domains, both
> heavily cgroup dependent and simple ones.
But now that I've rebooted, and then proceeded to do incremental builds
from both before and after the patch, I can't reproduce the crash.
Although my formula for creating my transient domain was the same both
yesterday and today, there may be some difference in the version of
libvirtd that was running at the time I created the domain that then
affected the XML affecting the libvirtd restarts.
>
>> Reverting 86759e and af2a1f0 was sufficient to get me going again
>> locally, but I'm not going to push my reversions until you've first had
>> a chance to address the regression.
>>
>>> #2 0x00007ffff7405673 in virCgroupHasEmptyTasks (cgroup=0x0,
>>> controller=2)
>>> at util/vircgroup.c:3935
>
> From this line it looks like priv->cgroup was not initialized. I did
> not add a check for that, so that may be the cause. I'll send a patch
> soon.
>
> But I wonder how did you manage to do that, is that a session libvirtd
> you restarted? Otherwise how come virCgroupNewDetectMachine() didn't
> fill the cgroup?
I'm not spotting anything obvious why it wasn't initialized at the time
I reproduced the crash, nor why I can't reproduce it now. Hopefully
it's not a lurking time bomb.
If it happens again, I'll definitely report it; but for now, without a
reliable reproduction, it could easily have been something caused on my
end (since it is my dev machine, it may have been caused by a half-baked
patch on my end that I was testing at the time I created the transient
domain, where using only upstream patches wouldn't see the issue). So
for now, don't worry too hard if you can't find it either.
There is a valid code path that allows the cgroup to be NULL, although
not easy to reach. However, since it is valid, we added a patch that
just skips the cgroup restoration for cgroup == NULL.
If you happen to stumble upon a similar problem, I'd be happy to try
dealing with it.
Martin
5:40 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: Leave cpuset.mems in parent cgroup alone
On Mon, Dec 22, 2014 at 10:45:33AM +0100, Martin Kletzander wrote:
On Wed, Dec 17, 2014 at 04:59:30PM -0700, Eric Blake wrote:
>On 12/17/2014 08:06 AM, Martin Kletzander wrote:
>> On Wed, Dec 17, 2014 at 12:00:36AM -0700, Eric Blake wrote:
>>> On 12/16/2014 11:51 PM, Eric Blake wrote:
>>>> On 12/15/2014 12:58 AM, Martin Kletzander wrote:
>>>>> Instead of setting the value of cpuset.mems once when the domain
starts
>>>>> and then re-calculating the value every time we need to change the
>>>>> child
>>>>> cgroup values, leave the cgroup alone and rather set the child data
>>>>> every time there is new cgroup created. We don't leave any task
in the
>>>>> parent group anyway. This will ease both current and future code.
>>>>>
>>>>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
>>>>> ---
>>>>> src/qemu/qemu_cgroup.c | 67
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>> src/qemu/qemu_driver.c | 59
>>>>> +++++++++++++++-----------------------------
>>>>> 2 files changed, 85 insertions(+), 41 deletions(-)
>>>>
>>>> This patch causes libvirtd to segfault on startup:
>>>
>>> More particularly, I'm doing an upgrade from older libvirtd to this
>>> version, while leaving a transient domain running that was started by
>>> the older libvirtd. Hope that helps you narrow in on the problem.
>
>Weird - At the time I made the report, I ran 'git bisect' and reliably
>reproduced the crash across multiple libvirtd restarts (a restart for
>each new build while trying to nail down the culprit commit), as long as
>I left that transient domain running.
>
I managed to reproduce this (or actually talk to a person who did) and
found out a bit more. As you say, until the domain was running
everything was reproducible. However, when I started it with the old
libvirt second time to find out the root cause, it just worked...
>>>
>>
>> I tried that and it works for me. And I tried various domains, both
>> heavily cgroup dependent and simple ones.
>
>But now that I've rebooted, and then proceeded to do incremental builds
>from both before and after the patch, I can't reproduce the crash.
>Although my formula for creating my transient domain was the same both
>yesterday and today, there may be some difference in the version of
>libvirtd that was running at the time I created the domain that then
>affected the XML affecting the libvirtd restarts.
>
>>
>>> Reverting 86759e and af2a1f0 was sufficient to get me going again
>>> locally, but I'm not going to push my reversions until you've first
had
>>> a chance to address the regression.
>>>
>
>>>> #2 0x00007ffff7405673 in virCgroupHasEmptyTasks (cgroup=0x0,
>>>> controller=2)
>>>> at util/vircgroup.c:3935
>>
>> From this line it looks like priv->cgroup was not initialized. I did
>> not add a check for that, so that may be the cause. I'll send a patch
>> soon.
>>
>> But I wonder how did you manage to do that, is that a session libvirtd
>> you restarted? Otherwise how come virCgroupNewDetectMachine() didn't
>> fill the cgroup?
>
>I'm not spotting anything obvious why it wasn't initialized at the time
>I reproduced the crash, nor why I can't reproduce it now. Hopefully
>it's not a lurking time bomb.
>
It might be and it may cause more problems that just non-working
hot-plugging of vCPUs. And it is not caused by these patches I
added. The thing is that old libvirt starts a domain, but it
*somehow* (this is the part I was not able to find out yet) doesn't
add the task into the cgroup hierarchy for all cgroup controllers.
The /proc/$(pidof qemu-kvm)/cgroups file looks like this then:
10:hugetlb:/
9:perf_event:/machine.slice/machine-qemu\x2da.scope
8:blkio:/
7:net_cls:/machine.slice/machine-qemu\x2da.scope
6:freezer:/machine.slice/machine-qemu\x2da.scope
5:devices:/
4:memory:/
3:cpuacct,cpu:/
2:cpuset:/machine.slice/machine-qemu\x2da.scope/emulator
1:name=systemd:/machine.slice/machine-qemu\x2da.scope
And when new libvirtd is starting, it detects the cgroup mounts and
placements (in this example the placement for memory cgroup is "/")
and the it goes and validates them. If the cgroup doesn't have a
placement that ends with machine-qemu-blah or libvirt-qemu or
whatever, the following condition in virCgroupValidateMachineGroup()
fails:
if (STRNEQ(tmp, name) &&
STRNEQ(tmp, partname) &&
STRNEQ(tmp, scopename))
[in here the variable tmp points after the last slash in the partition
string]
The function returns false, hence virCgroupNewDetectMachine() frees
the cgroup (and sets it to NULL) and returns success. This means we
will still properly detect the machine but we won't have access to any
cgroup settings of that particular machine.
The patch I posted fixes it to the extent that the daemon doesn't
crash, but it doesn't fix the root cause (which is unknown).
I played with it a bit more and I've found out that a domain, which
had its own partition in all the cgroups controllers when started,
*lost* some of them after a while. I might have been caused by
libvirtd restarting, but that's highly unlikely, I guess.
Even though inotify works for cgroup fs it does not show us who
accessed it. I'll have to resort to using systemtap again.
>If it happens again, I'll definitely report it; but for now,
without a
>reliable reproduction, it could easily have been something caused on my
>end (since it is my dev machine, it may have been caused by a half-baked
>patch on my end that I was testing at the time I created the transient
>domain, where using only upstream patches wouldn't see the issue). So
>for now, don't worry too hard if you can't find it either.
>
There is a valid code path that allows the cgroup to be NULL, although
not easy to reach. However, since it is valid, we added a patch that
just skips the cgroup restoration for cgroup == NULL.
If you happen to stumble upon a similar problem, I'd be happy to try
dealing with it.
Martin
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1:58 a.m.
New subject: [libvirt] [PATCH 6/6] qemu: Fix hotplugging cpus with strict memory pinning
When hot-plugging a VCPU into the guest, kvm needs to allocate some data
from the DMA zone, which might be in a memory node that's not allowed in
cpuset.mems. Basically the same problem as there was with starting the
domain and due to which commit 7e72ac787848b7434c9359a57c1e2789d92350f8
exists. This patch just extends it to hotplugging as well.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1161540
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bfa62c4..dbe48ad 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -97,6 +97,7 @@
#include "virhostdev.h"
#include "domain_capabilities.h"
#include "vircgroup.h"
+#include "virnuma.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -4621,6 +4622,11 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
int ncpuinfo;
qemuDomainObjPrivatePtr priv;
size_t i;
+ virCgroupPtr cgroup_temp = NULL;
+ char *mem_mask = NULL;
+ char *all_nodes_str = NULL;
+ virBitmapPtr all_nodes = NULL;
+ virErrorPtr err = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -4646,9 +4652,22 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
priv = vm->privateData;
+ if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0)
+ goto cleanup;
+
+ if (!(all_nodes = virNumaGetHostNodeset()))
+ goto cleanup;
+
+ if (!(all_nodes_str = virBitmapFormat(all_nodes)))
+ goto cleanup;
+
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
+ if (virCgroupGetCpusetMems(cgroup_temp, &mem_mask) < 0 ||
+ virCgroupSetCpusetMems(cgroup_temp, all_nodes_str) < 0)
+ goto endjob;
+
maximum = (flags & VIR_DOMAIN_VCPU_MAXIMUM) != 0;
flags &= ~VIR_DOMAIN_VCPU_MAXIMUM;
@@ -4758,6 +4777,12 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
ret = 0;
endjob:
+ if (mem_mask) {
+ err = virSaveLastError();
+ virCgroupSetCpusetMems(cgroup_temp, mem_mask);
+ virSetError(err);
+ }
+
if (!qemuDomainObjEndJob(driver, vm))
vm = NULL;
@@ -4766,6 +4791,10 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virObjectUnlock(vm);
virObjectUnref(caps);
VIR_FREE(cpuinfo);
+ VIR_FREE(mem_mask);
+ VIR_FREE(all_nodes_str);
+ virBitmapFree(all_nodes);
+ virCgroupFree(&cgroup_temp);
virObjectUnref(cfg);
return ret;
}
--
2.2.0
4:52 a.m.
On 15.12.2014 08:58, Martin Kletzander wrote:
Deatils are in the patches themselves, but the basic idea is this:
Setup:
$ grep DMA32 /proc/zoneinfo
Node 0, zone DMA32
$ virsh dumpxml domain | grep -C1 strict
<numatune>
<memory mode='strict' nodeset='1'/>
</numatune>
$ virsh start domain
Domain domain started
Before:
$ virsh setvcpus domain 2
error: Unable to read from monitor: Connection reset by peer
# Domain died
After:
$ virsh setvcpus domain 2
# hotplug successful
Martin
Martin Kletzander (6):
util: Add function virCgroupHasEmptyTasks
util: Add virNumaGetHostNodeset
qemu: Remove unnecessary qemuSetupCgroupPostInit function
qemu: Save numad advice into qemuDomainObjPrivate
qemu: Leave cpuset.mems in parent cgroup alone
qemu: Fix hotplugging cpus with strict memory pinning
src/libvirt_private.syms | 2 ++
src/qemu/qemu_cgroup.c | 94 +++++++++++++++++++++++++++++++++++++-----------
src/qemu/qemu_cgroup.h | 9 ++---
src/qemu/qemu_domain.c | 1 +
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 88 +++++++++++++++++++++++++--------------------
src/qemu/qemu_process.c | 21 ++++++-----
src/util/vircgroup.c | 23 ++++++++++++
src/util/vircgroup.h | 4 ++-
src/util/virnuma.c | 28 +++++++++++++++
src/util/virnuma.h | 1 +
11 files changed, 194 insertions(+), 78 deletions(-)
ACK series.
Michal
3655
days inactive
3663
days old
13 comments
3 participants
participants (3)
-
Eric Blake -
Martin Kletzander -
Michal Privoznik