[libvirt] [PATCH] qemuDomainSendKey: Relax the qemu driver locking
Currently, there is no reason to hold qemu driver locked throughout whole API execution. Moreover, we can use the new qemuDomObjFromDomain() internal API to lookup domain then. --- src/qemu/qemu_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6d4c1e9..100f10b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2407,19 +2407,12 @@ static int qemuDomainSendKey(virDomainPtr domain, } } - qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, domain->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(domain->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = qemuDomObjFromDomain(domain))) goto cleanup; - } priv = vm->privateData; - if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; if (!virDomainObjIsActive(vm)) { @@ -2428,9 +2421,9 @@ static int qemuDomainSendKey(virDomainPtr domain, goto endjob; } - qemuDomainObjEnterMonitorWithDriver(driver, vm); + qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes); - qemuDomainObjExitMonitorWithDriver(driver, vm); + qemuDomainObjExitMonitor(driver, vm); endjob: if (qemuDomainObjEndJob(driver, vm) == 0) @@ -2439,7 +2432,6 @@ endjob: cleanup: if (vm) virObjectUnlock(vm); - qemuDriverUnlock(driver); return ret; } -- 1.8.0.2
On 01/24/2013 10:41 AM, Michal Privoznik wrote:
Currently, there is no reason to hold qemu driver locked throughout whole API execution. Moreover, we can use the new qemuDomObjFromDomain() internal API to lookup domain then. --- src/qemu/qemu_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6d4c1e9..100f10b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2407,19 +2407,12 @@ static int qemuDomainSendKey(virDomainPtr domain, } }
- qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, domain->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(domain->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = qemuDomObjFromDomain(domain))) goto cleanup; - }
priv = vm->privateData;
- if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup;
if (!virDomainObjIsActive(vm)) { @@ -2428,9 +2421,9 @@ static int qemuDomainSendKey(virDomainPtr domain, goto endjob; }
- qemuDomainObjEnterMonitorWithDriver(driver, vm); + qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes); - qemuDomainObjExitMonitorWithDriver(driver, vm); + qemuDomainObjExitMonitor(driver, vm);
endjob: if (qemuDomainObjEndJob(driver, vm) == 0) @@ -2439,7 +2432,6 @@ endjob: cleanup: if (vm) virObjectUnlock(vm); - qemuDriverUnlock(driver); return ret; }
Formally this looks good and works on my system, so I would be fine with it. I am just wondering what the criteria are for holding a long-term lock vs a short-time on. I.e. why would DomainSendKey be relaxed while DomainInjectNMI keeps the driver lock? -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 01/24/13 11:13, Viktor Mihajlovski wrote:
On 01/24/2013 10:41 AM, Michal Privoznik wrote:
Currently, there is no reason to hold qemu driver locked throughout whole API execution. Moreover, we can use the new qemuDomObjFromDomain() internal API to lookup domain then. --- src/qemu/qemu_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
Formally this looks good and works on my system, so I would be fine with it. I am just wondering what the criteria are for holding a long-term lock vs a short-time on. I.e. why would DomainSendKey be relaxed while DomainInjectNMI keeps the driver lock?
That's due to historic reasons. The usual approach was to hold the driver lock all the time. We're trying to get rid of that. qemuDomainInjectNMI can be simplified too as it doesn't mess with the driver state. Peter
On 24.01.2013 11:13, Viktor Mihajlovski wrote:
On 01/24/2013 10:41 AM, Michal Privoznik wrote:
Currently, there is no reason to hold qemu driver locked throughout whole API execution. Moreover, we can use the new qemuDomObjFromDomain() internal API to lookup domain then. --- src/qemu/qemu_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6d4c1e9..100f10b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2407,19 +2407,12 @@ static int qemuDomainSendKey(virDomainPtr domain, } }
- qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, domain->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(domain->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = qemuDomObjFromDomain(domain))) goto cleanup; - }
priv = vm->privateData;
- if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup;
if (!virDomainObjIsActive(vm)) { @@ -2428,9 +2421,9 @@ static int qemuDomainSendKey(virDomainPtr domain, goto endjob; }
- qemuDomainObjEnterMonitorWithDriver(driver, vm); + qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes); - qemuDomainObjExitMonitorWithDriver(driver, vm); + qemuDomainObjExitMonitor(driver, vm);
endjob: if (qemuDomainObjEndJob(driver, vm) == 0) @@ -2439,7 +2432,6 @@ endjob: cleanup: if (vm) virObjectUnlock(vm); - qemuDriverUnlock(driver); return ret; }
Formally this looks good and works on my system, so I would be fine with it. I am just wondering what the criteria are for holding a long-term lock vs a short-time on. I.e. why would DomainSendKey be relaxed while DomainInjectNMI keeps the driver lock?
Yes, that's another candidate. Basically for now, everything that uses qemu driver other than: - domain lookup - begin/end job - enter/exit monitor SHOULD be using long term lock. This, however, will soon be past - I believe I saw an e-mail from Dan saying, qemu driver locking should be turned into R/W locks. Michal
On 01/24/2013 11:45 AM, Michal Privoznik wrote:
Yes, that's another candidate. Basically for now, everything that uses qemu driver other than: - domain lookup - begin/end job - enter/exit monitor SHOULD be using long term lock. This, however, will soon be past - I believe I saw an e-mail from Dan saying, qemu driver locking should be turned into R/W locks.
Michal
Sounds reasonable. Knowing that this is *some* work, wouldn't it make sense to relax the locking for all eligible qemu_driver functions in one patch? This would then be a tangible feature of 1.0.2. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Jan 24, 2013 at 01:17:35PM +0100, Viktor Mihajlovski wrote:
On 01/24/2013 11:45 AM, Michal Privoznik wrote:
Yes, that's another candidate. Basically for now, everything that uses qemu driver other than: - domain lookup - begin/end job - enter/exit monitor SHOULD be using long term lock. This, however, will soon be past - I believe I saw an e-mail from Dan saying, qemu driver locking should be turned into R/W locks.
Michal
Sounds reasonable. Knowing that this is *some* work, wouldn't it make sense to relax the locking for all eligible qemu_driver functions in one patch? This would then be a tangible feature of 1.0.2.
I'm killing the big qemu driver lock entirely in the release after this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/24/2013 02:41 AM, Michal Privoznik wrote:
Currently, there is no reason to hold qemu driver locked throughout whole API execution. Moreover, we can use the new qemuDomObjFromDomain() internal API to lookup domain then. --- src/qemu/qemu_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 24.01.2013 19:05, Eric Blake wrote:
On 01/24/2013 02:41 AM, Michal Privoznik wrote:
Currently, there is no reason to hold qemu driver locked throughout whole API execution. Moreover, we can use the new qemuDomObjFromDomain() internal API to lookup domain then. --- src/qemu/qemu_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
ACK.
Thanks, pushed. Michal
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Peter Krempa -
Viktor Mihajlovski