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

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