[libvirt] [PATCH 0/3] RFC: qemu: drop vioserialaddr cache
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
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
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
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
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).
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
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
participants (3)
-
Cole Robinson -
Ján Tomko -
Laine Stump