While we need to know the difference between the total memory stored in <memory> and the actual size not included in the possible memory modules we can't pre-calculate it reliably. This is due to the fact that libvirt's XML is copied via formatting and parsing the XML and the initial memory size can be reliably calculated only when certain conditions are met due to backwards compatibility. This patch removes the storage of 'initial_memory' and fixes the helpers to recalculate the initial memory size all the time from the total memory size. This conversion is possible when we also make sure that memory hotplug accounts properly for the update of the total memory size and thus the helpers for inserting and removing memory devices need to be tweaked too. This fixes a bug where a cold-plug and cold-remove of a memory device would increase the size reported in <memory> in the XML by the size of the memory device. This would happen as the persistent definition is copied before attaching the device and this would lead to the loss of data in 'initial_memory'. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344892 --- src/conf/domain_conf.c | 85 +++++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 3 -- src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 6 ++-- 4 files changed, 49 insertions(+), 46 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9504e5f..10e9e8b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3714,21 +3714,24 @@ virDomainDefPostParseMemory(virDomainDefPtr def, parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) numaMemory = virDomainNumaGetMemorySize(def->numa); + /* calculate the sizes of hotplug memory */ + for (i = 0; i < def->nmems; i++) + hotplugMemory += def->mems[i]->size; + if (numaMemory) { - virDomainDefSetMemoryInitial(def, numaMemory); + /* update the sizes in XML if nothing was set in the XML or ABI update + * is supported*/ + virDomainDefSetMemoryTotal(def, numaMemory + hotplugMemory); } else { - /* calculate the sizes of hotplug memory */ - for (i = 0; i < def->nmems; i++) - hotplugMemory += def->mems[i]->size; - + /* verify that the sum of memory modules doesn't exceed the total + * memory. This is necessary for virDomainDefGetMemoryInitial to work + * properly. */ if (hotplugMemory > def->mem.total_memory) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Total size of memory devices exceeds the total " "memory size")); return -1; } - - virDomainDefSetMemoryInitial(def, def->mem.total_memory - hotplugMemory); } if (virDomainDefGetMemoryInitial(def) == 0) { @@ -8041,13 +8044,18 @@ virDomainDefHasMemoryHotplug(const virDomainDef *def) * @def: domain definition * * Returns the size of the initial amount of guest memory. The initial amount - * is the memory size is either the configured amount in the <memory> element - * or the sum of memory sizes of NUMA nodes in case NUMA is enabled in @def. + * is the memory size excluding possible memory modules. */ unsigned long long virDomainDefGetMemoryInitial(const virDomainDef *def) { - return def->mem.initial_memory; + size_t i; + unsigned long long ret = def->mem.total_memory; + + for (i = 0; i < def->nmems; i++) + ret -= def->mems[i]->size; + + return ret; } @@ -8056,30 +8064,14 @@ virDomainDefGetMemoryInitial(const virDomainDef *def) * @def: domain definition * @size: size to set * - * Sets the total memory size in @def. This function should be used only by - * hypervisors that don't support memory hotplug. + * Sets the total memory size in @def. This value needs to include possible + * additional memory modules. */ void virDomainDefSetMemoryTotal(virDomainDefPtr def, unsigned long long size) { def->mem.total_memory = size; - def->mem.initial_memory = size; -} - - -/** - * virDomainDefSetMemoryInitial: - * @def: domain definition - * @size: size to set - * - * Sets the initial memory size (without memory modules) in @def. - */ -void -virDomainDefSetMemoryInitial(virDomainDefPtr def, - unsigned long long size) -{ - def->mem.initial_memory = size; } @@ -8088,21 +8080,12 @@ virDomainDefSetMemoryInitial(virDomainDefPtr def, * @def: domain definition * * Returns the current maximum memory size usable by the domain described by - * @def. This size is a sum of size returned by virDomainDefGetMemoryInitial - * and possible additional memory devices. + * @def. This size includes possible additional memory devices. */ unsigned long long virDomainDefGetMemoryActual(virDomainDefPtr def) { - unsigned long long ret; - size_t i; - - ret = def->mem.initial_memory; - - for (i = 0; i < def->nmems; i++) - ret += def->mems[i]->size; - - return ret; + return def->mem.total_memory; } @@ -14551,10 +14534,18 @@ virDomainMemoryFindInactiveByDef(virDomainDefPtr def, } +/** + * virDomainMemoryInsert: + * + * Inserts a memory device definition into the domain definition. This helper + * should be used only in hotplug cases as it's blindly modifying the total + * memory size. + */ int virDomainMemoryInsert(virDomainDefPtr def, virDomainMemoryDefPtr mem) { + unsigned long long memory = virDomainDefGetMemoryActual(def); int id = def->nmems; if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && @@ -14565,24 +14556,38 @@ virDomainMemoryInsert(virDomainDefPtr def, return -1; } - if (VIR_APPEND_ELEMENT(def->mems, def->nmems, mem) < 0) + if (VIR_APPEND_ELEMENT_COPY(def->mems, def->nmems, mem) < 0) return -1; + virDomainDefSetMemoryTotal(def, memory + mem->size); + return id; } +/** + * virDomainMemoryInsert: + * + * Removes a memory device definition into the domain definition. This helper + * should be used only in hotplug cases as it's blindly modifying the total + * memory size. + */ virDomainMemoryDefPtr virDomainMemoryRemove(virDomainDefPtr def, int idx) { + unsigned long long memory = virDomainDefGetMemoryActual(def); virDomainMemoryDefPtr ret = def->mems[idx]; + VIR_DELETE_ELEMENT(def->mems, idx, def->nmems); /* fix up balloon size */ if (def->mem.cur_balloon > virDomainDefGetMemoryActual(def)) def->mem.cur_balloon = virDomainDefGetMemoryActual(def); + /* fix total memory size of the domain */ + virDomainDefSetMemoryTotal(def, memory - ret->size); + return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 15f9c80..96ac510 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2070,8 +2070,6 @@ struct _virDomainMemtune { /* total memory size including memory modules in kibibytes, this field * should be accessed only via accessors */ unsigned long long total_memory; - /* initial memory size in kibibytes = total_memory excluding memory modules*/ - unsigned long long initial_memory; unsigned long long cur_balloon; /* in kibibytes, capped at ulong thanks to virDomainGetInfo */ @@ -2272,7 +2270,6 @@ virDomainVcpuInfoPtr virDomainDefGetVcpu(virDomainDefPtr def, unsigned int vcpu) unsigned long long virDomainDefGetMemoryInitial(const virDomainDef *def); void virDomainDefSetMemoryTotal(virDomainDefPtr def, unsigned long long size); -void virDomainDefSetMemoryInitial(virDomainDefPtr def, unsigned long long size); unsigned long long virDomainDefGetMemoryActual(virDomainDefPtr def); bool virDomainDefHasMemoryHotplug(const virDomainDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e939de3..3d1716c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -240,7 +240,6 @@ virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; virDomainDefPostParse; -virDomainDefSetMemoryInitial; virDomainDefSetMemoryTotal; virDomainDefSetVcpus; virDomainDefSetVcpusMax; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d1f8175..8df3996 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4718,6 +4718,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) unsigned long long maxmemkb = virMemoryMaxValue(false) >> 10; unsigned long long maxmemcapped = virMemoryMaxValue(true) >> 10; unsigned long long initialmem = 0; + unsigned long long hotplugmem = 0; unsigned long long mem; unsigned long long align = qemuDomainGetMemorySizeAlignment(def); size_t ncells = virDomainNumaGetNodeCount(def->numa); @@ -4748,8 +4749,6 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) return -1; } - virDomainDefSetMemoryInitial(def, initialmem); - def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align); if (def->mem.max_memory > maxmemkb) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -4761,6 +4760,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) for (i = 0; i < def->nmems; i++) { align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]); def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align); + hotplugmem += def->mems[i]->size; if (def->mems[i]->size > maxmemkb) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4770,6 +4770,8 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) } } + virDomainDefSetMemoryTotal(def, initialmem + hotplugmem); + return 0; } -- 2.8.3 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list