[libvirt] [RFC PATCH 2/2] qemu: use virObject to manage reference-counting for qemu monitor
Daniel P. Berrange
berrange at redhat.com
Fri Mar 18 11:20:56 UTC 2011
On Wed, Mar 16, 2011 at 06:30:20PM +0800, Hu Tao wrote:
> ---
> src/qemu/qemu_domain.c | 26 +-----------
> src/qemu/qemu_monitor.c | 106 ++++++++++++++++++++++++++--------------------
> src/qemu/qemu_monitor.h | 4 +-
> 3 files changed, 64 insertions(+), 72 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 8a2b9cc..e056ef0 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -566,7 +566,6 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
> qemuDomainObjPrivatePtr priv = obj->privateData;
>
> qemuMonitorLock(priv->mon);
> - qemuMonitorRef(priv->mon);
> virDomainObjUnlock(obj);
> }
>
> @@ -578,19 +577,9 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
> void qemuDomainObjExitMonitor(virDomainObjPtr obj)
> {
> qemuDomainObjPrivatePtr priv = obj->privateData;
> - int refs;
> -
> - refs = qemuMonitorUnref(priv->mon);
> -
> - if (refs > 0)
> - qemuMonitorUnlock(priv->mon);
>
> + qemuMonitorUnlock(priv->mon);
> virDomainObjLock(obj);
> -
> - if (refs == 0) {
> - virDomainObjUnref(obj);
> - priv->mon = NULL;
> - }
> }
>
>
> @@ -608,7 +597,6 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver,
> qemuDomainObjPrivatePtr priv = obj->privateData;
>
> qemuMonitorLock(priv->mon);
> - qemuMonitorRef(priv->mon);
> virDomainObjUnlock(obj);
> qemuDriverUnlock(driver);
> }
> @@ -623,20 +611,10 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver,
> virDomainObjPtr obj)
> {
> qemuDomainObjPrivatePtr priv = obj->privateData;
> - int refs;
> -
> - refs = qemuMonitorUnref(priv->mon);
> -
> - if (refs > 0)
> - qemuMonitorUnlock(priv->mon);
>
> + qemuMonitorUnlock(priv->mon);
> qemuDriverLock(driver);
> virDomainObjLock(obj);
> -
> - if (refs == 0) {
> - virDomainObjUnref(obj);
> - priv->mon = NULL;
> - }
> }
I don't think these changes aren't right. The EnterMonitor() method
is releasing the lock on the virDomainObjPtr. This means that other
threads (specifically the main I/O thread) can use the virDomainObjPtr
and its qemuMonitorPtr instance. Incrementing reference count on the
qemuMonitorPtr is the only thing that prevents it being free()d while
it is unlocked by this thread.
eg, consider this
In the main thread making a monitor call
qemuDomainObjEnterMonitor(obj)
qemuMonitorSetCPU(mon, 1, 1);
qemuDomainObjExitMonitor(obj)
So, at step 2 there, qemuMonitorSetCPU() is asleep on the condition
variable, waiting for the reply from QEMU.
Now QEMU dies for some reason, so in the I/O thread, we have
qemuMonitorIO() called. This sees the EOF on the socket, so
it invokes the callback "(eofNotify)(mon, vm, failed);".
This callback is qemuMonitorClose(), which calls qemuMonitorFree()
which releases the last reference count, which deletes the condition
variable. The other thread is now waiting on a condition variable
which has been deleted. It'll either hang forever, or crash
We *must* prevent the monitor being free'd while a thread is running
a command, hence qemuDomainObjEnterMonitor() must call qemuMonitorRef()
and qemuDomainObjExitMonitor() must call qemuMonitorUnref().
> -int qemuMonitorUnref(qemuMonitorPtr mon)
> +static void qemuMonitorFree(qemuMonitorPtr mon)
> {
> - mon->refs--;
> + virObjectUnref(&mon->obj);
> +}
We should just get rid fo qemuMonitorFree() entirely
and make everyone call qemuMonitorUnref(). Or vica-verca.
There's no point having both methods do the same thing.
>
> - if (mon->refs == 0) {
> - qemuMonitorUnlock(mon);
> - qemuMonitorFree(mon);
> - return 0;
> - }
> +void qemuMonitorRef(qemuMonitorPtr mon)
> +{
> + virObjectRef(&mon->obj);
> +}
>
> - return mon->refs;
> +void qemuMonitorUnref(qemuMonitorPtr mon)
> +{
> + virObjectUnref(&mon->obj);
> }
>
> static void
> @@ -238,7 +269,6 @@ qemuMonitorUnwatch(void *monitor)
> {
> qemuMonitorPtr mon = monitor;
>
> - qemuMonitorLock(mon);
> qemuMonitorUnref(mon);
> }
>
> @@ -519,7 +549,6 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) {
>
> /* lock access to the monitor and protect fd */
> qemuMonitorLock(mon);
> - qemuMonitorRef(mon);
> #if DEBUG_IO
> VIR_DEBUG("Monitor %p I/O on watch %d fd %d events %d", mon, watch, fd, events);
> #endif
> @@ -587,13 +616,11 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) {
> virDomainObjPtr vm = mon->vm;
> /* Make sure anyone waiting wakes up now */
> virCondSignal(&mon->notify);
> - if (qemuMonitorUnref(mon) > 0)
> - qemuMonitorUnlock(mon);
> + qemuMonitorUnlock(mon);
> VIR_DEBUG("Triggering EOF callback error? %d", failed);
> (eofNotify)(mon, vm, failed);
> } else {
> - if (qemuMonitorUnref(mon) > 0)
> - qemuMonitorUnlock(mon);
> + qemuMonitorUnlock(mon);
> }
> }
>
> @@ -612,30 +639,13 @@ qemuMonitorOpen(virDomainObjPtr vm,
> return NULL;
> }
>
> - if (VIR_ALLOC(mon) < 0) {
> - virReportOOMError();
> + mon = qemuMonitorAlloc();
> + if (!mon)
> return NULL;
> - }
>
> - if (virMutexInit(&mon->lock) < 0) {
> - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("cannot initialize monitor mutex"));
> - VIR_FREE(mon);
> - return NULL;
> - }
> - if (virCondInit(&mon->notify) < 0) {
> - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("cannot initialize monitor condition"));
> - virMutexDestroy(&mon->lock);
> - VIR_FREE(mon);
> - return NULL;
> - }
> - mon->fd = -1;
> - mon->refs = 1;
> mon->vm = vm;
> mon->json = json;
> mon->cb = cb;
> - qemuMonitorLock(mon);
>
> switch (config->type) {
> case VIR_DOMAIN_CHR_TYPE_UNIX:
> @@ -668,6 +678,12 @@ qemuMonitorOpen(virDomainObjPtr vm,
> }
>
>
> + /* mon will be accessed by qemuMonitorIO which is called in
> + * event thread, so ref it before passing it to the thread.
> + *
> + * Note: mon is unrefed in qemuMonitorUnwatch
> + */
> + qemuMonitorRef(mon);
> if ((mon->watch = virEventAddHandle(mon->fd,
> VIR_EVENT_HANDLE_HANGUP |
> VIR_EVENT_HANDLE_ERROR |
> @@ -678,10 +694,8 @@ qemuMonitorOpen(virDomainObjPtr vm,
> _("unable to register monitor events"));
> goto cleanup;
> }
> - qemuMonitorRef(mon);
Hmm, there are quite afew other code paths higher up in this
function which also do 'goto cleanup'. You've added an extra
qemuMonitorFree() call in 'cleanup:', but only some of the
places which jump there actually obtain an extra reference.
Is that really correct ?
>
> VIR_DEBUG("New mon %p fd =%d watch=%d", mon, mon->fd, mon->watch);
> - qemuMonitorUnlock(mon);
>
> return mon;
>
> @@ -692,8 +706,8 @@ cleanup:
> * so kill the callbacks now.
> */
> mon->cb = NULL;
> - qemuMonitorUnlock(mon);
> qemuMonitorClose(mon);
> + qemuMonitorFree(mon);
> return NULL;
> }
>
> @@ -713,8 +727,8 @@ void qemuMonitorClose(qemuMonitorPtr mon)
> VIR_FORCE_CLOSE(mon->fd);
> }
>
> - if (qemuMonitorUnref(mon) > 0)
> - qemuMonitorUnlock(mon);
> + qemuMonitorUnlock(mon);
> + qemuMonitorFree(mon);
> }
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 :|
More information about the libvir-list
mailing list