conf: Remove pre-calculation of initial memory size

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

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 215538a525bdfef0773ae0bbf49ffd5731d6cb61..7090c31dbbd0aeea219b8e27c66e711600455c71 100644 (file)
--- 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;
 }
 
 
@@ -14542,10 +14525,18 @@ virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
 }
 
 
+/**
+ * virDomainMemoryInsert:
+ *
+ * Inserts a memory device definition into the domain definition. This helper
+ * should be used only in hot/cold-plug 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 &&
@@ -14556,24 +14547,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;
 }
 
 
+/**
+ * virDomainMemoryRemove:
+ *
+ * Removes a memory device definition from the domain definition. This helper
+ * should be used only in hot/cold-plug 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 15f9c80d019a87ccc4f78776052383d51f309040..96ac5103eb773a501b9f8ae260f97dd077685619 100644 (file)
--- 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 af30476d3741a8ab8c9a90b5a7840092e73f9dec..748b523599652fd392210241f497654a36b9bc24 100644 (file)
--- 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 c844566d3435fc79b4d394e606e0aa7f210c24e3..26bd76876cf7dd24289984017f3650aad1ca60ef 100644 (file)
--- 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;
 }