[libvirt] [PATCH 00/13] Memory alignment vs. migration fixes
The refactorings that I've done in preparation for memory hotplug had the right idea in regards to handling of the various memory size fields in libvirt, but the execution of the refactors was suboptimal for migration compatibilty. This patchset fixes problems when migrating from older libvirt versions that allowed specifying a config where contents <memory> were not equal to the total size of all NUMA nodes. Additionally a few Patches 01-05/13 are small cleanups to various parts of the code. Patch 06/13 adds a XML parser flag that will be used later on. Patches 07-08 do a few more cleanups in the memory parsing code. Patch 12/13 in this series isn't entirely relevant to this series, but depends on 09-11/13 so I've included it here. The patch modifies the alignment for PPC machines that started requiring alignment to 256MiB in some cases. As we now have a way to keep migrations intact we can switch the alignment always. Patch 13/13 adds a test case based on an existing test to check that the sizing code did not regress (again ...). Peter Krempa (13): libxl: vz: Use accessor instead of direct access for max_balloon conf: Add helper to determine whether memory hotplug is enabled for a vm qemu: Make memory alignment helper more universal conf: Drop VIR_DOMAIN_DEF_PARSE_CLOCK_ADJUST flag conf: Document all VIR_DOMAIN_DEF_PARSE_* flags conf: Add XML parser flag that will allow us to do incompatible updates conf: Split memory related post parse stuff into separate function conf: Rename max_balloon to total_memory conf: Pre-calculate initial memory size instead of always calculating it conf: Don't always recalculate initial memory size from NUMA size totals qemu: command: Align memory sizes only on fresh starts qemu: ppc64: Align memory sizes to 256MiB blocks test: Add test to validate that memory sizes don't get updated on migration src/conf/domain_conf.c | 113 +++++++++++++++------ src/conf/domain_conf.h | 29 ++++-- src/hyperv/hyperv_driver.c | 2 +- src/libvirt_private.syms | 2 + src/libxl/libxl_driver.c | 4 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_native.c | 4 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_command.c | 12 ++- src/qemu/qemu_domain.c | 44 +++++--- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_driver.c | 14 ++- src/qemu/qemu_hotplug.c | 4 +- src/qemu/qemu_migration.c | 8 +- src/test/test_driver.c | 2 +- src/uml/uml_driver.c | 2 +- src/vbox/vbox_common.c | 4 +- src/vmx/vmx.c | 2 +- src/vz/vz_driver.c | 2 +- src/vz/vz_sdk.c | 2 +- src/xen/xm_internal.c | 2 +- src/xenapi/xenapi_driver.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- .../qemuxml2argv-migrate-numa-unaligned.args | 13 +++ .../qemuxml2argv-migrate-numa-unaligned.xml | 33 ++++++ .../qemuxml2argv-pseries-cpu-compat.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-restore-v1.args | 2 +- tests/qemuxml2argvtest.c | 13 ++- tests/qemuxml2xmltest.c | 3 +- 30 files changed, 238 insertions(+), 93 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml -- 2.4.5
Commits 45697fe5 and f863ac80 used direct access to the variable instead of the preferred accessor method. --- src/libxl/libxl_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 8087c27..019f04f 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -553,7 +553,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver) vm->def->vcpus = d_info.vcpu_online; vm->def->maxvcpus = d_info.vcpu_max_id + 1; vm->def->mem.cur_balloon = d_info.current_memkb; - vm->def->mem.max_balloon = d_info.max_memkb; + virDomainDefSetMemoryInitial(vm->def, d_info.max_memkb); ret = 0; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 8fa7957..ba5d7e4 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1200,7 +1200,7 @@ vzDomainGetMaxMemory(virDomainPtr domain) if (!(dom = vzDomObjFromDomain(domain))) return -1; - ret = dom->def->mem.max_balloon; + ret = virDomainDefGetMemoryActual(dom->def->mem); virObjectUnlock(dom); return ret; } -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
Commits 45697fe5 and f863ac80 used direct access to the variable instead of the preferred accessor method. --- src/libxl/libxl_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 8087c27..019f04f 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -553,7 +553,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver) vm->def->vcpus = d_info.vcpu_online; vm->def->maxvcpus = d_info.vcpu_max_id + 1; vm->def->mem.cur_balloon = d_info.current_memkb; - vm->def->mem.max_balloon = d_info.max_memkb; + virDomainDefSetMemoryInitial(vm->def, d_info.max_memkb);
ret = 0;
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 8fa7957..ba5d7e4 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1200,7 +1200,7 @@ vzDomainGetMaxMemory(virDomainPtr domain) if (!(dom = vzDomObjFromDomain(domain))) return -1;
- ret = dom->def->mem.max_balloon; + ret = virDomainDefGetMemoryActual(dom->def->mem); virObjectUnlock(dom); return ret; }
ACK with this squashed in: diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index ba5d7e4..2eb025c 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1200,7 +1200,7 @@ vzDomainGetMaxMemory(virDomainPtr domain) if (!(dom = vzDomObjFromDomain(domain))) return -1; - ret = virDomainDefGetMemoryActual(dom->def->mem); + ret = virDomainDefGetMemoryActual(dom->def); virObjectUnlock(dom); return ret; } Michal
Add a simple helper so that the code doesn't have to rewrite the same condition multiple times. --- src/conf/domain_conf.c | 9 ++++++++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_migration.c | 5 ++--- 6 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3b3ccb..fa2e331 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1154,7 +1154,7 @@ int virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def) { /* memory hotplug tunables are not supported by this driver */ - if (def->mem.max_memory > 0 || def->mem.memory_slots > 0) { + if (virDomainDefHasMemoryHotplug(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("memory hotplug tunables <maxMemory> are not " "supported by this hypervisor driver")); @@ -7671,6 +7671,13 @@ virDomainParseMemoryLimit(const char *xpath, } +bool +virDomainDefHasMemoryHotplug(const virDomainDef *def) +{ + return def->mem.memory_slots > 0 || def->mem.max_memory > 0; +} + + /** * virDomainDefGetMemoryInitial: * @def: domain definition diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8be390b..cfd2600 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2324,6 +2324,7 @@ struct _virDomainDef { unsigned long long virDomainDefGetMemoryInitial(virDomainDefPtr def); void virDomainDefSetMemoryInitial(virDomainDefPtr def, unsigned long long size); unsigned long long virDomainDefGetMemoryActual(virDomainDefPtr def); +bool virDomainDefHasMemoryHotplug(const virDomainDef *def); typedef enum { VIR_DOMAIN_KEY_WRAP_CIPHER_NAME_AES, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8c1f388..1a92422 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -217,6 +217,7 @@ virDomainDefGetMemoryActual; virDomainDefGetMemoryInitial; virDomainDefGetSecurityLabelDef; virDomainDefHasDeviceAddress; +virDomainDefHasMemoryHotplug; virDomainDefMaybeAddController; virDomainDefMaybeAddInput; virDomainDefNeedsPlacementAdvice; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 25f57f2..e1f199c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9323,7 +9323,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-m"); - if (def->mem.max_memory) { + 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")); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e4a88cd..f840b0d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1367,7 +1367,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } if (dev->type == VIR_DOMAIN_DEVICE_MEMORY && - def->mem.max_memory == 0) { + !virDomainDefHasMemoryHotplug(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("maxMemory has to be specified when using memory " "devices ")); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 903612b..948ad3b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3000,10 +3000,9 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, } } - if (vm->def->mem.max_memory || + if (virDomainDefHasMemoryHotplug(vm->def) || ((flags & VIR_MIGRATE_PERSIST_DEST) && - vm->newDef && - vm->newDef->mem.max_memory)) + vm->newDef && virDomainDefHasMemoryHotplug(vm->newDef))) cookieFlags |= QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG; if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
Add a simple helper so that the code doesn't have to rewrite the same condition multiple times. --- src/conf/domain_conf.c | 9 ++++++++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_migration.c | 5 ++--- 6 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3b3ccb..fa2e331 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1154,7 +1154,7 @@ int virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def) { /* memory hotplug tunables are not supported by this driver */ - if (def->mem.max_memory > 0 || def->mem.memory_slots > 0) { + if (virDomainDefHasMemoryHotplug(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("memory hotplug tunables <maxMemory> are not " "supported by this hypervisor driver")); @@ -7671,6 +7671,13 @@ virDomainParseMemoryLimit(const char *xpath, }
+bool +virDomainDefHasMemoryHotplug(const virDomainDef *def) +{ + return def->mem.memory_slots > 0 || def->mem.max_memory > 0; +} +
There are some other occurrences of this pattern too, e.g.: virDomainDefPostParseInternal virDomainDefFormatInternal Probably worth 'fixing' those places too.
+ /** * virDomainDefGetMemoryInitial: * @def: domain definition diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8be390b..cfd2600 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2324,6 +2324,7 @@ struct _virDomainDef { unsigned long long virDomainDefGetMemoryInitial(virDomainDefPtr def); void virDomainDefSetMemoryInitial(virDomainDefPtr def, unsigned long long size); unsigned long long virDomainDefGetMemoryActual(virDomainDefPtr def); +bool virDomainDefHasMemoryHotplug(const virDomainDef *def);
typedef enum { VIR_DOMAIN_KEY_WRAP_CIPHER_NAME_AES, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8c1f388..1a92422 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -217,6 +217,7 @@ virDomainDefGetMemoryActual; virDomainDefGetMemoryInitial; virDomainDefGetSecurityLabelDef; virDomainDefHasDeviceAddress; +virDomainDefHasMemoryHotplug; virDomainDefMaybeAddController; virDomainDefMaybeAddInput; virDomainDefNeedsPlacementAdvice; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 25f57f2..e1f199c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9323,7 +9323,7 @@ qemuBuildCommandLine(virConnectPtr conn,
virCommandAddArg(cmd, "-m");
- if (def->mem.max_memory) { + 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")); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e4a88cd..f840b0d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1367,7 +1367,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, }
if (dev->type == VIR_DOMAIN_DEVICE_MEMORY && - def->mem.max_memory == 0) { + !virDomainDefHasMemoryHotplug(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("maxMemory has to be specified when using memory " "devices ")); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 903612b..948ad3b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3000,10 +3000,9 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, } }
- if (vm->def->mem.max_memory || + if (virDomainDefHasMemoryHotplug(vm->def) || ((flags & VIR_MIGRATE_PERSIST_DEST) && - vm->newDef && - vm->newDef->mem.max_memory)) + vm->newDef && virDomainDefHasMemoryHotplug(vm->newDef))) cookieFlags |= QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG;
if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0)))
ACK Michal
On Tue, Sep 22, 2015 at 14:29:26 +0200, Michal Privoznik wrote:
On 21.09.2015 19:21, Peter Krempa wrote:
Add a simple helper so that the code doesn't have to rewrite the same condition multiple times. --- src/conf/domain_conf.c | 9 ++++++++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_migration.c | 5 ++--- 6 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3b3ccb..fa2e331 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1154,7 +1154,7 @@ int virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def) { /* memory hotplug tunables are not supported by this driver */ - if (def->mem.max_memory > 0 || def->mem.memory_slots > 0) { + if (virDomainDefHasMemoryHotplug(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("memory hotplug tunables <maxMemory> are not " "supported by this hypervisor driver")); @@ -7671,6 +7671,13 @@ virDomainParseMemoryLimit(const char *xpath, }
+bool +virDomainDefHasMemoryHotplug(const virDomainDef *def) +{ + return def->mem.memory_slots > 0 || def->mem.max_memory > 0; +} +
There are some other occurrences of this pattern too, e.g.:
virDomainDefPostParseInternal
Well this place makes sure that both the slot count and maximum size were specified so I think it makes sense to leave the condition to stay explicit as it's now so that it's more clear what's happening there.
virDomainDefFormatInternal
I'll change this one.
Probably worth 'fixing' those places too.
Peter
Extract the size determination into a separate function and reuse it across the memory device alignment functions. Since later we will need to decide the alignment size according to architecture let's pass def to the functions. --- src/qemu/qemu_domain.c | 26 ++++++++++++++++++-------- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_hotplug.c | 4 ++-- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f840b0d..a5e9b75 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3367,30 +3367,39 @@ qemuDomainAgentAvailable(virDomainObjPtr vm, } +static unsigned long long +qemuDomainGetMemorySizeAlignment(virDomainDefPtr def ATTRIBUTE_UNUSED) +{ + /* Align memory size. QEMU requires rounding to next 4KiB block. + * We'll take the "traditional" path and round it to 1MiB*/ + + return 1024; +} + + int qemuDomainAlignMemorySizes(virDomainDefPtr def) { unsigned long long mem; + unsigned long long align = qemuDomainGetMemorySizeAlignment(def); size_t ncells = virDomainNumaGetNodeCount(def->numa); size_t i; /* align NUMA cell sizes if relevant */ for (i = 0; i < ncells; i++) { mem = virDomainNumaGetNodeMemorySize(def->numa, i); - virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(mem, 1024)); + virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(mem, align)); } /* align initial memory size */ mem = virDomainDefGetMemoryInitial(def); - virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, 1024)); + virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, align)); - /* Align maximum memory size. QEMU requires rounding to next 4KiB block. - * We'll take the "traditional" path and round it to 1MiB*/ - def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, 1024); + def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align); /* Align memory module sizes */ for (i = 0; i < def->nmems; i++) - qemuDomainMemoryDeviceAlignSize(def->mems[i]); + def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align); return 0; } @@ -3405,9 +3414,10 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) * size so this should be safe). */ void -qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem) +qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, + virDomainMemoryDefPtr mem) { - mem->size = VIR_ROUND_UP(mem->size, 1024); + mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def)); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8cf535f..64cd7e1 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -469,7 +469,8 @@ bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool copy_only) ATTRIBUTE_NONNULL(1); int qemuDomainAlignMemorySizes(virDomainDefPtr def); -void qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem); +void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, + virDomainMemoryDefPtr mem); virDomainChrSourceDefPtr qemuFindAgentConfig(virDomainDefPtr def); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4797836..afc5408 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1802,7 +1802,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (!(devstr = qemuBuildMemoryDeviceStr(mem, vm->def, priv->qemuCaps))) goto cleanup; - qemuDomainMemoryDeviceAlignSize(mem); + qemuDomainMemoryDeviceAlignSize(vm->def, mem); if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, mem->targetNode, mem->sourceNodes, NULL, @@ -4273,7 +4273,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMemoryDeviceAlignSize(memdef); + qemuDomainMemoryDeviceAlignSize(vm->def, memdef); if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
Extract the size determination into a separate function and reuse it across the memory device alignment functions. Since later we will need to decide the alignment size according to architecture let's pass def to the functions. --- src/qemu/qemu_domain.c | 26 ++++++++++++++++++-------- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_hotplug.c | 4 ++-- 3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f840b0d..a5e9b75 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3367,30 +3367,39 @@ qemuDomainAgentAvailable(virDomainObjPtr vm, }
+static unsigned long long +qemuDomainGetMemorySizeAlignment(virDomainDefPtr def ATTRIBUTE_UNUSED) +{ + /* Align memory size. QEMU requires rounding to next 4KiB block. + * We'll take the "traditional" path and round it to 1MiB*/ + + return 1024; +} + + int qemuDomainAlignMemorySizes(virDomainDefPtr def) { unsigned long long mem; + unsigned long long align = qemuDomainGetMemorySizeAlignment(def);
How about: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a5e9b75..807112c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3381,7 +3381,7 @@ int qemuDomainAlignMemorySizes(virDomainDefPtr def) { unsigned long long mem; - unsigned long long align = qemuDomainGetMemorySizeAlignment(def); + unsigned long long const align = qemuDomainGetMemorySizeAlignment(def); size_t ncells = virDomainNumaGetNodeCount(def->numa); size_t i;
size_t ncells = virDomainNumaGetNodeCount(def->numa); size_t i;
/* align NUMA cell sizes if relevant */ for (i = 0; i < ncells; i++) { mem = virDomainNumaGetNodeMemorySize(def->numa, i); - virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(mem, 1024)); + virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(mem, align)); }
/* align initial memory size */ mem = virDomainDefGetMemoryInitial(def); - virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, 1024)); + virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, align));
- /* Align maximum memory size. QEMU requires rounding to next 4KiB block. - * We'll take the "traditional" path and round it to 1MiB*/ - def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, 1024); + def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align);
/* Align memory module sizes */ for (i = 0; i < def->nmems; i++) - qemuDomainMemoryDeviceAlignSize(def->mems[i]); + def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align);
return 0; } @@ -3405,9 +3414,10 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) * size so this should be safe). */ void -qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem) +qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, + virDomainMemoryDefPtr mem) { - mem->size = VIR_ROUND_UP(mem->size, 1024); + mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def)); }
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8cf535f..64cd7e1 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -469,7 +469,8 @@ bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool copy_only) ATTRIBUTE_NONNULL(1);
int qemuDomainAlignMemorySizes(virDomainDefPtr def); -void qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem); +void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, + virDomainMemoryDefPtr mem);
virDomainChrSourceDefPtr qemuFindAgentConfig(virDomainDefPtr def);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4797836..afc5408 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1802,7 +1802,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (!(devstr = qemuBuildMemoryDeviceStr(mem, vm->def, priv->qemuCaps))) goto cleanup;
- qemuDomainMemoryDeviceAlignSize(mem); + qemuDomainMemoryDeviceAlignSize(vm->def, mem);
if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, mem->targetNode, mem->sourceNodes, NULL, @@ -4273,7 +4273,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, return -1; }
- qemuDomainMemoryDeviceAlignSize(memdef); + qemuDomainMemoryDeviceAlignSize(vm->def, memdef);
if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s",
ACK Michal
The flag was used only for formatting the XML and once the parser and formatter flags were split in 0ecd6851093945dd5ddc78266c61b577c65394ae it doesn't make sense any more to have it. --- src/conf/domain_conf.c | 1 - src/conf/domain_conf.h | 7 +++---- tests/qemuxml2xmltest.c | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fa2e331..63dcecd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22716,7 +22716,6 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, VIR_DOMAIN_DEF_PARSE_STATUS | VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_PARSE_CLOCK_ADJUST | VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS))) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cfd2600..fcb6854 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2639,14 +2639,13 @@ typedef enum { VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES = 1 << 3, VIR_DOMAIN_DEF_PARSE_ALLOW_ROM = 1 << 4, VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT = 1 << 5, - VIR_DOMAIN_DEF_PARSE_CLOCK_ADJUST = 1 << 6, /* parse only source half of <disk> */ - VIR_DOMAIN_DEF_PARSE_DISK_SOURCE = 1 << 7, - VIR_DOMAIN_DEF_PARSE_VALIDATE = 1 << 8, + VIR_DOMAIN_DEF_PARSE_DISK_SOURCE = 1 << 6, + VIR_DOMAIN_DEF_PARSE_VALIDATE = 1 << 7, /* don't validate os.type and arch against capabilities. Prevents * VMs from disappearing when qemu is removed and libvirtd is restarted */ - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 9, + VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 8, } virDomainDefParseFlags; typedef enum { diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 5a20ebc..d7464e8 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -180,8 +180,7 @@ testCompareStatusXMLToXMLFiles(const void *opaque) driver.caps, driver.xmlopt, VIR_DOMAIN_DEF_PARSE_STATUS | VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | - VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_PARSE_CLOCK_ADJUST))) { + VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES))) { fprintf(stderr, "Failed to parse domain status XML:\n%s", source); goto cleanup; } -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
The flag was used only for formatting the XML and once the parser and formatter flags were split in 0ecd6851093945dd5ddc78266c61b577c65394ae it doesn't make sense any more to have it. --- src/conf/domain_conf.c | 1 - src/conf/domain_conf.h | 7 +++---- tests/qemuxml2xmltest.c | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-)
ACK Michal
--- src/conf/domain_conf.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fcb6854..15a8576 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2632,19 +2632,25 @@ void virDomainObjListRemoveLocked(virDomainObjListPtr doms, typedef enum { /* parse internal domain status information */ VIR_DOMAIN_DEF_PARSE_STATUS = 1 << 0, + /* Parse only parts of the XML that would be present in an inactive libvirt + * XML. Note that the flag does not imply that ABI incompatible + * transformations can be used, since it's used to strip runtime info when + * restoring save images/migration. */ VIR_DOMAIN_DEF_PARSE_INACTIVE = 1 << 1, /* parse <actual> element */ VIR_DOMAIN_DEF_PARSE_ACTUAL_NET = 1 << 2, /* parse original states of host PCI device */ VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES = 1 << 3, + /* internal flag passed to device info sub-parser to allow using <rom> */ VIR_DOMAIN_DEF_PARSE_ALLOW_ROM = 1 << 4, + /* internal flag passed to device info sub-parser to allow specifying boot order */ VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT = 1 << 5, /* parse only source half of <disk> */ VIR_DOMAIN_DEF_PARSE_DISK_SOURCE = 1 << 6, + /* perform RNG schema validation on the passed XML document */ VIR_DOMAIN_DEF_PARSE_VALIDATE = 1 << 7, /* don't validate os.type and arch against capabilities. Prevents - * VMs from disappearing when qemu is removed and libvirtd is restarted - */ + * VMs from disappearing when qemu is removed and libvirtd is restarted */ VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 8, } virDomainDefParseFlags; -- 2.4.5
Add a new parser flag that will mark code paths that parse XML files wich will not be used with existing VM state so that post parse callbacks can possibly do ABI incompatible changes if needed. --- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_driver.c | 12 ++++++++---- src/qemu/qemu_migration.c | 3 ++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 15a8576..a1221ba 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2652,6 +2652,8 @@ typedef enum { /* don't validate os.type and arch against capabilities. Prevents * VMs from disappearing when qemu is removed and libvirtd is restarted */ VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 8, + /* allow updates in post parse callback that would break ABI otherwise */ + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9, } virDomainDefParseFlags; typedef enum { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fc3b60d..3c959f6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1705,7 +1705,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD; virQEMUCapsPtr qemuCaps = NULL; virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY | @@ -7146,7 +7147,8 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, goto cleanup; def = virDomainDefParseString(xmlData, caps, driver->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE); if (!def) goto cleanup; @@ -7460,7 +7462,8 @@ static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml virQEMUCapsPtr qemuCaps = NULL; virQEMUDriverConfigPtr cfg; virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -8430,7 +8433,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; virQEMUCapsPtr qemuCaps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 948ad3b..3dd3718 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1270,7 +1270,8 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, } mig->persistent = virDomainDefParseNode(doc, nodes[0], caps, driver->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE); if (!mig->persistent) { /* virDomainDefParseNode already reported * an error for us */ -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
Add a new parser flag that will mark code paths that parse XML files wich will not be used with existing VM state so that post parse callbacks can possibly do ABI incompatible changes if needed. --- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_driver.c | 12 ++++++++---- src/qemu/qemu_migration.c | 3 ++- 3 files changed, 12 insertions(+), 5 deletions(-)
ACK Michal
The post parse func is growing rather large. Since later patches will introduce more logic in the memory post parse code, split it into a separate handler. --- src/conf/domain_conf.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 63dcecd..ca60e60 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3726,18 +3726,8 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr def) static int -virDomainDefPostParseInternal(virDomainDefPtr def, - virCapsPtr caps ATTRIBUTE_UNUSED) +virDomainDefPostParseMemory(virDomainDefPtr def) { - size_t i; - - /* verify init path for container based domains */ - if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("init binary must be specified")); - return -1; - } - if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the " @@ -3765,6 +3755,26 @@ virDomainDefPostParseInternal(virDomainDefPtr def, return -1; } + return 0; +} + + +static int +virDomainDefPostParseInternal(virDomainDefPtr def, + virCapsPtr caps ATTRIBUTE_UNUSED) +{ + size_t i; + + /* verify init path for container based domains */ + if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("init binary must be specified")); + return -1; + } + + if (virDomainDefPostParseMemory(def) < 0) + return -1; + /* * Some really crazy backcompat stuff for consoles * -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
The post parse func is growing rather large. Since later patches will introduce more logic in the memory post parse code, split it into a separate handler. --- src/conf/domain_conf.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
ACK Michal
The name of the variable was misleading. Rename it and it's setting accessor before other fixes. --- src/conf/domain_conf.c | 18 +++++++++--------- src/conf/domain_conf.h | 7 ++++--- src/hyperv/hyperv_driver.c | 2 +- src/libvirt_private.syms | 2 +- src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_native.c | 4 ++-- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/test/test_driver.c | 2 +- src/uml/uml_driver.c | 2 +- src/vbox/vbox_common.c | 4 ++-- src/vmx/vmx.c | 2 +- src/vz/vz_sdk.c | 2 +- src/xen/xm_internal.c | 2 +- src/xenapi/xenapi_driver.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- 20 files changed, 35 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ca60e60..a20ff2c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7706,27 +7706,27 @@ virDomainDefGetMemoryInitial(virDomainDefPtr def) if ((ret = virDomainNumaGetMemorySize(def->numa)) > 0) { return ret; } else { - ret = def->mem.max_balloon; + ret = def->mem.total_memory; for (i = 0; i < def->nmems; i++) ret -= def->mems[i]->size; } - return def->mem.max_balloon; + return def->mem.total_memory; } /** - * virDomainDefSetMemoryInitial: + * virDomainDefSetMemoryTotal: * @def: domain definition * @size: size to set * - * Sets the initial memory size in @def. + * Sets the total memory size in @def. */ void -virDomainDefSetMemoryInitial(virDomainDefPtr def, - unsigned long long size) +virDomainDefSetMemoryTotal(virDomainDefPtr def, + unsigned long long size) { - def->mem.max_balloon = size; + def->mem.total_memory = size; } @@ -7748,7 +7748,7 @@ virDomainDefGetMemoryActual(virDomainDefPtr def) for (i = 0; i < def->nmems; i++) ret += def->mems[i]->size; } else { - ret = def->mem.max_balloon; + ret = def->mem.total_memory; } return ret; @@ -14763,7 +14763,7 @@ virDomainDefParseXML(xmlDocPtr xml, /* Extract domain memory */ if (virDomainParseMemory("./memory[1]", NULL, ctxt, - &def->mem.max_balloon, false, true) < 0) + &def->mem.total_memory, false, true) < 0) goto error; if (virDomainParseMemory("./currentMemory[1]", NULL, ctxt, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a1221ba..74c29bd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2142,8 +2142,9 @@ typedef struct _virDomainMemtune virDomainMemtune; typedef virDomainMemtune *virDomainMemtunePtr; struct _virDomainMemtune { - unsigned long long max_balloon; /* in kibibytes, capped at ulong thanks - to virDomainGetMaxMemory */ + /* total memory size including memory modules in kibibytes, this field + * should be accessed only via accessors */ + unsigned long long total_memory; unsigned long long cur_balloon; /* in kibibytes, capped at ulong thanks to virDomainGetInfo */ @@ -2322,7 +2323,7 @@ struct _virDomainDef { }; unsigned long long virDomainDefGetMemoryInitial(virDomainDefPtr def); -void virDomainDefSetMemoryInitial(virDomainDefPtr def, unsigned long long size); +void virDomainDefSetMemoryTotal(virDomainDefPtr def, unsigned long long size); unsigned long long virDomainDefGetMemoryActual(virDomainDefPtr def); bool virDomainDefHasMemoryHotplug(const virDomainDef *def); diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index b539541..1958bbe 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -870,7 +870,7 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) if (VIR_STRDUP(def->description, virtualSystemSettingData->data->Notes) < 0) goto cleanup; - virDomainDefSetMemoryInitial(def, memorySettingData->data->Limit * 1024); /* megabyte to kilobyte */ + virDomainDefSetMemoryTotal(def, memorySettingData->data->Limit * 1024); /* megabyte to kilobyte */ def->mem.cur_balloon = memorySettingData->data->VirtualQuantity * 1024; /* megabyte to kilobyte */ def->vcpus = processorSettingData->data->VirtualQuantity; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1a92422..d31687d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -227,7 +227,7 @@ virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; virDomainDefPostParse; -virDomainDefSetMemoryInitial; +virDomainDefSetMemoryTotal; virDomainDeleteConfig; virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 019f04f..5048957 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -553,7 +553,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver) vm->def->vcpus = d_info.vcpu_online; vm->def->maxvcpus = d_info.vcpu_max_id + 1; vm->def->mem.cur_balloon = d_info.current_memkb; - virDomainDefSetMemoryInitial(vm->def, d_info.max_memkb); + virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb); ret = 0; @@ -1498,7 +1498,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (flags & VIR_DOMAIN_MEM_CONFIG) { /* Help clang 2.8 decipher the logic flow. */ sa_assert(persistentDef); - virDomainDefSetMemoryInitial(persistentDef, newmem); + virDomainDefSetMemoryTotal(persistentDef, newmem); if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(cfg->configDir, persistentDef); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 71be9c7..a9f0005 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -734,7 +734,7 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - virDomainDefSetMemoryInitial(persistentDef, newmem); + virDomainDefSetMemoryTotal(persistentDef, newmem); if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 6faa701..2f95597 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -770,7 +770,7 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties) if (lxcConvertSize(value->str, &size) < 0) return -1; size = size / 1024; - virDomainDefSetMemoryInitial(def, size); + virDomainDefSetMemoryTotal(def, size); def->mem.hard_limit = virMemoryLimitTruncate(size); } @@ -1010,7 +1010,7 @@ lxcParseConfigString(const char *config) } vmdef->id = -1; - virDomainDefSetMemoryInitial(vmdef, 64 * 1024); + virDomainDefSetMemoryTotal(vmdef, 64 * 1024); vmdef->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; vmdef->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 54dec70..2912fc4 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3281,7 +3281,7 @@ phypDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) goto err; } - virDomainDefSetMemoryInitial(&def, memory); + virDomainDefSetMemoryTotal(&def, memory); if ((def.mem.cur_balloon = phypGetLparMem(dom->conn, managed_system, dom->id, 1)) == 0) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e1f199c..3044b11 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -13010,7 +13010,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, def->id = -1; def->mem.cur_balloon = 64 * 1024; - virDomainDefSetMemoryInitial(def, def->mem.cur_balloon); + virDomainDefSetMemoryTotal(def, def->mem.cur_balloon); def->maxvcpus = 1; def->vcpus = 1; def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; @@ -13216,7 +13216,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, _("cannot parse memory level '%s'"), val); goto error; } - virDomainDefSetMemoryInitial(def, mem * 1024); + virDomainDefSetMemoryTotal(def, mem * 1024); def->mem.cur_balloon = mem * 1024; } else if (STREQ(arg, "-smp")) { WANT_VALUE(); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a5e9b75..8f9b40c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3393,7 +3393,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) /* align initial memory size */ mem = virDomainDefGetMemoryInitial(def); - virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, align)); + virDomainDefSetMemoryTotal(def, VIR_ROUND_UP(mem, align)); def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3c959f6..2387cf3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2357,7 +2357,7 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, goto endjob; } - virDomainDefSetMemoryInitial(persistentDef, newmem); + virDomainDefSetMemoryTotal(persistentDef, newmem); if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 213a9a1..18e8c95 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2256,7 +2256,7 @@ static int testDomainSetMaxMemory(virDomainPtr domain, return -1; /* XXX validate not over host memory wrt to other domains */ - virDomainDefSetMemoryInitial(privdom->def, memory); + virDomainDefSetMemoryTotal(privdom->def, memory); virDomainObjEndAPI(&privdom); return 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index c3c5fa7..2b61f73 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1828,7 +1828,7 @@ static int umlDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) goto cleanup; } - virDomainDefSetMemoryInitial(vm->def, newmax); + virDomainDefSetMemoryTotal(vm->def, newmax); ret = 0; cleanup: diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 91a61f8..3e6ed7a 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3898,7 +3898,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) * reading and while dumping xml */ /* def->mem.max_balloon = maxMemorySize * 1024; */ - virDomainDefSetMemoryInitial(def, memorySize * 1024); + virDomainDefSetMemoryTotal(def, memorySize * 1024); gVBoxAPI.UIMachine.GetCPUCount(machine, &CPUCount); def->maxvcpus = def->vcpus = CPUCount; @@ -6051,7 +6051,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, * the notation here seems to be inconsistent while * reading and while dumping xml */ - virDomainDefSetMemoryInitial(def->dom, memorySize * 1024); + virDomainDefSetMemoryTotal(def->dom, memorySize * 1024); def->dom->os.type = VIR_DOMAIN_OSTYPE_HVM; def->dom->os.arch = virArchFromHost(); gVBoxAPI.UIMachine.GetCPUCount(machine, &CPUCount); diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index c6d97f8..7c3c10a 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1416,7 +1416,7 @@ virVMXParseConfig(virVMXContext *ctx, goto cleanup; } - virDomainDefSetMemoryInitial(def, memsize * 1024); /* Scale from megabytes to kilobytes */ + virDomainDefSetMemoryTotal(def, memsize * 1024); /* Scale from megabytes to kilobytes */ /* vmx:sched.mem.max -> def:mem.cur_balloon */ if (virVMXGetConfigLong(conf, "sched.mem.max", &sched_mem_max, memsize, diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 58c77ab..7a2afd6 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1299,7 +1299,7 @@ prlsdkLoadDomain(vzConnPtr privconn, /* get RAM parameters */ pret = PrlVmCfg_GetRamSize(sdkdom, &ram); prlsdkCheckRetGoto(pret, error); - virDomainDefSetMemoryInitial(def, ram << 10); /* RAM size obtained in Mbytes, + virDomainDefSetMemoryTotal(def, ram << 10); /* RAM size obtained in Mbytes, convert to Kbytes */ def->mem.cur_balloon = ram << 10; diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 07b9eb4..75f98b1 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -605,7 +605,7 @@ xenXMDomainSetMaxMemory(virConnectPtr conn, if (entry->def->mem.cur_balloon > memory) entry->def->mem.cur_balloon = memory; - virDomainDefSetMemoryInitial(entry->def, memory); + virDomainDefSetMemoryTotal(entry->def, memory); /* If this fails, should we try to undo our changes to the * in-memory representation of the config file. I say not! */ diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 11f6e91..3045c5a 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1492,7 +1492,7 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) VIR_FREE(val); } memory = xenapiDomainGetMaxMemory(dom); - virDomainDefSetMemoryInitial(defPtr, memory); + virDomainDefSetMemoryTotal(defPtr, memory); if (xen_vm_get_memory_dynamic_max(session, &dynamic_mem, vm)) { defPtr->mem.cur_balloon = (unsigned long) (dynamic_mem / 1024); } else { diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 0dfe60e..0890c73 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -316,7 +316,7 @@ xenParseMem(virConfPtr conf, virDomainDefPtr def) return -1; def->mem.cur_balloon *= 1024; - virDomainDefSetMemoryInitial(def, memory * 1024); + virDomainDefSetMemoryTotal(def, memory * 1024); return 0; } diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 1d43ec1..033b0eb 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -1154,7 +1154,7 @@ xenParseSxpr(const struct sexpr *root, } } - virDomainDefSetMemoryInitial(def, (sexpr_u64(root, "domain/maxmem") << 10)); + virDomainDefSetMemoryTotal(def, (sexpr_u64(root, "domain/maxmem") << 10)); def->mem.cur_balloon = (sexpr_u64(root, "domain/memory") << 10); if (def->mem.cur_balloon > virDomainDefGetMemoryActual(def)) -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
The name of the variable was misleading. Rename it and it's setting accessor before other fixes. --- src/conf/domain_conf.c | 18 +++++++++--------- src/conf/domain_conf.h | 7 ++++--- src/hyperv/hyperv_driver.c | 2 +- src/libvirt_private.syms | 2 +- src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_native.c | 4 ++-- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/test/test_driver.c | 2 +- src/uml/uml_driver.c | 2 +- src/vbox/vbox_common.c | 4 ++-- src/vmx/vmx.c | 2 +- src/vz/vz_sdk.c | 2 +- src/xen/xm_internal.c | 2 +- src/xenapi/xenapi_driver.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- 20 files changed, 35 insertions(+), 34 deletions(-)
ACK Michal
Add 'initial_memory' member to struct virDomainMemtune so that the memory size can be pre-calculated once instead of inferring it always again and again. Separating of the fields will also allow finer granularity of decisions in later patches where it will allow to keep the old initial memory value in cases where we are handling incomming migration from older versions that did not always update the size from NUMA as the code did previously. The change also requires modification of the qemu memory alignment function since at the point where we are modifying the size of NUMA nodes the total size needs to be recalculated too. The refactoring done in this patch also fixes a crash in the hyperv driver that did not properly initialize def->numa and thus virDomainNumaGetMemorySize(def->numa) crashed. In summary this patch should have no functional impact at this point. --- src/conf/domain_conf.c | 52 +++++++++++++++++++++++++++++------------------- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 15 +++++++++----- 4 files changed, 46 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a20ff2c..d2a13ca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3728,6 +3728,15 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr def) static int virDomainDefPostParseMemory(virDomainDefPtr def) { + size_t i; + + if ((def->mem.initial_memory = virDomainNumaGetMemorySize(def->numa)) == 0) { + def->mem.initial_memory = def->mem.total_memory; + + for (i = 0; i < def->nmems; i++) + def->mem.initial_memory -= def->mems[i]->size; + } + if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the " @@ -7699,19 +7708,7 @@ virDomainDefHasMemoryHotplug(const virDomainDef *def) unsigned long long virDomainDefGetMemoryInitial(virDomainDefPtr def) { - unsigned long long ret; - size_t i; - - /* return NUMA memory size total in case numa is enabled */ - if ((ret = virDomainNumaGetMemorySize(def->numa)) > 0) { - return ret; - } else { - ret = def->mem.total_memory; - for (i = 0; i < def->nmems; i++) - ret -= def->mems[i]->size; - } - - return def->mem.total_memory; + return def->mem.initial_memory; } @@ -7720,13 +7717,30 @@ virDomainDefGetMemoryInitial(virDomainDefPtr def) * @def: domain definition * @size: size to set * - * Sets the total memory size in @def. + * Sets the total memory size in @def. This function should be used only by + * hypervisors that don't support memory hotplug. */ 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; } @@ -7744,12 +7758,10 @@ virDomainDefGetMemoryActual(virDomainDefPtr def) unsigned long long ret; size_t i; - if ((ret = virDomainNumaGetMemorySize(def->numa)) > 0) { - for (i = 0; i < def->nmems; i++) - ret += def->mems[i]->size; - } else { - ret = def->mem.total_memory; - } + ret = def->mem.initial_memory; + + for (i = 0; i < def->nmems; i++) + ret += def->mems[i]->size; return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 74c29bd..ab250bd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2145,6 +2145,8 @@ 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 */ @@ -2324,6 +2326,7 @@ struct _virDomainDef { unsigned long long virDomainDefGetMemoryInitial(virDomainDefPtr 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 d31687d..c87efa1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -227,6 +227,7 @@ virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; virDomainDefPostParse; +virDomainDefSetMemoryInitial; virDomainDefSetMemoryTotal; virDomainDeleteConfig; virDomainDeviceAddressIsValid; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8f9b40c..f92e1d3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3380,6 +3380,7 @@ qemuDomainGetMemorySizeAlignment(virDomainDefPtr def ATTRIBUTE_UNUSED) int qemuDomainAlignMemorySizes(virDomainDefPtr def) { + unsigned long long initialmem = 0; unsigned long long mem; unsigned long long align = qemuDomainGetMemorySizeAlignment(def); size_t ncells = virDomainNumaGetNodeCount(def->numa); @@ -3387,13 +3388,17 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) /* align NUMA cell sizes if relevant */ for (i = 0; i < ncells; i++) { - mem = virDomainNumaGetNodeMemorySize(def->numa, i); - virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(mem, align)); + mem = VIR_ROUND_UP(virDomainNumaGetNodeMemorySize(def->numa, i), align); + initialmem += mem; + virDomainNumaSetNodeMemorySize(def->numa, i, mem); } - /* align initial memory size */ - mem = virDomainDefGetMemoryInitial(def); - virDomainDefSetMemoryTotal(def, VIR_ROUND_UP(mem, align)); + /* align initial memory size, if NUMA is present calculate it as total of + * individual aligned NUMA node sizes */ + if (initialmem == 0) + initialmem = VIR_ROUND_UP(virDomainDefGetMemoryInitial(def), align); + + virDomainDefSetMemoryInitial(def, initialmem); def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align); -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
Add 'initial_memory' member to struct virDomainMemtune so that the memory size can be pre-calculated once instead of inferring it always again and again.
Separating of the fields will also allow finer granularity of decisions in later patches where it will allow to keep the old initial memory value in cases where we are handling incomming migration from older versions that did not always update the size from NUMA as the code did previously.
The change also requires modification of the qemu memory alignment function since at the point where we are modifying the size of NUMA nodes the total size needs to be recalculated too.
The refactoring done in this patch also fixes a crash in the hyperv driver that did not properly initialize def->numa and thus virDomainNumaGetMemorySize(def->numa) crashed.
In summary this patch should have no functional impact at this point. --- src/conf/domain_conf.c | 52 +++++++++++++++++++++++++++++------------------- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 15 +++++++++----- 4 files changed, 46 insertions(+), 25 deletions(-)
ACK Michal
When implementing memory hotplug I've opted to recalculate the initial memory size (contents of the <memory> element) as a sum of the sizes of NUMA nodes when NUMA was enabled. This was based on an assumption that qemu did not allow starting when the NUMA node size total didn't equal to the initial memory size. Unfortunately the check was introduced to qemu just lately. This patch uses the new XML parser flag to decide whether it's safe to update the memory size total from the NUMA cell sizes or not. As an additional improvement we now report an error in case when the size of hotplug memory would exceed the total memory size. The rest of the changes assures that the function is called with correct flags. --- src/conf/domain_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 3 ++- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d2a13ca..64cfd8c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3726,15 +3726,36 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr def) static int -virDomainDefPostParseMemory(virDomainDefPtr def) +virDomainDefPostParseMemory(virDomainDefPtr def, + unsigned int parseFlags) { size_t i; + unsigned long long numaMemory = 0; + unsigned long long hotplugMemory = 0; - if ((def->mem.initial_memory = virDomainNumaGetMemorySize(def->numa)) == 0) { + /* Attempt to infer the initial memory size from the sum NUMA memory sizes + * in case ABI updates are allowed or the <memory> element wasn't specified */ + if (def->mem.total_memory == 0 || + parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) + numaMemory = virDomainNumaGetMemorySize(def->numa); + + if (numaMemory) { + def->mem.initial_memory = numaMemory; + } else { def->mem.initial_memory = def->mem.total_memory; + /* calculate the sizes of hotplug memory */ for (i = 0; i < def->nmems; i++) - def->mem.initial_memory -= def->mems[i]->size; + hotplugMemory += def->mems[i]->size; + + if (hotplugMemory > def->mem.initial_memory) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Total size of memory devices exceeds the total " + "memory size")); + return -1; + } + + def->mem.initial_memory -= hotplugMemory; } if (virDomainDefGetMemoryInitial(def) == 0) { @@ -3770,7 +3791,8 @@ virDomainDefPostParseMemory(virDomainDefPtr def) static int virDomainDefPostParseInternal(virDomainDefPtr def, - virCapsPtr caps ATTRIBUTE_UNUSED) + virCapsPtr caps ATTRIBUTE_UNUSED, + unsigned int parseFlags) { size_t i; @@ -3781,7 +3803,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def, return -1; } - if (virDomainDefPostParseMemory(def) < 0) + if (virDomainDefPostParseMemory(def, parseFlags) < 0) return -1; /* @@ -4274,6 +4296,7 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, + unsigned int parseFlags, virDomainXMLOptionPtr xmlopt) { int ret; @@ -4299,7 +4322,7 @@ virDomainDefPostParse(virDomainDefPtr def, return ret; - if ((ret = virDomainDefPostParseInternal(def, caps)) < 0) + if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0) return ret; return 0; @@ -16426,7 +16449,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; /* callback to fill driver specific domain aspects */ - if (virDomainDefPostParse(def, caps, xmlopt) < 0) + if (virDomainDefPostParse(def, caps, flags, xmlopt) < 0) goto error; /* Auto-add any implied controllers which aren't present */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ab250bd..25914b4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2459,6 +2459,7 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt) int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, + unsigned int parseFlags, virDomainXMLOptionPtr xmlopt); static inline bool diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3044b11..7a1c9fe 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -13958,7 +13958,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, if (virDomainDefAddImplicitControllers(def) < 0) goto error; - if (virDomainDefPostParse(def, qemuCaps, xmlopt) < 0) + if (virDomainDefPostParse(def, qemuCaps, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + xmlopt) < 0) goto error; if (cmd->num_args || cmd->num_env) { -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
When implementing memory hotplug I've opted to recalculate the initial memory size (contents of the <memory> element) as a sum of the sizes of NUMA nodes when NUMA was enabled. This was based on an assumption that qemu did not allow starting when the NUMA node size total didn't equal to the initial memory size. Unfortunately the check was introduced to qemu just lately.
This patch uses the new XML parser flag to decide whether it's safe to update the memory size total from the NUMA cell sizes or not.
As an additional improvement we now report an error in case when the size of hotplug memory would exceed the total memory size.
The rest of the changes assures that the function is called with correct flags. --- src/conf/domain_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 3 ++- 3 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d2a13ca..64cfd8c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3726,15 +3726,36 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr def)
static int -virDomainDefPostParseMemory(virDomainDefPtr def) +virDomainDefPostParseMemory(virDomainDefPtr def, + unsigned int parseFlags) { size_t i; + unsigned long long numaMemory = 0; + unsigned long long hotplugMemory = 0;
- if ((def->mem.initial_memory = virDomainNumaGetMemorySize(def->numa)) == 0) { + /* Attempt to infer the initial memory size from the sum NUMA memory sizes + * in case ABI updates are allowed or the <memory> element wasn't specified */ + if (def->mem.total_memory == 0 || + parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) + numaMemory = virDomainNumaGetMemorySize(def->numa); + + if (numaMemory) { + def->mem.initial_memory = numaMemory;
Is there a reason to not use the setter function you've introduced in previous patch?
+ } else { def->mem.initial_memory = def->mem.total_memory;
+ /* calculate the sizes of hotplug memory */ for (i = 0; i < def->nmems; i++) - def->mem.initial_memory -= def->mems[i]->size; + hotplugMemory += def->mems[i]->size; + + if (hotplugMemory > def->mem.initial_memory) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Total size of memory devices exceeds the total " + "memory size")); + return -1; + } + + def->mem.initial_memory -= hotplugMemory; }
if (virDomainDefGetMemoryInitial(def) == 0) { @@ -3770,7 +3791,8 @@ virDomainDefPostParseMemory(virDomainDefPtr def)
static int virDomainDefPostParseInternal(virDomainDefPtr def, - virCapsPtr caps ATTRIBUTE_UNUSED) + virCapsPtr caps ATTRIBUTE_UNUSED, + unsigned int parseFlags) { size_t i;
@@ -3781,7 +3803,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def, return -1; }
- if (virDomainDefPostParseMemory(def) < 0) + if (virDomainDefPostParseMemory(def, parseFlags) < 0) return -1;
/* @@ -4274,6 +4296,7 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, + unsigned int parseFlags, virDomainXMLOptionPtr xmlopt) { int ret; @@ -4299,7 +4322,7 @@ virDomainDefPostParse(virDomainDefPtr def, return ret;
- if ((ret = virDomainDefPostParseInternal(def, caps)) < 0) + if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0) return ret;
return 0; @@ -16426,7 +16449,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error;
/* callback to fill driver specific domain aspects */ - if (virDomainDefPostParse(def, caps, xmlopt) < 0) + if (virDomainDefPostParse(def, caps, flags, xmlopt) < 0) goto error;
/* Auto-add any implied controllers which aren't present */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ab250bd..25914b4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2459,6 +2459,7 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt) int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, + unsigned int parseFlags, virDomainXMLOptionPtr xmlopt);
static inline bool diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3044b11..7a1c9fe 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -13958,7 +13958,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, if (virDomainDefAddImplicitControllers(def) < 0) goto error;
- if (virDomainDefPostParse(def, qemuCaps, xmlopt) < 0) + if (virDomainDefPostParse(def, qemuCaps, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + xmlopt) < 0) goto error;
if (cmd->num_args || cmd->num_env) {
ACK Michal
On Tue, Sep 22, 2015 at 14:29:02 +0200, Michal Privoznik wrote:
On 21.09.2015 19:21, Peter Krempa wrote:
When implementing memory hotplug I've opted to recalculate the initial memory size (contents of the <memory> element) as a sum of the sizes of NUMA nodes when NUMA was enabled. This was based on an assumption that qemu did not allow starting when the NUMA node size total didn't equal to the initial memory size. Unfortunately the check was introduced to qemu just lately.
This patch uses the new XML parser flag to decide whether it's safe to update the memory size total from the NUMA cell sizes or not.
As an additional improvement we now report an error in case when the size of hotplug memory would exceed the total memory size.
The rest of the changes assures that the function is called with correct flags. --- src/conf/domain_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 3 ++- 3 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d2a13ca..64cfd8c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3726,15 +3726,36 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr def)
static int -virDomainDefPostParseMemory(virDomainDefPtr def) +virDomainDefPostParseMemory(virDomainDefPtr def, + unsigned int parseFlags) { size_t i; + unsigned long long numaMemory = 0; + unsigned long long hotplugMemory = 0;
- if ((def->mem.initial_memory = virDomainNumaGetMemorySize(def->numa)) == 0) { + /* Attempt to infer the initial memory size from the sum NUMA memory sizes + * in case ABI updates are allowed or the <memory> element wasn't specified */ + if (def->mem.total_memory == 0 || + parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) + numaMemory = virDomainNumaGetMemorySize(def->numa); + + if (numaMemory) { + def->mem.initial_memory = numaMemory;
Is there a reason to not use the setter function you've introduced in previous patch?
Not really, it even saves a few lines of code. Peter
When we are starting a qemu process for an incomming migration or snapshot reloading we should not modify the memory sizes in the domain since we could potentially change the guest ABI that was tediously checked before. Additionally the function now updates the initial memory size according to the NUMA node size, which should not happen if we are restoring state. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1252685 --- src/qemu/qemu_command.c | 3 ++- tests/qemuxml2argvdata/qemuxml2argv-restore-v1.args | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7a1c9fe..bb1835c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9318,7 +9318,8 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildDomainLoaderCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (qemuDomainAlignMemorySizes(def) < 0) + if (!migrateFrom && !snapshot && + qemuDomainAlignMemorySizes(def) < 0) goto error; virCommandAddArg(cmd, "-m"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-restore-v1.args b/tests/qemuxml2argvdata/qemuxml2argv-restore-v1.args index 5c67702..458c015 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-restore-v1.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-restore-v1.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +pc -m 213 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel \ none -incoming stdio -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
When we are starting a qemu process for an incomming migration or snapshot reloading we should not modify the memory sizes in the domain since we could potentially change the guest ABI that was tediously checked before. Additionally the function now updates the initial memory size according to the NUMA node size, which should not happen if we are restoring state.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1252685 --- src/qemu/qemu_command.c | 3 ++- tests/qemuxml2argvdata/qemuxml2argv-restore-v1.args | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)
ACK Michal
For some machine types ppc64 machines now require that memory sizes are aligned to 256MiB increments (due to the dynamically reconfigurable memory). As now we treat existing configs reasonably in regards to migration, we can round all the sizes unconditionally. The only drawback will be that the memory size of a VM can potentially increase by (256MiB - 1byte) * number_of_NUMA_nodes. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1249006 --- CC: David Gibson <dgibson@redhat.com> src/qemu/qemu_domain.c | 7 ++++++- tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat.args | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f92e1d3..2065fc4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3368,8 +3368,13 @@ qemuDomainAgentAvailable(virDomainObjPtr vm, static unsigned long long -qemuDomainGetMemorySizeAlignment(virDomainDefPtr def ATTRIBUTE_UNUSED) +qemuDomainGetMemorySizeAlignment(virDomainDefPtr def) { + /* PPC requires the memory sizes to be rounded to 256MiB increments, so + * round them to the size always. */ + if (ARCH_IS_PPC64(def->os.arch)) + return 256 * 1024; + /* Align memory size. QEMU requires rounding to next 4KiB block. * We'll take the "traditional" path and round it to 1MiB*/ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat.args index 64df406..305e924 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat.args @@ -1,7 +1,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ QEMU_AUDIO_DRV=none /usr/bin/qemu-system-ppc64 -S -M pseries \ -cpu host,compat=power7 \ --m 214 -smp 4 -nographic -nodefconfig -nodefaults \ +-m 256 -smp 4 -nographic -nodefconfig -nodefaults \ -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ -chardev pty,id=charserial0 \ -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
For some machine types ppc64 machines now require that memory sizes are aligned to 256MiB increments (due to the dynamically reconfigurable memory). As now we treat existing configs reasonably in regards to migration, we can round all the sizes unconditionally. The only drawback will be that the memory size of a VM can potentially increase by (256MiB - 1byte) * number_of_NUMA_nodes.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1249006 --- CC: David Gibson <dgibson@redhat.com>
src/qemu/qemu_domain.c | 7 ++++++- tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat.args | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
ACK Michal
--- .../qemuxml2argv-migrate-numa-unaligned.args | 13 +++++++++ .../qemuxml2argv-migrate-numa-unaligned.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 13 +++++++-- 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args b/tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args new file mode 100644 index 0000000..4659a65 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args @@ -0,0 +1,13 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/kvm -S -M pc -m 14338 -smp 32 \ +-object memory-backend-ram,id=ram-node0,size=20482048,host-nodes=3,\ +policy=preferred \ +-numa node,nodeid=0,cpus=0,memdev=ram-node0 \ +-object memory-backend-ram,id=ram-node1,size=675907584,host-nodes=0-7,\ +policy=bind \ +-numa node,nodeid=1,cpus=1-27,cpus=29,memdev=ram-node1 \ +-object memory-backend-ram,id=ram-node2,size=24578457600,host-nodes=1-2,\ +host-nodes=5,host-nodes=7,policy=bind \ +-numa node,nodeid=2,cpus=28,cpus=30-31,memdev=ram-node2 \ +-nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -net none -serial none -parallel none -incoming stdio diff --git a/tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml b/tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml new file mode 100644 index 0000000..4fbb210 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest</name> + <uuid>9f4b6512-e73a-4a25-93e8-5307802821ce</uuid> + <memory unit='KiB'>14682468</memory> + <currentMemory unit='KiB'>14682468</currentMemory> + <vcpu placement='static'>32</vcpu> + <numatune> + <memnode cellid='0' mode='preferred' nodeset='3'/> + <memory mode='strict' nodeset='0-7'/> + <memnode cellid='2' mode='strict' nodeset='1-2,5-7,^6'/> + </numatune> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='20002' unit='KiB'/> + <cell id='1' cpus='1-27,29' memory='660066' unit='KiB'/> + <cell id='2' cpus='28,30-31' memory='24002400' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/kvm</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d4432df..2a423df 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -553,16 +553,19 @@ mymain(void) FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ __VA_ARGS__) +# define DO_TEST_LINUX(name, ...) \ + DO_TEST_LINUX_FULL(name, NULL, -1, 0, __VA_ARGS__) + # ifdef __linux__ /* This is a macro that invokes test only on Linux. It's * meant to be called in those cases where qemuxml2argvmock * cooperation is expected (e.g. we need a fixed time, * predictable NUMA topology and so on). On non-Linux * platforms the macro just consume its argument. */ -# define DO_TEST_LINUX(name, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, __VA_ARGS__) +# define DO_TEST_LINUX_FULL(name, ...) \ + DO_TEST_FULL(name, __VA_ARGS__) # else /* __linux__ */ -# define DO_TEST_LINUX(name, ...) \ +# define DO_TEST_LINUX_FULL(name, ...) \ do { \ const char *tmp ATTRIBUTE_UNUSED = name; \ } while (0) @@ -1271,6 +1274,10 @@ mymain(void) DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, QEMU_CAPS_MIGRATE_QEMU_TCP); + DO_TEST_LINUX_FULL("migrate-numa-unaligned", "stdio", 7, 0, + QEMU_CAPS_MIGRATE_KVM_STDIO, QEMU_CAPS_NUMA, + QEMU_CAPS_OBJECT_MEMORY_RAM); + DO_TEST("qemu-ns", NONE); DO_TEST("smp", QEMU_CAPS_SMP_TOPOLOGY); -- 2.4.5
On 21.09.2015 19:21, Peter Krempa wrote:
--- .../qemuxml2argv-migrate-numa-unaligned.args | 13 +++++++++ .../qemuxml2argv-migrate-numa-unaligned.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 13 +++++++-- 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml
ACK Michal
On Tue, Sep 22, 2015 at 14:28:50 +0200, Michal Privoznik wrote:
On 21.09.2015 19:21, Peter Krempa wrote:
--- .../qemuxml2argv-migrate-numa-unaligned.args | 13 +++++++++ .../qemuxml2argv-migrate-numa-unaligned.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 13 +++++++-- 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml
ACK
Thanks, I've fixed few of the problems you've pointed out and pushed the series. Peter
participants (2)
-
Michal Privoznik -
Peter Krempa