[libvirt] [PATCH 0/3] RFC: qemu: drop vioserialaddr cache

[Ján Tomko]

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