[libvirt] [PATCH 0/4] qemu: fix vcpupin/emulatorpin with --config on VMs with automatic NUMA pinning
1:39 a.m.
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
1:39 a.m.
New subject: [libvirt] [PATCH 1/4] conf: Introduce virDomainObjGetOneDefState
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
1:39 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: domain: Add macro to simplify access to vm private data
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
1:39 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: driver: Don't return automatic NUMA vCPU pinning data for persistentDef
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
1:39 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: driver: Don't return automatic NUMA emulator pinning data for persistentDef
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
7:17 a.m.
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.
8:54 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:42 a.m.
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
9:50 a.m.
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
10:04 a.m.
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.
3018
days inactive
3025
days old
9 comments
3 participants
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa