[libvirt] [PATCH 0/3] qemu: fix automatic NUMA pinning with memory hotplug
We forgot to propagate the automatic nodeset and reported non-ideal errors. Peter Krempa (3): qemu: command: Pass numad nodeset when formatting memory devices at boot qemu: command: Split up formatting of -numa and memory devices qemu: hotplug: Improve error when hotpluging memory with auto-placement src/conf/numa_conf.c | 20 +++++++++++++++++--- src/conf/numa_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 27 ++++++++++++++------------- src/qemu/qemu_hotplug.c | 7 +++++++ 5 files changed, 40 insertions(+), 16 deletions(-) -- 2.7.3
When starting up a VM libvirtd asks numad to place the VM in case of automatic nodeset. The nodeset would not be passed to the memory device formatter and the user would get an error. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1269715 --- src/qemu/qemu_command.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 45c5398..8545533 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2945,7 +2945,8 @@ static char * qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, - virQEMUDriverConfigPtr cfg) + virQEMUDriverConfigPtr cfg, + virBitmapPtr auto_nodeset) { virJSONValuePtr props = NULL; char *alias = NULL; @@ -2962,7 +2963,7 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, goto cleanup; if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, - mem->targetNode, mem->sourceNodes, NULL, + mem->targetNode, mem->sourceNodes, auto_nodeset, def, qemuCaps, cfg, &backendType, &props, true) < 0) goto cleanup; @@ -7200,7 +7201,7 @@ qemuBuildNumaCommandLine(virCommandPtr cmd, char *dimmStr; if (!(backStr = qemuBuildMemoryDimmBackendStr(def->mems[i], def, - qemuCaps, cfg))) + qemuCaps, cfg, nodeset))) return -1; if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i]))) { -- 2.7.3
They recently were extracted to a separate function. They don't belong together though. Since -numa formatting is pretty compact, move it to the main function and rename qemuBuildNumaCommandLine to qemuBuildMemoryDeviceCommandLine. --- src/qemu/qemu_command.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8545533..2d0ca97 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7182,18 +7182,14 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, static int -qemuBuildNumaCommandLine(virCommandPtr cmd, - virQEMUDriverConfigPtr cfg, - virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virBitmapPtr nodeset) +qemuBuildMemoryDeviceCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virBitmapPtr nodeset) { size_t i; - if (virDomainNumaGetNodeCount(def->numa) && - qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) - return -1; - /* memory hotplug requires NUMA to be enabled - we already checked * that memory devices are present only when NUMA is */ for (i = 0; i < def->nmems; i++) { @@ -9260,7 +9256,11 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildIOThreadCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (qemuBuildNumaCommandLine(cmd, cfg, def, qemuCaps, nodeset) < 0) + if (virDomainNumaGetNodeCount(def->numa) && + qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) + goto error; + + if (qemuBuildMemoryDeviceCommandLine(cmd, cfg, def, qemuCaps, nodeset) < 0) goto error; virUUIDFormat(def->uuid, uuid); -- 2.7.3
The numad advice is not valid at the point of hotplug so we are not reusing it. If the user would want to hot-add memory with automatic placement we'd report: 'Advice from numad is needed in case of automatic numa placement' Change it to: 'automatic placement of memory is not possible when hotplugging' --- src/conf/numa_conf.c | 20 +++++++++++++++++--- src/conf/numa_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 7 +++++++ 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index e0d5688..e10fed8 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -419,6 +419,22 @@ virDomainNumatuneFormatNodeset(virDomainNumaPtr numatune, } +/** + * virDomainNumatuneNeedNumadAdvice: + * @numatune: domain numa definition + * + * Returns true if the numa configuration requires automatic placement data + * gathered by numad. + */ +bool +virDomainNumatuneNeedNumadAdvice(virDomainNumaPtr numatune) +{ + return numatune && + numatune->memory.specified && + numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO; +} + + int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, @@ -434,9 +450,7 @@ virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, !numatune->memory.specified) return 0; - if (numatune->memory.specified && - numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO && - !auto_nodeset) { + if (virDomainNumatuneNeedNumadAdvice(numatune) && !auto_nodeset) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Advice from numad is needed in case of " "automatic numa placement")); diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 90deacb..82f0155 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -155,5 +155,6 @@ int virDomainNumaDefCPUFormat(virBufferPtr buf, virDomainNumaPtr def); unsigned int virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa); +bool virDomainNumatuneNeedNumadAdvice(virDomainNumaPtr numatune); #endif /* __NUMA_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7c44047..8b88667 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -692,6 +692,7 @@ virDomainNumatuneMaybeFormatNodeset; virDomainNumatuneMaybeGetNodeset; virDomainNumatuneMemModeTypeFromString; virDomainNumatuneMemModeTypeToString; +virDomainNumatuneNeedNumadAdvice; virDomainNumatuneNodesetIsAvailable; virDomainNumatuneNodeSpecified; virDomainNumatuneParseXML; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e82dbf5..84475b7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1760,6 +1760,13 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (vm->def->mem.cur_balloon == virDomainDefGetMemoryActual(vm->def)) fix_balloon = true; + if (virDomainNumatuneNeedNumadAdvice(vm->def->numa) && !mem->sourceNodes) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("automatic placement of memory is not possible when " + "hotplugging")); + goto cleanup; + } + if (!(devstr = qemuBuildMemoryDeviceStr(mem))) goto cleanup; -- 2.7.3
On 03/29/2016 10:52 AM, Peter Krempa wrote:
We forgot to propagate the automatic nodeset and reported non-ideal errors.
Peter Krempa (3): qemu: command: Pass numad nodeset when formatting memory devices at boot qemu: command: Split up formatting of -numa and memory devices qemu: hotplug: Improve error when hotpluging memory with auto-placement
src/conf/numa_conf.c | 20 +++++++++++++++++--- src/conf/numa_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 27 ++++++++++++++------------- src/qemu/qemu_hotplug.c | 7 +++++++ 5 files changed, 40 insertions(+), 16 deletions(-)
ACK series (seems safe to me), John
On Wed, Mar 30, 2016 at 07:26:09 -0400, John Ferlan wrote:
On 03/29/2016 10:52 AM, Peter Krempa wrote:
We forgot to propagate the automatic nodeset and reported non-ideal errors.
Peter Krempa (3): qemu: command: Pass numad nodeset when formatting memory devices at boot qemu: command: Split up formatting of -numa and memory devices qemu: hotplug: Improve error when hotpluging memory with auto-placement
src/conf/numa_conf.c | 20 +++++++++++++++++--- src/conf/numa_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 27 ++++++++++++++------------- src/qemu/qemu_hotplug.c | 7 +++++++ 5 files changed, 40 insertions(+), 16 deletions(-)
ACK series (seems safe to me),
I'll shortly push patches 1 and 2 since 1 is a pretty straightforward bug fix and 2 is a no-op. In patch 3 I've noticed that the function I've created would be duplicate with virDomainNumatuneHasPlacementAuto so I'll repost it later. Thanks. Peter
participants (2)
-
John Ferlan -
Peter Krempa