[libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef

[Cole Robinson]

On 06/22/2016 01:04 PM, Martin Kletzander wrote:
> On Wed, Jun 22, 2016 at 07:24:41AM -0400, Cole Robinson wrote:
>> On 06/21/2016 10:08 PM, Laine Stump wrote:
>>> On 06/20/2016 10:26 PM, Tomasz Flendrich wrote:
>>>>> Apologies if I'm missing something, I didn't look too closely at this
>>>>> series,
>>>>> however have you seen this thread?
>>>>>
>>>>> http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html
>>>> I haven’t noticed that some work has been done on that, thank you!
>>>>
> 
> Me neither, I'm kind of busier nowadays (it's 7pm and I'm yet again
> sitting at work even though I'm now temporarily working part-time).
> Sorry for that, we discussed Tomasz's work before that and I must've
> missed it after.
> 
>>>>> My understanding of the current code is that the cached
>>>>> vioserial/ccw/pciaddrs
>>>>> lists in qemu aren't actually required…they were at one point to handle
>>>>> older qemu, but we dropped that support. Maybe you can pick up my patches
>>>>> and
>>>>> finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs
>>>>> cache in
>>>>> bhyve can be dropped as well, I don't think it was ever strictly
>>>>> required, the
>>>>> code just followed the qemu example
>>>> If we could do without the caching, it would make the current code simpler.
>>>> There wouldn’t be those booleans in qemu_hotplug.c that remember whether
>>>> an address has to be deleted or not in case something fails. We could
>>>> delete qemuDomainReleaseDeviceAddress() and a few more functions.
>>>
>>> I'm completely ignorant about vioserial and ccw. As far as the "cache" for pci
>>> addresses, I guess whether or not we want the cache depends completely on how
>>> long it takes to reconstruct vs. how often a device is added.
>>
>> Constructing the cache likely takes less time than parsing a single XML
>> document... it's just iterating over the parsed XML in memory and performing
>> the collision checks.
>>
> 
> I'm 100% for dropping any unnecessary data storage, especially if it is
> used wrong anyway.  @Cole: it looks like there was a discussion on your
> series but nobody actually reviewed it.  Looking at it it's pretty
> trivial but there are no tests.  So I propose that Tomasz could pick up
> your work with the addition of tests that would make sure the hotplug
> behaviour is kept (or even fixed if there is a problem) and he would
> continue with other addresses afterwards.  Is that OK with you or do you
> have some deeper plans with this area of code?
> 

No deeper plans and I wasn't expecting to get back to it any time soon. The
original posting was mostly to start a discussion about whether the caching
served any purpose that I was missing, so there was consensus that it was safe
for removal before anyone took it any further.

Adding the hotplug tests is definitely a good idea too. I suggest getting some
hotplug tests for the current vioserial state, then make sure my patches don't
cause any regressions, then add a patch on top that removes all the now unused
vioserial addresses I alluded to in that series cover letter. I'm happy to
help with review and IRC discussion too if needed.

>> There is also
>>> the issue of the mismatch between live and config devices' address use, and
>>> the inconsistency that can be caused by that (if you hotplug a device with
>>> --live, then hotplug another with --live --config, then the 2nd device will
>>> have the same address in config as the first has in the live state of the
>>> guest (more importantly, the address of the 2nd device will change the next
>>> time the domain is shutdown and restarted, which all of this address
>>> assignment stuff is intended to avoid) - I don't know if that problem would be
>>> more easily solved by a cache that is used for assigning addresses for both
>>> --live and --config, or if, as Cole suggests, it would be better just to
>>> remove the cache and rebuild the allocation table each time a new device is
>>> added (this would mean that we would need to scan through all the live devices
>>> *and* persistent devices to re-populate it)
>>>
>>
>> For 'config' updates we don't even use the address cache presently, at least
>> for vioserialaddr, I didn't confirm for the other ones. It's only used for
>> hotplug. For 'config' updates we basically just insert the device into the XML
>> and effectively 'redefine' it, and the generic XML parsing machinery and
>> domain callbacks assign addresses.
>>
>> The address list is only needed for runtime hotplug because we need to
>> generate an address on demand, for passing to the qemu monitor commands.
>>
> 
> We'll take that issue into account and will work towards fixing that as
> a part of our work.  It looks like the only harder thing will be making
> sure the code is not total mess when assigning the addresses, especially
> since it needs to work with two different definitions together.
> 

Most of that stuff is already in place in the current code, there's completely
separate code paths for config vs online hotplug. If you look at my patch #2
from that series the hotplug changes are pretty minimal actually

http://www.redhat.com/archives/libvir-list/2016-May/msg01073.html

There may still be unexpected hurdles that I missed, but I'm guessing not. The
hardest part may in fact be writing the hotplug test cases :)

- Cole