Re: [libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
On Wed, Jun 22, 2016 at 01:14:18PM -0400, Cole Robinson wrote: 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. Great, that's what I meant, we'll test-drive develop the heck out of it ;) 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 That's the problem, the address will need to be created
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
Re: [libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
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? 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. Thanks, Cole signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
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! >> >>> 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. 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. Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
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! 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. 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) I examined vioserial and pci addresses and it looks like it could be done. However, I'm not an expert on qemu_hotplug yet and this is where the interesting stuff happens with addresses, so I am not entirely sure yet. I also don't know what the plans are for device addresses in the future. Perhaps there are some features that will require caching them. I think that recalculation may change the current behavior of ccw addresses. Function virDomainCCWAddressReleaseAddr() modifies addrs->next only when the address being released was the address most recently assigned. Laine, you know a lot about PCI addresses and you also mentioned that you want to modify them in the future. What do you think? "in the future" sounds so distant :-). Within the next week I will be modifiying the PCI address allocation to eliminate the auto-adding of pci-bridge devices whenever there aren't enough free slots, and instead add a new controller of the appropriate type for each extra slot that is needed. I haven't been able to concentrate enough to determine if that is going to cause any merge conflict with what you're going to do. Martin, should I work on that instead? Kind regards, Tomasz -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
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! > >> 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 examined vioserial and pci addresses and it looks like > it could be done. However, I'm not an expert on qemu_hotplug yet > and this is where the interesting stuff happens with addresses, > so I am not entirely sure yet. > I also don't know what the plans are for device addresses in the future. > Perhaps there are some features that will require caching them. > In a way they are currently cached twice: once in these lists, and once in the runtime VM XML. That's what my vioserial series did, was replace the hotplug usage of the cached list with just fetching it on demand from the running VM XML, so at least at a high level these cached lists seem redundant because the info is tracked elsewhere. > I think that recalculation may change the current behavior of ccw addresses. > Function virDomainCCWAddressReleaseAddr() modifies addrs->next > only when the address being released was the address most recently > assigned. > Hmm, that's interesting. Not sure what that is about. I tried adding this diff: diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 794270d..7d39b69 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -792,14 +792,7 @@ virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, if (!addr) return -1; -if ((ret = virHashRemoveEntry(addrs->defined, addr)) == 0 && -dev->addr.ccw.cssid == addrs->next.cssid && -dev->addr.ccw.ssid == addrs->next.ssid && -dev->addr.ccw.devno < addrs->next.devno) { -addrs->next.devno = dev->addr.ccw.devno; -addrs->next.assigned = false; -} - +ret = virHashRemoveEntry(addrs->defined, addr); VIR_FREE(addr); return ret; And the test suite doesn't break, so at least it's not something completely obvious. But then again this seems to only be via hotunplug call path, and our test suite coverage for hotplug isn't that great. This logic may in fact just be an artifact of maintaining a persistent set of addresses. If we are generating an address set on demand from the runtime XML, we probably don't need to worry about 'releasing' an address from that set at all, so it may just side step whatever issue this logic was trying to address. > Laine, you know a lot about PCI addresses and you also mentioned that > you want to modify them in the future. What do you think? > Laine did have some patches that did touch some bits in this area, but only as a side effect. Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
> 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! > 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 examined vioserial and pci addresses and it looks like it could be done. However, I'm not an expert on qemu_hotplug yet and this is where the interesting stuff happens with addresses, so I am not entirely sure yet. I also don't know what the plans are for device addresses in the future. Perhaps there are some features that will require caching them. I think that recalculation may change the current behavior of ccw addresses. Function virDomainCCWAddressReleaseAddr() modifies addrs->next only when the address being released was the address most recently assigned. Laine, you know a lot about PCI addresses and you also mentioned that you want to modify them in the future. What do you think? Martin, should I work on that instead? Kind regards, Tomasz -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
On 06/19/2016 08:18 PM, Tomasz Flendrich wrote: > This patch depends on another one, because it was created on top > of the changes in the other one. Link to the dependency: > https://www.redhat.com/archives/libvir-list/2016-June/msg01227.html > > This is the next patch of the ongoing process of abstracting device > address generation. > > In this patch, addres sets are moved from QEMU's (ccw and pci) > and bhyve's (pci only) private data to domainDef. In the future, > it's likely that more hypervisors will add the feature of explicitly > specifying the device addresses, so handling the addresses should be > made more generic. > 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 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 That said I think it makes sense in general to move this generic stuff out of qemu since it's generic code as you say, but better to remove the obsolete stuff first IMO Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
This patch depends on another one, because it was created on top of the changes in the other one. Link to the dependency: https://www.redhat.com/archives/libvir-list/2016-June/msg01227.html This is the next patch of the ongoing process of abstracting device address generation. In this patch, addres sets are moved from QEMU's (ccw and pci) and bhyve's (pci only) private data to domainDef. In the future, it's likely that more hypervisors will add the feature of explicitly specifying the device addresses, so handling the addresses should be made more generic. Tomasz Flendrich (4): Move functions from qemu_domain_address to domain_addr Move ccwaddrs from QEMU's private data to domainDef Move virDomainPCIAddressSetFree from domain_addr to domain_conf Move pciaddrs from private data to domainDef src/bhyve/bhyve_device.c | 16 +- src/bhyve/bhyve_domain.c | 2 - src/bhyve/bhyve_domain.h | 3 - src/conf/domain_addr.c | 497 ++-- src/conf/domain_addr.h | 67 ++--- src/conf/domain_conf.c | 21 ++ src/conf/domain_conf.h | 51 src/libvirt_private.syms | 11 +- src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_domain.h | 3 - src/qemu/qemu_domain_address.c | 561 +++-- src/qemu/qemu_domain_address.h | 3 +- src/qemu/qemu_hotplug.c| 23 +- src/qemu/qemu_process.c| 6 +- tests/qemuhotplugtest.c| 2 +- 15 files changed, 629 insertions(+), 641 deletions(-) -- 2.7.4 (Apple Git-66) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list