[libvirt] [PATCH] Refresh QEMU driver caps in getCapabilities
Hi all, The attached patch fixes QEMU getCapabilities to refresh caps before returning them to the user. Currently the capabilities are only refreshed once (at daemon startup), which means libvirtd needs to be restarted to pick up changes if QEMU or KVM are installed while the daemon is running. See: https://bugzilla.redhat.com/show_bug.cgi?id=460649 There are several things 'wrong' with this change: - We reset/rescan fields that won't change (host arch, mac address prefix). This should be fixed at some point, but isn't a big deal since total performance impact is negligible (see below). - We only refresh the capabilities when the user calls getCapabilities, which means we are still carrying around stale caps prior to that, which is what libvirt validates against. In practice, virt-manager and virt-install both call getCapabilities often so this isn't an issue, though the caps internal API should probably address this at some point. To test the performance impact, I used a simple python script: import libvirt conn = libvirt.open("qemu:///system") for i in range(0, 30): conn.getCapabilities() The time difference was on average .02 seconds slower, which I think is negligible. If at somepoint in the future, capabilities generation becomes smarter (searching PATH emulators, scraping device list output, etc.) it might be worth re-checking the time impact. But for now it doesn't seem to be an issue. Thanks, Cole
Cole Robinson schrieb:
(...)
To test the performance impact, I used a simple python script:
import libvirt conn = libvirt.open("qemu:///system") for i in range(0, 30): conn.getCapabilities()
The time difference was on average .02 seconds slower, which I think is negligible.
If at somepoint in the future, capabilities generation becomes smarter (searching PATH emulators, scraping device list output, etc.) it might be worth re-checking the time impact. But for now it doesn't seem to be an issue.
Thanks, Cole
------------------------------------------------------------------------
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
.02 seconds slower compared to what time? Without a given base this result doesn't say much. It wouldn't matter if the execution-time was 3600 seconds prior to the change but would be a realt bottleneck if it was 0.01 seconds prior to the change. I know a virsh capabilities is not that slow, but i haven't clocked it up to now but have done so now that i have read your post and got this on my machine: rr016# time virsh capabilities (...) real 0m0.005s user 0m0.001s sys 0m0.002s So the change would mean a 300% penalty. I could live with that because even with .02 seconds it is fast. Gerrit
On 04/28/2009 05:13 PM, Gerrit Slomma wrote:
Cole Robinson schrieb:
(...)
To test the performance impact, I used a simple python script:
import libvirt conn = libvirt.open("qemu:///system") for i in range(0, 30): conn.getCapabilities()
The time difference was on average .02 seconds slower, which I think is negligible.
If at somepoint in the future, capabilities generation becomes smarter (searching PATH emulators, scraping device list output, etc.) it might be worth re-checking the time impact. But for now it doesn't seem to be an issue.
Thanks, Cole
------------------------------------------------------------------------
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
.02 seconds slower compared to what time? Without a given base this result doesn't say much. It wouldn't matter if the execution-time was 3600 seconds prior to the change but would be a realt bottleneck if it was 0.01 seconds prior to the change.
The results were around .4 seconds real time. I didn't think that info was particularly relevant in this case though, since the python script is fetching the capabilities 30 times. Rereading my original email again, I was pretty unclear that the .02 time difference was for calling capabilities all 30 times, so the real impact would only be .02/30 = .00067 seconds per capabilities call, which is negligible no matter what the % slow down.
I know a virsh capabilities is not that slow, but i haven't clocked it up to now but have done so now that i have read your post and got this on my machine:
rr016# time virsh capabilities (...) real 0m0.005s user 0m0.001s sys 0m0.002s
Just calling virsh capabilities isn't the best metric, since there will be overhead in driver opening, possibly authentication, actually printing the result, etc. Not that my method is perfect either, but incurs less overhead than virsh. Thanks, Cole
On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote:
Hi all,
The attached patch fixes QEMU getCapabilities to refresh caps before returning them to the user. Currently the capabilities are only refreshed once (at daemon startup), which means libvirtd needs to be restarted to pick up changes if QEMU or KVM are installed while the daemon is running. See: https://bugzilla.redhat.com/show_bug.cgi?id=460649
There are several things 'wrong' with this change:
- We reset/rescan fields that won't change (host arch, mac address prefix). This should be fixed at some point, but isn't a big deal since total performance impact is negligible (see below).
- We only refresh the capabilities when the user calls getCapabilities, which means we are still carrying around stale caps prior to that, which is what libvirt validates against. In practice, virt-manager and virt-install both call getCapabilities often so this isn't an issue, though the caps internal API should probably address this at some point.
To test the performance impact, I used a simple python script:
import libvirt conn = libvirt.open("qemu:///system") for i in range(0, 30): conn.getCapabilities()
The time difference was on average .02 seconds slower, which I think is negligible.
More importantly I don't see this API call as being a performance critical one. It is not something applicaitions will be calling inside a hot loop.
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 79ee072..6b5c17f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn,
static char *qemudGetCapabilities(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; - char *xml; + char *xml = NULL;
qemuDriverLock(driver); - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) + virCapabilitiesFree(qemu_driver->caps); + if ((qemu_driver->caps = qemudCapsInit()) == NULL || + (xml = virCapabilitiesFormatXML(driver->caps)) == NULL) virReportOOMError(conn); qemuDriverUnlock(driver);
The thing to be wary of now, is that all use of driver->caps needs to be protected by the driver mutex. Most usages are OK, but I spotted a couple that are not. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote:
Hi all,
The attached patch fixes QEMU getCapabilities to refresh caps before returning them to the user. Currently the capabilities are only refreshed once (at daemon startup), which means libvirtd needs to be restarted to pick up changes if QEMU or KVM are installed while the daemon is running. See: https://bugzilla.redhat.com/show_bug.cgi?id=460649
There are several things 'wrong' with this change:
- We reset/rescan fields that won't change (host arch, mac address prefix). This should be fixed at some point, but isn't a big deal since total performance impact is negligible (see below).
- We only refresh the capabilities when the user calls getCapabilities, which means we are still carrying around stale caps prior to that, which is what libvirt validates against. In practice, virt-manager and virt-install both call getCapabilities often so this isn't an issue, though the caps internal API should probably address this at some point.
To test the performance impact, I used a simple python script:
import libvirt conn = libvirt.open("qemu:///system") for i in range(0, 30): conn.getCapabilities()
The time difference was on average .02 seconds slower, which I think is negligible.
More importantly I don't see this API call as being a performance critical one. It is not something applicaitions will be calling inside a hot loop.
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 79ee072..6b5c17f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn,
static char *qemudGetCapabilities(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; - char *xml; + char *xml = NULL;
qemuDriverLock(driver); - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) + virCapabilitiesFree(qemu_driver->caps); + if ((qemu_driver->caps = qemudCapsInit()) == NULL || + (xml = virCapabilitiesFormatXML(driver->caps)) == NULL) virReportOOMError(conn); qemuDriverUnlock(driver);
The thing to be wary of now, is that all use of driver->caps needs to be protected by the driver mutex. Most usages are OK, but I spotted a couple that are not.
Daniel
Ahh, yes I didn't think of that repercussion. I'll update this patch. Thanks, Cole
Daniel P. Berrange wrote:
On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 79ee072..6b5c17f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn,
static char *qemudGetCapabilities(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; - char *xml; + char *xml = NULL;
qemuDriverLock(driver); - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) + virCapabilitiesFree(qemu_driver->caps); + if ((qemu_driver->caps = qemudCapsInit()) == NULL || + (xml = virCapabilitiesFormatXML(driver->caps)) == NULL) virReportOOMError(conn); qemuDriverUnlock(driver);
The thing to be wary of now, is that all use of driver->caps needs to be protected by the driver mutex. Most usages are OK, but I spotted a couple that are not.
Daniel
Okay, updated patch attached. The only unsafe caps usage I found was in qemudNodeGetSecurityModel, not sure if you spotted any others. Thanks, Cole
On Mon, May 04, 2009 at 03:08:20PM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 79ee072..6b5c17f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn,
static char *qemudGetCapabilities(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; - char *xml; + char *xml = NULL;
qemuDriverLock(driver); - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) + virCapabilitiesFree(qemu_driver->caps); + if ((qemu_driver->caps = qemudCapsInit()) == NULL || + (xml = virCapabilitiesFormatXML(driver->caps)) == NULL) virReportOOMError(conn); qemuDriverUnlock(driver);
The thing to be wary of now, is that all use of driver->caps needs to be protected by the driver mutex. Most usages are OK, but I spotted a couple that are not.
Daniel
Okay, updated patch attached. The only unsafe caps usage I found was in qemudNodeGetSecurityModel, not sure if you spotted any others.
That patch looks fine to me, any reason not to apply it ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 05/06/2009 09:14 AM, Daniel Veillard wrote:
On Mon, May 04, 2009 at 03:08:20PM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 79ee072..6b5c17f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn,
static char *qemudGetCapabilities(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; - char *xml; + char *xml = NULL;
qemuDriverLock(driver); - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) + virCapabilitiesFree(qemu_driver->caps); + if ((qemu_driver->caps = qemudCapsInit()) == NULL || + (xml = virCapabilitiesFormatXML(driver->caps)) == NULL) virReportOOMError(conn); qemuDriverUnlock(driver); The thing to be wary of now, is that all use of driver->caps needs to be protected by the driver mutex. Most usages are OK, but I spotted a couple that are not.
Daniel Okay, updated patch attached. The only unsafe caps usage I found was in qemudNodeGetSecurityModel, not sure if you spotted any others.
That patch looks fine to me, any reason not to apply it ?
Daniel
Nope! I've pushed it now. Thanks, Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Gerrit Slomma