[libvirt] [PATCH 0/4] qemu: fix vcpupin/emulatorpin with --config on VMs with automatic NUMA pinning
The automatic nodeset would creep into the persistent definition for live VMs with automatic pinning which is incorrect. https://bugzilla.redhat.com/show_bug.cgi?id=1365779 Peter Krempa (4): conf: Introduce virDomainObjGetOneDefState qemu: domain: Add macro to simplify access to vm private data qemu: driver: Don't return automatic NUMA vCPU pinning data for persistentDef qemu: driver: Don't return automatic NUMA emulator pinning data for persistentDef src/conf/domain_conf.c | 40 ++++++++++++++++++++++++++++++++++++---- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 23 +++++++++++++---------- 5 files changed, 56 insertions(+), 14 deletions(-) -- 2.10.0
Return whether the live or persistent definition was returned. Sometimes it's necessary to base the decisions on this. --- src/conf/domain_conf.c | 40 ++++++++++++++++++++++++++++++++++++---- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0828041..8f48a74 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3118,22 +3118,25 @@ virDomainObjGetDefs(virDomainObjPtr vm, /** - * virDomainObjGetOneDef: + * virDomainObjGetOneDefState: * * @vm: Domain object * @flags: for virDomainModificationImpact + * @live: set to true if live config was returned (may be omitted) * * Helper function to resolve @flags and return the correct domain pointer * object. This function returns one of @vm->def or @vm->persistentDef - * according to @flags. This helper should be used only in APIs that guarantee + * according to @flags. @live is set to true if the live vm config will be + * returned. This helper should be used only in APIs that guarantee * that @flags contains exactly one of VIR_DOMAIN_AFFECT_LIVE or * VIR_DOMAIN_AFFECT_CONFIG and not both. * * Returns the correct definition pointer or NULL on error. */ virDomainDefPtr -virDomainObjGetOneDef(virDomainObjPtr vm, - unsigned int flags) +virDomainObjGetOneDefState(virDomainObjPtr vm, + unsigned int flags, + bool *live) { if (flags & VIR_DOMAIN_AFFECT_LIVE && flags & VIR_DOMAIN_AFFECT_CONFIG) { virReportInvalidArg(ctl, "%s", @@ -3146,6 +3149,13 @@ virDomainObjGetOneDef(virDomainObjPtr vm, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) return NULL; + if (live) { + if (flags & VIR_DOMAIN_AFFECT_LIVE) + *live = true; + else + *live = false; + } + if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; else @@ -3153,6 +3163,28 @@ virDomainObjGetOneDef(virDomainObjPtr vm, } +/** + * virDomainObjGetOneDef: + * + * @vm: Domain object + * @flags: for virDomainModificationImpact + * + * Helper function to resolve @flags and return the correct domain pointer + * object. This function returns one of @vm->def or @vm->persistentDef + * according to @flags. This helper should be used only in APIs that guarantee + * that @flags contains exactly one of VIR_DOMAIN_AFFECT_LIVE or + * VIR_DOMAIN_AFFECT_CONFIG and not both. + * + * Returns the correct definition pointer or NULL on error. + */ +virDomainDefPtr +virDomainObjGetOneDef(virDomainObjPtr vm, + unsigned int flags) +{ + return virDomainObjGetOneDefState(vm, flags, NULL); +} + + static int virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c14a39c..ac99b15 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2592,6 +2592,9 @@ int virDomainObjGetDefs(virDomainObjPtr vm, unsigned int flags, virDomainDefPtr *liveDef, virDomainDefPtr *persDef); +virDomainDefPtr virDomainObjGetOneDefState(virDomainObjPtr vm, + unsigned int flags, + bool *state); virDomainDefPtr virDomainObjGetOneDef(virDomainObjPtr vm, unsigned int flags); virDomainDefPtr virDomainDefCopy(virDomainDefPtr src, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2569772..dfaebad 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -417,6 +417,7 @@ virDomainObjFormat; virDomainObjGetDefs; virDomainObjGetMetadata; virDomainObjGetOneDef; +virDomainObjGetOneDefState; virDomainObjGetPersistentDef; virDomainObjGetShortName; virDomainObjGetState; -- 2.10.0
Sometimes adding a separate variable to access vm->privateData is not necessary. Add a macro that will do the typecasting rather than having to add a temp variable to force the compiler to typecast it. --- src/qemu/qemu_domain.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index a1404d0..e9dfbff 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -234,6 +234,9 @@ struct _qemuDomainObjPrivate { size_t masterKeyLen; }; +# define QEMU_DOMAIN_PRIVATE(vm) \ + ((qemuDomainObjPrivatePtr) (vm)->privateData) + /* Type of domain secret */ typedef enum { VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0, -- 2.10.0
Calling virDomainGetVcpuPinInfo on a live VM with automatic NUMA pinning and VIR_DOMAIN_AFFECT_CONFIG would return the automatic pinning data in some cases which is bogus. Use the autoCpuset property only when called on a live definition. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1365779 --- src/qemu/qemu_driver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dda82d3..a1cbeb0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5171,8 +5171,9 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, { virDomainObjPtr vm = NULL; virDomainDefPtr def; + bool live; int ret = -1; - qemuDomainObjPrivatePtr priv = NULL; + virBitmapPtr autoCpuset = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -5183,14 +5184,14 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (virDomainGetVcpuPinInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(def = virDomainObjGetOneDef(vm, flags))) + if (!(def = virDomainObjGetOneDefState(vm, flags, &live))) goto cleanup; - priv = vm->privateData; + if (live) + autoCpuset = QEMU_DOMAIN_PRIVATE(vm)->autoCpuset; ret = virDomainDefGetVcpuPinInfoHelper(def, maplen, ncpumaps, cpumaps, - virHostCPUGetCount(), - priv->autoCpuset); + virHostCPUGetCount(), autoCpuset); cleanup: virDomainObjEndAPI(&vm); return ret; -- 2.10.0
Calling virDomainGetEmulatorPinInfo on a live VM with automatic NUMA pinning and VIR_DOMAIN_AFFECT_CONFIG would return the automatic pinning data in some cases which is bogus. Use the autoCpuset property only when called on a live definition. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1365779 --- src/qemu/qemu_driver.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a1cbeb0..221d9f7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5317,11 +5317,12 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, { virDomainObjPtr vm = NULL; virDomainDefPtr def; + bool live; int ret = -1; int hostcpus; virBitmapPtr cpumask = NULL; virBitmapPtr bitmap = NULL; - qemuDomainObjPrivatePtr priv = NULL; + virBitmapPtr autoCpuset = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -5332,21 +5333,22 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, if (virDomainGetEmulatorPinInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(def = virDomainObjGetOneDef(vm, flags))) + if (!(def = virDomainObjGetOneDefState(vm, flags, &live))) goto cleanup; if ((hostcpus = virHostCPUGetCount()) < 0) goto cleanup; - priv = vm->privateData; + if (live) + autoCpuset = QEMU_DOMAIN_PRIVATE(vm)->autoCpuset; if (def->cputune.emulatorpin) { cpumask = def->cputune.emulatorpin; } else if (def->cpumask) { cpumask = def->cpumask; } else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && - priv->autoCpuset) { - cpumask = priv->autoCpuset; + autoCpuset) { + cpumask = autoCpuset; } else { if (!(bitmap = virBitmapNew(hostcpus))) goto cleanup; -- 2.10.0
On Wed, Sep 14, 2016 at 08:39:05 +0200, Peter Krempa wrote:
The automatic nodeset would creep into the persistent definition for live VMs with automatic pinning which is incorrect.
https://bugzilla.redhat.com/show_bug.cgi?id=1365779
Peter Krempa (4): conf: Introduce virDomainObjGetOneDefState qemu: domain: Add macro to simplify access to vm private data qemu: driver: Don't return automatic NUMA vCPU pinning data for persistentDef qemu: driver: Don't return automatic NUMA emulator pinning data for persistentDef
Ping.
On Wed, Sep 14, 2016 at 08:39:05AM +0200, Peter Krempa wrote:
The automatic nodeset would creep into the persistent definition for live VMs with automatic pinning which is incorrect.
https://bugzilla.redhat.com/show_bug.cgi?id=1365779
Peter Krempa (4): conf: Introduce virDomainObjGetOneDefState qemu: domain: Add macro to simplify access to vm private data
I must say I don't like this one ^^, it's way more readable to just use the variable which can be just optimized out, but since we use that already in multiple places, meh ACK series, I guess.
qemu: driver: Don't return automatic NUMA vCPU pinning data for persistentDef qemu: driver: Don't return automatic NUMA emulator pinning data for persistentDef
src/conf/domain_conf.c | 40 ++++++++++++++++++++++++++++++++++++---- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 23 +++++++++++++---------- 5 files changed, 56 insertions(+), 14 deletions(-)
-- 2.10.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Sep 21, 2016 at 15:54:34 +0200, Martin Kletzander wrote:
On Wed, Sep 14, 2016 at 08:39:05AM +0200, Peter Krempa wrote:
The automatic nodeset would creep into the persistent definition for live VMs with automatic pinning which is incorrect.
https://bugzilla.redhat.com/show_bug.cgi?id=1365779
Peter Krempa (4): conf: Introduce virDomainObjGetOneDefState qemu: domain: Add macro to simplify access to vm private data
I must say I don't like this one ^^, it's way more readable to just use the variable which can be just optimized out, but since we use that already in multiple places, meh ACK series, I guess.
Thanks, pushed. Peter
On 21.09.2016 15:54, Martin Kletzander wrote:
On Wed, Sep 14, 2016 at 08:39:05AM +0200, Peter Krempa wrote:
The automatic nodeset would creep into the persistent definition for live VMs with automatic pinning which is incorrect.
https://bugzilla.redhat.com/show_bug.cgi?id=1365779
Peter Krempa (4): conf: Introduce virDomainObjGetOneDefState qemu: domain: Add macro to simplify access to vm private data
I must say I don't like this one ^^, it's way more readable to just use the variable which can be just optimized out, but since we use that already in multiple places, meh ACK series, I guess.
Agreed. It makes debugging way worse and also does not work well with autocomplete in my vim. Revert it please. Michal
On Wed, Sep 21, 2016 at 04:50:52PM +0200, Michal Privoznik wrote:
On 21.09.2016 15:54, Martin Kletzander wrote:
On Wed, Sep 14, 2016 at 08:39:05AM +0200, Peter Krempa wrote:
The automatic nodeset would creep into the persistent definition for live VMs with automatic pinning which is incorrect.
https://bugzilla.redhat.com/show_bug.cgi?id=1365779
Peter Krempa (4): conf: Introduce virDomainObjGetOneDefState qemu: domain: Add macro to simplify access to vm private data
I must say I don't like this one ^^, it's way more readable to just use the variable which can be just optimized out, but since we use that already in multiple places, meh ACK series, I guess.
Agreed. It makes debugging way worse and also does not work well with autocomplete in my vim. Revert it please.
I would suggest removing all at the same time. The conversation about how some people don't like it, even though it doesn't matter, can happen in that series then.
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa