1:25 p.m.
qemuDomainObjPrivate caches three lists of device addresses:
virDomainPCIAddressSetPtr pciaddrs;
virDomainCCWAddressSetPtr ccwaddrs;
virDomainVirtioSerialAddrSetPtr vioserialaddrs;
Yet I can't quite tell what issue they fix... they are only used
at hotplug time for checking for address collisions, however it
appears that we can generate those lists on demand from the runtime
XML, which contains all the info we need.
In truth I only looked deeply at the vioserialaddrs list... perhaps
PCI has more to it. But at least for virtio serial it looks like
this caching can be dropped. CCing jtomko who originally added it
If this is acceptable, dropping all the caching will be a step
towards unifying all uses of qemuDomainAssignAddresses, rather
than sprinkling around a dozen call sites throughout the code.
Also (again if this is acceptable), there's more cleanup to do
here, dropping some now unused code in domain_addr
Cole Robinson (3):
domain: Add virDomainVirtioSerialAddrSetCreateFromDomain
qemu: hotplug: generate vioserial address list on demand
qemu: remove vioserialaddr cache
src/conf/domain_addr.c | 35 ++++++++++++++++++++++++++++++++---
src/conf/domain_addr.h | 7 ++-----
src/libvirt_private.syms | 3 +--
src/qemu/qemu_domain.c | 1 -
src/qemu/qemu_domain.h | 1 -
src/qemu/qemu_domain_address.c | 28 +++-------------------------
src/qemu/qemu_hotplug.c | 37 +++++++++++++++++++++++++------------
src/qemu/qemu_process.c | 2 --
8 files changed, 63 insertions(+), 51 deletions(-)
--
2.7.4
1:25 p.m.
New subject: [libvirt] [PATCH 1/3] domain: Add virDomainVirtioSerialAddrSetCreateFromDomain
This handles all the bits needed for generating a
virDomainVirtioSerialAddrSetPtr by inspecting a virDomainDefPtr
---
src/conf/domain_addr.c | 35 ++++++++++++++++++++++++++++++++---
src/conf/domain_addr.h | 7 ++-----
src/libvirt_private.syms | 3 +--
src/qemu/qemu_domain_address.c | 9 +--------
4 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index acd8ce6..01a3ae1 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -845,7 +845,7 @@ virDomainCCWAddressSetCreate(void)
*
* Allocates an address set for virtio serial addresses
*/
-virDomainVirtioSerialAddrSetPtr
+static virDomainVirtioSerialAddrSetPtr
virDomainVirtioSerialAddrSetCreate(void)
{
virDomainVirtioSerialAddrSetPtr ret = NULL;
@@ -947,7 +947,7 @@
virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs,
* Adds virtio serial ports of controllers present in the domain definition
* to the address set.
*/
-int
+static int
virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs,
virDomainDefPtr def)
{
@@ -962,7 +962,6 @@
virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs
return 0;
}
-
void
virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
{
@@ -1085,6 +1084,36 @@
virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSetPtr addr
return 0;
}
+/* virDomainVirtioSerialAddrSetCreateFromDomain
+ *
+ * @def: Domain def to introspect
+ *
+ * Inspect the domain definition and return an address set containing
+ * every virtio serial address we find
+ */
+virDomainVirtioSerialAddrSetPtr
+virDomainVirtioSerialAddrSetCreateFromDomain(virDomainDefPtr def)
+{
+ virDomainVirtioSerialAddrSetPtr addrs;
+ virDomainVirtioSerialAddrSetPtr ret = NULL;
+
+ if (!(addrs = virDomainVirtioSerialAddrSetCreate()))
+ goto cleanup;
+
+ if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0)
+ goto cleanup;
+
+ if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve,
+ addrs) < 0)
+ goto cleanup;
+
+ ret = addrs;
+ cleanup:
+ if (!ret)
+ virDomainVirtioSerialAddrSetFree(addrs);
+ return ret;
+}
+
/* virDomainVirtioSerialAddrAutoAssign
*
* reserve a virtio serial address of the device (if it has one)
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index f3eda89..c29f85a 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -201,11 +201,8 @@ typedef struct _virDomainVirtioSerialAddrSet
virDomainVirtioSerialAddrSet;
typedef virDomainVirtioSerialAddrSet *virDomainVirtioSerialAddrSetPtr;
virDomainVirtioSerialAddrSetPtr
-virDomainVirtioSerialAddrSetCreate(void);
-int
-virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs,
- virDomainDefPtr def)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+virDomainVirtioSerialAddrSetCreateFromDomain(virDomainDefPtr def)
+ ATTRIBUTE_NONNULL(1);
void
virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs);
bool
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fb24808..5530e05 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -113,8 +113,7 @@ virDomainVirtioSerialAddrAutoAssign;
virDomainVirtioSerialAddrIsComplete;
virDomainVirtioSerialAddrRelease;
virDomainVirtioSerialAddrReserve;
-virDomainVirtioSerialAddrSetAddControllers;
-virDomainVirtioSerialAddrSetCreate;
+virDomainVirtioSerialAddrSetCreateFromDomain;
virDomainVirtioSerialAddrSetFree;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 9c8c262..e17b34b 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -115,14 +115,7 @@ qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def,
virDomainVirtioSerialAddrSetPtr addrs = NULL;
qemuDomainObjPrivatePtr priv = NULL;
- if (!(addrs = virDomainVirtioSerialAddrSetCreate()))
- goto cleanup;
-
- if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0)
- goto cleanup;
-
- if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve,
- addrs) < 0)
+ if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def)))
goto cleanup;
VIR_DEBUG("Finished reserving existing ports");
--
2.7.4
1:25 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: hotplug: generate vioserial address list on demand
The hotplug address allocation depends on the vioserialaddrs
cache being populated elsewhere. However the cache isn't needed
and we can just generate the address list on demand from the
VM's definition
---
src/qemu/qemu_hotplug.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f40b34d..28a49a0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1486,38 +1486,51 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
}
static int
-qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv,
+qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
+ qemuDomainObjPrivatePtr priv,
virDomainChrDefPtr chr)
{
+ int ret = -1;
+ virDomainVirtioSerialAddrSetPtr vioaddrs = NULL;
+
+ if (!(vioaddrs = virDomainVirtioSerialAddrSetCreateFromDomain(def)))
+ goto cleanup;
+
if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
- if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs,
+ if (virDomainVirtioSerialAddrAutoAssign(def, vioaddrs,
&chr->info, true) < 0)
- return -1;
- return 1;
+ goto cleanup;
+ ret = 1;
} else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0)
- return -1;
- return 1;
+ goto cleanup;
+ ret = 1;
} else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) {
- if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs,
+ if (virDomainVirtioSerialAddrAutoAssign(def, vioaddrs,
&chr->info, false) < 0)
- return -1;
- return 1;
+ goto cleanup;
+ ret = 1;
}
+ if (ret == 1)
+ goto cleanup;
+
if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL ||
chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Unsupported address type for character device"));
- return -1;
+ goto cleanup;
}
- return 0;
+ ret = 0;
+ cleanup:
+ virDomainVirtioSerialAddrSetFree(vioaddrs);
+ return ret;
}
int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
@@ -1540,7 +1553,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0)
goto cleanup;
- if ((rc = qemuDomainAttachChrDeviceAssignAddr(priv, chr)) < 0)
+ if ((rc = qemuDomainAttachChrDeviceAssignAddr(vm->def, priv, chr)) < 0)
goto cleanup;
if (rc == 1)
need_release = true;
--
2.7.4
1:25 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: remove vioserialaddr cache
There's no users left, so drop it
---
src/qemu/qemu_domain.c | 1 -
src/qemu/qemu_domain.h | 1 -
src/qemu/qemu_domain_address.c | 19 ++-----------------
src/qemu/qemu_process.c | 2 --
4 files changed, 2 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b0eb3b6..e972956 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1174,7 +1174,6 @@ qemuDomainObjPrivateFree(void *data)
virCgroupFree(&priv->cgroup);
virDomainPCIAddressSetFree(priv->pciaddrs);
virDomainCCWAddressSetFree(priv->ccwaddrs);
- virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs);
virDomainChrSourceDefFree(priv->monConfig);
qemuDomainObjFreeJob(priv);
VIR_FREE(priv->vcpupids);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index dd90e67..7c5ae54 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -189,7 +189,6 @@ struct _qemuDomainObjPrivate {
virDomainPCIAddressSetPtr pciaddrs;
virDomainCCWAddressSetPtr ccwaddrs;
- virDomainVirtioSerialAddrSetPtr vioserialaddrs;
int persistentAddrs;
virQEMUCapsPtr qemuCaps;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e17b34b..ff5b4f4 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -107,13 +107,11 @@ qemuDomainSetSCSIControllerModel(const virDomainDef *def,
static int
-qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def,
- virDomainObjPtr obj)
+qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def)
{
int ret = -1;
size_t i;
virDomainVirtioSerialAddrSetPtr addrs = NULL;
- qemuDomainObjPrivatePtr priv = NULL;
if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def)))
goto cleanup;
@@ -138,16 +136,7 @@ qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def,
goto cleanup;
}
- if (obj && obj->privateData) {
- priv = obj->privateData;
- /* if this is the live domain object, we persist the addresses */
- virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs);
- priv->persistentAddrs = 1;
- priv->vioserialaddrs = addrs;
- addrs = NULL;
- }
ret = 0;
-
cleanup:
virDomainVirtioSerialAddrSetFree(addrs);
return ret;
@@ -1639,7 +1628,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virDomainObjPtr obj)
{
- if (qemuDomainAssignVirtioSerialAddresses(def, obj) < 0)
+ if (qemuDomainAssignVirtioSerialAddresses(def) < 0)
return -1;
if (qemuDomainAssignSpaprVIOAddresses(def, qemuCaps) < 0)
@@ -1679,8 +1668,4 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
&info->addr.pci) < 0)
VIR_WARN("Unable to release PCI address on %s",
NULLSTR(devstr));
- if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
- virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, info) < 0)
- VIR_WARN("Unable to release virtio-serial address on %s",
- NULLSTR(devstr));
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index eddf3a7..b6a71c4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6095,8 +6095,6 @@ void qemuProcessStop(virQEMUDriverPtr driver,
virDomainDefClearCCWAddresses(vm->def);
virDomainCCWAddressSetFree(priv->ccwaddrs);
priv->ccwaddrs = NULL;
- virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs);
- priv->vioserialaddrs = NULL;
}
qemuHostdevReAttachDomainDevices(driver, vm->def);
--
2.7.4
9:05 a.m.
On 05/14/2016 02:25 PM, Cole Robinson wrote:
qemuDomainObjPrivate caches three lists of device addresses:
virDomainPCIAddressSetPtr pciaddrs;
virDomainCCWAddressSetPtr ccwaddrs;
virDomainVirtioSerialAddrSetPtr vioserialaddrs;
Yet I can't quite tell what issue they fix... they are only used
at hotplug time for checking for address collisions, however it
appears that we can generate those lists on demand from the runtime
XML, which contains all the info we need.
In truth I only looked deeply at the vioserialaddrs list... perhaps
PCI has more to it. But at least for virtio serial it looks like
this caching can be dropped. CCing jtomko who originally added it
If this is acceptable, dropping all the caching will be a step
towards unifying all uses of qemuDomainAssignAddresses, rather
than sprinkling around a dozen call sites throughout the code.
FWIW, when I was doing stuff that touched address assignment, I noticed
that priv->persistentAddrs was set to 1/0 by each of the
qemuDomainAssign*Addresses() functions without regard to whether or not
it had already been set by one of the other qemuDomainAssign*Addresses()
functions. This meant that, for example, the VirtioSerial version could
set persistentAddrs = 1, and then the PCI version could reset it back to
0. Since nobody had complained and I truthfully don't know what the
usefulness of the cache is, I didn't touch it. It does look like any
domain that has no PCI addresses ends up with persistentAddrs == 0, so
probably the cache is empty anyway (? I guess. I didn't actually look at
what's behind that faux-boolean).
9:22 a.m.
On 05/16/2016 10:05 AM, Laine Stump wrote:
On 05/14/2016 02:25 PM, Cole Robinson wrote:
> qemuDomainObjPrivate caches three lists of device addresses:
>
> virDomainPCIAddressSetPtr pciaddrs;
> virDomainCCWAddressSetPtr ccwaddrs;
> virDomainVirtioSerialAddrSetPtr vioserialaddrs;
>
> Yet I can't quite tell what issue they fix... they are only used
> at hotplug time for checking for address collisions, however it
> appears that we can generate those lists on demand from the runtime
> XML, which contains all the info we need.
>
> In truth I only looked deeply at the vioserialaddrs list... perhaps
> PCI has more to it. But at least for virtio serial it looks like
> this caching can be dropped. CCing jtomko who originally added it
>
> If this is acceptable, dropping all the caching will be a step
> towards unifying all uses of qemuDomainAssignAddresses, rather
> than sprinkling around a dozen call sites throughout the code.
FWIW, when I was doing stuff that touched address assignment, I noticed that
priv->persistentAddrs was set to 1/0 by each of the
qemuDomainAssign*Addresses() functions without regard to whether or not it had
already been set by one of the other qemuDomainAssign*Addresses() functions.
This meant that, for example, the VirtioSerial version could set
persistentAddrs = 1, and then the PCI version could reset it back to 0. Since
nobody had complained and I truthfully don't know what the usefulness of the
cache is, I didn't touch it. It does look like any domain that has no PCI
addresses ends up with persistentAddrs == 0, so probably the cache is empty
anyway (? I guess. I didn't actually look at what's behind that faux-boolean).
Hmm. Yeah that is definitely wrong. And that just makes this thing even more
confusing... persistentAddrs is only used in a qemu_domain.c to trigger 1)
freeing the address caches, and 2) clearing addresses from the XML? no idea
what the latter bit is about. And the logic of using one boolean to cover both
PCI and CCW is definitely wrong here at least
I suspect when the PCI address caching was added a long time back it actually
served a purpose, but I don't think it's relevant anymore, probably due to
unconditionally assigning addresses for every qemu VM. Maybe it was
conditional at some point. CCIng danpb too, maybe he knows
- Cole
3:54 a.m.
On Mon, May 16, 2016 at 10:22:23AM -0400, Cole Robinson wrote:
On 05/16/2016 10:05 AM, Laine Stump wrote:
> On 05/14/2016 02:25 PM, Cole Robinson wrote:
>> qemuDomainObjPrivate caches three lists of device addresses:
>>
>> virDomainPCIAddressSetPtr pciaddrs;
>> virDomainCCWAddressSetPtr ccwaddrs;
>> virDomainVirtioSerialAddrSetPtr vioserialaddrs;
>>
>> Yet I can't quite tell what issue they fix... they are only used
>> at hotplug time for checking for address collisions, however it
>> appears that we can generate those lists on demand from the runtime
>> XML, which contains all the info we need.
>>
>> In truth I only looked deeply at the vioserialaddrs list... perhaps
>> PCI has more to it. But at least for virtio serial it looks like
>> this caching can be dropped. CCing jtomko who originally added it
>>
Yes, this is just a cache. I assume dropping it won't have any
performance impact, even for the PCI code, but I did not measure it.
Also, for vioserialaddrs the persistentAddrs bool is slightly more useless,
it just tracks if vioserialaddrs is non-NULL. The cargo cult made me do
it.
>> If this is acceptable, dropping all the caching will be a
step
>> towards unifying all uses of qemuDomainAssignAddresses, rather
>> than sprinkling around a dozen call sites throughout the code.
>
> FWIW, when I was doing stuff that touched address assignment, I noticed that
> priv->persistentAddrs was set to 1/0 by each of the
> qemuDomainAssign*Addresses() functions without regard to whether or not it had
> already been set by one of the other qemuDomainAssign*Addresses() functions.
> This meant that, for example, the VirtioSerial version could set
> persistentAddrs = 1, and then the PCI version could reset it back to 0. Since
> nobody had complained and I truthfully don't know what the usefulness of the
> cache is, I didn't touch it. It does look like any domain that has no PCI
> addresses ends up with persistentAddrs == 0, so probably the cache is empty
> anyway (? I guess. I didn't actually look at what's behind that
faux-boolean).
>
Hmm. Yeah that is definitely wrong. And that just makes this thing even more
confusing... persistentAddrs is only used in a qemu_domain.c to trigger 1)
freeing the address caches, and 2) clearing addresses from the XML? no idea
what the latter bit is about. And the logic of using one boolean to cover both
PCI and CCW is definitely wrong here at least
I suspect when the PCI address caching was added a long time back it actually
served a purpose, but I don't think it's relevant anymore, probably due to
unconditionally assigning addresses for every qemu VM. Maybe it was
conditional at some point. CCIng danpb too, maybe he knows
I did some digital archaeology:
The variable was added by:
commit 141dea6bc7222107c2357acb68066baea5b26df3
CommitDate: 2010-02-12 17:25:52 +0000
Add persistence of PCI addresses to QEMU
Where it was set to 0 on domain startup if qemu did not support the
QEMUD_CMD_FLAG_DEVICE capability, to clear the addresses at shutdown,
because QEMU might make up different ones next time.
As of commit f5dd58a6088cfc6e8bd354b693d399807a8ec395
CommitDate: 2012-07-11 11:19:05 +0200
qemu: Extended qemuDomainAssignAddresses to be callable from
everywhere.
this was broken, when the persistentAddrs = 0 assignment was moved
inside qemuDomainAssignPCIAddresses and while it pretends to check
for !QEMU_CAPS_DEVICE, its parent qemuDomainAssignAddresses is only
called if QEMU_CAPS_DEVICE is present.
Jan
3146
days inactive
3152
days old
6 comments
3 participants
participants (3)
-
Cole Robinson -
Ján Tomko -
Laine Stump