On 10/16/2015 08:11 AM, Peter Krempa wrote:
Add a function that will aggregate various checks related to memory hotplug so that they aren't scattered accross various parts of the code. --- src/qemu/qemu_command.c | 26 ++--------------- src/qemu/qemu_domain.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_hotplug.c | 9 ++---- 4 files changed, 87 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 43e81c7..4a709db 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9414,26 +9414,12 @@ qemuBuildCommandLine(virConnectPtr conn, qemuDomainAlignMemorySizes(def) < 0) goto error;
+ if (qemuDomainDefValidateMemoryHotplug(def, qemuCaps, NULL) < 0) + goto error; + virCommandAddArg(cmd, "-m");
if (virDomainDefHasMemoryHotplug(def)) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("memory hotplug isn't supported by this QEMU binary")); - goto error; - } - - /* due to guest support, qemu would silently enable NUMA with one node - * once the memory hotplug backend is enabled. To avoid possible - * confusion we will enforce user originated numa configuration along - * with memory hotplug. */ - if (virDomainNumaGetNodeCount(def->numa) == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("At least one numa node has to be configured when " - "enabling memory hotplug")); - goto error; - } - /* Use the 'k' suffix to let qemu handle the units */ virCommandAddArgFormat(cmd, "size=%lluk,slots=%u,maxmem=%lluk", virDomainDefGetMemoryInitial(def), @@ -9491,12 +9477,6 @@ qemuBuildCommandLine(virConnectPtr conn, /* memory hotplug requires NUMA to be enabled - we already checked * that memory devices are present only when NUMA is */
Does the comment make sense any more?
- if (def->nmems > def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device count '%zu' exceeds slots count '%u'"), - def->nmems, def->mem.memory_slots); - goto error; - }
for (i = 0; i < def->nmems; i++) { char *backStr; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bdc0e67..cb50754 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3553,6 +3553,83 @@ qemuDomainMachineIsS390CCW(const virDomainDef *def)
/** + * qemuDomainDefValidateMemoryHotplug:
Not necessarily called only in Hotplug path, but I understand why it's named this way since *HasMemoryHotplug uses similar naming and adding a Capable on the end is an unnecessarily long name.
+ * @def: domain definition + * @qemuCaps: qemu capabilities object + * @mem: definition of memory device that is to be added to @def with hotplug
or NULL when ...
+ * + * Validates that the domain definition and memory modules have valid + * configuration and are possibly able to accept @mem via hotplug if it's + * passed. + * + * Returns 0 on success; -1 and a libvirt error on error. + */ +int +qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + const virDomainMemoryDef *mem) +{ + unsigned int nmems = def->nmems; + unsigned long long hotplugSpace; + unsigned long long hotplugMemory = 0; + size_t i; + + hotplugSpace = def->mem.max_memory - virDomainDefGetMemoryInitial(def); + + if (mem) { + nmems++; + hotplugMemory = mem->size;
I think you'd have to move qemuDomainMemoryDeviceAlignSize to sooner in the caller so that this calculation and future check is more correct.
+ } + + if (!virDomainDefHasMemoryHotplug(def)) { + if (nmems) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("maxMemory has to be specified when using memory " + "devices "));
extraneous space ^ Although I have to say the check and error reads strange... The *Has function returns true when memory_slots || max_memory > 0. So while it's obvious knowing the code and XML that maxMemory needs to be defined in order for hotplug to work, it just a strange message to see for someone perhaps passing memory device XML. Essentially there's a failure because someone was requesting to hotplug a memory device, but the domain doesn't support it. Perhaps, "cannot hotplug a memory device when domain 'maxMemory' is not defined" ?
+ return -1; + } + + return 0; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("memory hotplug isn't supported by this QEMU binary")); + return -1; + } + + /* due to guest support, qemu would silently enable NUMA with one node + * once the memory hotplug backend is enabled. To avoid possible + * confusion we will enforce user originated numa configuration along + * with memory hotplug. */ + if (virDomainNumaGetNodeCount(def->numa) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("At least one numa node has to be configured when " + "enabling memory hotplug")); + return -1; + } + + if (nmems > def->mem.memory_slots) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device count '%u' exceeds slots count '%u'"), + nmems, def->mem.memory_slots);
This is one of those cases where different error messages based on operation may be useful. In one case (hotplug) one cannot add a memory device because there's no slots, while the other case (domain startup) we cannot startup the domain because we have more 'nmems' provided than 'memory_slots' available.
+ return -1; + } + + for (i = 0; i < def->nmems; i++) + hotplugMemory += def->mems[i]->size; + + if (hotplugMemory > hotplugSpace) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("memory device total size exceeds hotplug space")); + return -1; + } + + return 0; +} + + +/** * qemuDomainUpdateCurrentMemorySize: * * Updates the current balloon size from the monitor if necessary. In case when diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 64cd7e1..23f5b1f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -482,4 +482,8 @@ bool qemuDomainMachineIsS390CCW(const virDomainDef *def); int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, virDomainObjPtr vm);
+int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + const virDomainMemoryDef *mem); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index afc5408..2c7f165 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1784,18 +1784,15 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, int id; int ret = -1;
- if (vm->def->nmems == vm->def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("no free memory device slot available")); - goto cleanup; - } - if (virAsprintf(&mem->info.alias, "dimm%zu", vm->def->nmems) < 0) goto cleanup;
if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0) goto cleanup;
+ if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0) + goto cleanup; +
Why wait? It's not like validate routine is checking the alias. ACK with a couple of tweaks John
if (vm->def->mem.cur_balloon == virDomainDefGetMemoryActual(vm->def)) fix_balloon = true;