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