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;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list