[libvirt] [RFC PATCH 0/2] virObject for reference-counting
virObject is a base struct that manages reference-counting. structs that need the ability of reference-counting can inherit from virObject and implement ref/unref interface easily. The goal of this series is to make reference-counting easy to use, and improve the current libvir reference-counting mechanism. The plan is to update all existing structs that use reference-counting to use virObject if virObject is acceptable. Patch 1 implements virObject, patch 2 is an illstration of usage of virObject. This series is in draft stage, any comments are welcome. Hu Tao (2): Add virObject. qemu: use virObject to manages reference-counting for qemu monitor src/Makefile.am | 1 + src/libvirt_private.syms | 5 ++ src/qemu/qemu_domain.c | 26 +---------- src/qemu/qemu_monitor.c | 106 ++++++++++++++++++++++++++-------------------- src/qemu/qemu_monitor.h | 4 +- src/util/object.c | 55 ++++++++++++++++++++++++ src/util/object.h | 39 +++++++++++++++++ 7 files changed, 164 insertions(+), 72 deletions(-) create mode 100644 src/util/object.c create mode 100644 src/util/object.h -- 1.7.3.1 -- Thanks, Hu Tao
virObject is the base struct that manages reference-counting for all structs that need the ability of reference-counting. --- src/Makefile.am | 1 + src/libvirt_private.syms | 5 ++++ src/util/object.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/object.h | 39 ++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 0 deletions(-) create mode 100644 src/util/object.c create mode 100644 src/util/object.h diff --git a/src/Makefile.am b/src/Makefile.am index 645119e..3ebabe9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -81,6 +81,7 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/xml.c util/xml.h \ util/virtaudit.c util/virtaudit.h \ + util/object.c util/object.h \ util/virterror.c util/virterror_internal.h EXTRA_DIST += util/threads-pthread.c util/threads-win32.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c0da78e..4f46eac 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -970,3 +970,8 @@ virXPathUInt; virXPathULong; virXPathULongHex; virXPathULongLong; + +# object.h +virObjectInit; +virObjectRef; +virObjectUnref; diff --git a/src/util/object.c b/src/util/object.c new file mode 100644 index 0000000..31ea27b --- /dev/null +++ b/src/util/object.c @@ -0,0 +1,55 @@ +/* + * object.c: base object that manages reference-counting + * + * Copyright (C) 2011 Hu Tao + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Hu Tao <hutao@cn.fujitsu.com> + */ + +#include <assert.h> + +#include "object.h" + +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj)) +{ + if (!free) + return -1; + + obj->ref = 1; + obj->free = free; + + return 0; +} + +#define virAtomicInc(value) \ + __sync_add_and_fetch(&value, 1) +#define virAtomicDec(value) \ + __sync_sub_and_fetch(&value, 1) + +void virObjectRef(virObjectPtr obj) +{ + assert(obj->ref > 0); + virAtomicInc(obj->ref); +} + +void virObjectUnref(virObjectPtr obj) +{ + assert(obj->ref > 0); + if (virAtomicDec(obj->ref) == 0) + obj->free(obj); +} diff --git a/src/util/object.h b/src/util/object.h new file mode 100644 index 0000000..f6eaea0 --- /dev/null +++ b/src/util/object.h @@ -0,0 +1,39 @@ +/* + * object.c: base object that manages reference-counting + * + * Copyright (C) 2011 Hu Tao + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Hu Tao <hutao@cn.fujitsu.com> + */ + +#ifndef __VIR_OBJECT_H +#define __VIR_OBJECT_H + +typedef struct _virObject virObject; +typedef virObject *virObjectPtr; + +struct _virObject { + int ref; + void (*free)(virObjectPtr obj); +}; + +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj)); +void virObjectRef(virObjectPtr obj); +void virObjectUnref(virObjectPtr obj); + +#endif /* __VIR_OBJECT_H */ -- 1.7.3.1 -- Thanks, Hu Tao
On Wed, Mar 16, 2011 at 06:29:52PM +0800, Hu Tao wrote:
virObject is the base struct that manages reference-counting for all structs that need the ability of reference-counting. --- src/Makefile.am | 1 + src/libvirt_private.syms | 5 ++++ src/util/object.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/object.h | 39 ++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 0 deletions(-) create mode 100644 src/util/object.c create mode 100644 src/util/object.h
diff --git a/src/Makefile.am b/src/Makefile.am index 645119e..3ebabe9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -81,6 +81,7 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/xml.c util/xml.h \ util/virtaudit.c util/virtaudit.h \ + util/object.c util/object.h \
Can you rename this to virobject.c / virobject.h. NB without the 't'. I want rename the virtaudit/virterror stuff later. Ideally all new files in util/ should have a 'vir' prefix, since our current names are so short & generic they can easily clash and / or cause confusion. eg, the name of the file should match the name of the APIs inside, so virObjectPtr -> virobject.h
diff --git a/src/util/object.c b/src/util/object.c new file mode 100644 index 0000000..31ea27b --- /dev/null +++ b/src/util/object.c +#include <assert.h>
NB, our coding guidelines don't allow use of assert(). You can use sa_assert() which is a no-op just designed to be detectable by static analysis tools.
+#include "object.h" + +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj)) +{ + if (!free) + return -1;
We should raise an error here
+ + obj->ref = 1; + obj->free = free; + + return 0; +} + +#define virAtomicInc(value) \ + __sync_add_and_fetch(&value, 1) +#define virAtomicDec(value) \ + __sync_sub_and_fetch(&value, 1)
I reckon it would be a good idea to put these into a separate file 'src/util/virtatomic.{h,c}'. Also, I think we need to make this more portable, since at least on Windows, I don't think we want to assume use of GCC. I think we should at the very least have an impl for GCC builtins, and an impl for Win32 to start with. If someone wants to do further impls later they can... If you look at glib's gatomic.c you'll see example code for Win32 atomic ops which is LGPLv2+ licensed
+void virObjectRef(virObjectPtr obj) +{ + assert(obj->ref > 0); + virAtomicInc(obj->ref); +} + +void virObjectUnref(virObjectPtr obj) +{ + assert(obj->ref > 0); + if (virAtomicDec(obj->ref) == 0) + obj->free(obj); +}
Aside from assert() not being allowed, this usage surely isn't safe because it doesn't do an atomic read on 'obj->ref' when comparing it. Regards, 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 Fri, Mar 18, 2011 at 11:02:25AM +0000, Daniel P. Berrange wrote:
On Wed, Mar 16, 2011 at 06:29:52PM +0800, Hu Tao wrote:
virObject is the base struct that manages reference-counting for all structs that need the ability of reference-counting. --- src/Makefile.am | 1 + src/libvirt_private.syms | 5 ++++ src/util/object.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/object.h | 39 ++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 0 deletions(-) create mode 100644 src/util/object.c create mode 100644 src/util/object.h
diff --git a/src/Makefile.am b/src/Makefile.am index 645119e..3ebabe9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -81,6 +81,7 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/xml.c util/xml.h \ util/virtaudit.c util/virtaudit.h \ + util/object.c util/object.h \
Can you rename this to virobject.c / virobject.h. NB without
OK.
the 't'. I want rename the virtaudit/virterror stuff later. Ideally all new files in util/ should have a 'vir' prefix, since our current names are so short & generic they can easily clash and / or cause confusion. eg, the name of the file should match the name of the APIs inside, so virObjectPtr -> virobject.h
diff --git a/src/util/object.c b/src/util/object.c new file mode 100644 index 0000000..31ea27b --- /dev/null +++ b/src/util/object.c +#include <assert.h>
NB, our coding guidelines don't allow use of assert().
You can use sa_assert() which is a no-op just designed to be detectable by static analysis tools.
OK.
+#include "object.h" + +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj)) +{ + if (!free) + return -1;
We should raise an error here
OK.
+ + obj->ref = 1; + obj->free = free; + + return 0; +} + +#define virAtomicInc(value) \ + __sync_add_and_fetch(&value, 1) +#define virAtomicDec(value) \ + __sync_sub_and_fetch(&value, 1)
I reckon it would be a good idea to put these into a separate file 'src/util/virtatomic.{h,c}'.
Agree. Will do it. Is the `virt' prefix a typo? Since you've suggested `vir' prefix for files in util/.
Also, I think we need to make this more portable, since at least on Windows, I don't think we want to assume use of GCC. I think we should at the very least have an impl for GCC builtins, and an impl for Win32 to start with. If someone wants to do further impls later they can... If you look at glib's gatomic.c you'll see example code for Win32 atomic ops which is LGPLv2+ licensed
Yes, atomic ops should be portable, not limited just on gcc. Can we borrow glib's win32 atomic ops? Will it cause any license conflict? Anyway I will have a look at glib's win32 atomic implementation. Thanks for the information.
+void virObjectRef(virObjectPtr obj) +{ + assert(obj->ref > 0); + virAtomicInc(obj->ref); +} + +void virObjectUnref(virObjectPtr obj) +{ + assert(obj->ref > 0); + if (virAtomicDec(obj->ref) == 0) + obj->free(obj); +}
Aside from assert() not being allowed, this usage surely isn't safe because it doesn't do an atomic read on 'obj->ref' when comparing it.
Right, will update it. -- Thanks, Hu Tao
On Mon, Mar 21, 2011 at 04:03:23PM +0800, Hu Tao wrote:
On Fri, Mar 18, 2011 at 11:02:25AM +0000, Daniel P. Berrange wrote:
On Wed, Mar 16, 2011 at 06:29:52PM +0800, Hu Tao wrote:
virObject is the base struct that manages reference-counting for all structs that need the ability of reference-counting. --- src/Makefile.am | 1 + src/libvirt_private.syms | 5 ++++ src/util/object.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/object.h | 39 ++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 0 deletions(-) create mode 100644 src/util/object.c create mode 100644 src/util/object.h
diff --git a/src/Makefile.am b/src/Makefile.am index 645119e..3ebabe9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -81,6 +81,7 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/xml.c util/xml.h \ util/virtaudit.c util/virtaudit.h \ + util/object.c util/object.h \
Can you rename this to virobject.c / virobject.h. NB without
OK.
the 't'. I want rename the virtaudit/virterror stuff later. Ideally all new files in util/ should have a 'vir' prefix, since our current names are so short & generic they can easily clash and / or cause confusion. eg, the name of the file should match the name of the APIs inside, so virObjectPtr -> virobject.h
diff --git a/src/util/object.c b/src/util/object.c new file mode 100644 index 0000000..31ea27b --- /dev/null +++ b/src/util/object.c +#include <assert.h>
NB, our coding guidelines don't allow use of assert().
You can use sa_assert() which is a no-op just designed to be detectable by static analysis tools.
OK.
+#include "object.h" + +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj)) +{ + if (!free) + return -1;
We should raise an error here
OK.
+ + obj->ref = 1; + obj->free = free; + + return 0; +} + +#define virAtomicInc(value) \ + __sync_add_and_fetch(&value, 1) +#define virAtomicDec(value) \ + __sync_sub_and_fetch(&value, 1)
I reckon it would be a good idea to put these into a separate file 'src/util/virtatomic.{h,c}'.
Agree. Will do it. Is the `virt' prefix a typo? Since you've suggested `vir' prefix for files in util/.
Yes, I mean viratomic.h
Also, I think we need to make this more portable, since at least on Windows, I don't think we want to assume use of GCC. I think we should at the very least have an impl for GCC builtins, and an impl for Win32 to start with. If someone wants to do further impls later they can... If you look at glib's gatomic.c you'll see example code for Win32 atomic ops which is LGPLv2+ licensed
Yes, atomic ops should be portable, not limited just on gcc. Can we borrow glib's win32 atomic ops? Will it cause any license conflict? Anyway I will have a look at glib's win32 atomic implementation. Thanks for the information.
Glib's atomic ops code is LGPLv2+ licensed, so we will be fine borrowing the relevant bits of it. Regards, 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 :|
--- 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; - } } void qemuDomainObjEnterRemoteWithDriver(struct qemud_driver *driver, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 37401df..d37265e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -37,6 +37,7 @@ #include "memory.h" #include "logging.h" #include "files.h" +#include "object.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -44,11 +45,10 @@ #define DEBUG_RAW_IO 0 struct _qemuMonitor { + virObject obj; virMutex lock; /* also used to protect fd */ virCond notify; - int refs; - int fd; int watch; int hasSendFD; @@ -202,35 +202,66 @@ void qemuMonitorUnlock(qemuMonitorPtr mon) } -static void qemuMonitorFree(qemuMonitorPtr mon) +static void doMonitorFree(virObjectPtr obj) { + qemuMonitorPtr mon = (qemuMonitorPtr)obj; + VIR_DEBUG("mon=%p", mon); if (mon->cb && mon->cb->destroy) (mon->cb->destroy)(mon, mon->vm); - if (virCondDestroy(&mon->notify) < 0) - {} + ignore_value(virCondDestroy(&mon->notify)); virMutexDestroy(&mon->lock); VIR_FREE(mon->buffer); VIR_FREE(mon); } -int qemuMonitorRef(qemuMonitorPtr mon) +static qemuMonitorPtr qemuMonitorAlloc(void) { - mon->refs++; - return mon->refs; + qemuMonitorPtr mon; + + if (VIR_ALLOC(mon) < 0) { + virReportOOMError(); + return NULL; + } + + if (virObjectInit(&mon->obj, doMonitorFree)) { + VIR_FREE(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; + + return mon; } -int qemuMonitorUnref(qemuMonitorPtr mon) +static void qemuMonitorFree(qemuMonitorPtr mon) { - mon->refs--; + virObjectUnref(&mon->obj); +} - 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); 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); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index ece15a8..4d6b12e 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -130,8 +130,8 @@ int qemuMonitorSetCapabilities(qemuMonitorPtr mon); void qemuMonitorLock(qemuMonitorPtr mon); void qemuMonitorUnlock(qemuMonitorPtr mon); -int qemuMonitorRef(qemuMonitorPtr mon); -int qemuMonitorUnref(qemuMonitorPtr mon); +void qemuMonitorRef(qemuMonitorPtr mon); +void qemuMonitorUnref(qemuMonitorPtr mon); /* These APIs are for use by the internal Text/JSON monitor impl code only */ int qemuMonitorSend(qemuMonitorPtr mon, -- 1.7.3.1 -- Thanks, Hu Tao
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 :|
On Fri, Mar 18, 2011 at 11:20:56AM +0000, Daniel P. Berrange wrote:
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().
You're all right except on when to ref the monitor. The monitor may be freed by another thread before call of qemuDomainObjEnterMonitor(), so call of qemuMonitorRef() at this time doesn't help. I think the monitor must have already been refed before a thread starts operating it. (After checking this patch carefully, I found this patch doesn't do so, and it is buggy)
-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.
qemuMonitorFree is paired with qemuMonitorAlloc to prevent people from being confused if they have to call qemuMonitorUnref to free a monitor. But it's OK to get rid of qemuMonitorFree() if well documented.
- 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 ?
Sorry lack of a call to qemuMonitorUnref(mon) if virEventAddHandle fails.
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 :|
On Mon, Mar 21, 2011 at 06:15:36PM +0800, Hu Tao wrote:
On Fri, Mar 18, 2011 at 11:20:56AM +0000, Daniel P. Berrange wrote:
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().
You're all right except on when to ref the monitor. The monitor may be freed by another thread before call of qemuDomainObjEnterMonitor(), so call of qemuMonitorRef() at this time doesn't help. I think the monitor must have already been refed before a thread starts operating it.
No, that scenario should not be possible. As per src/qemu/THREADS.txt, the 'virDomainObjPtr' lock *must* be held when making any change to that object, so if 'vm' is locked, and you have checked virDomainObjIsActive, then it should not be poissible for ',mon' to be NULL in the call to qemuDomainObjEnterMonitor. Consider this sequence: virDomainObjPtr vm; qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); /* The mutex on 'vm' is held at this point */ if (qemuDomainObjBeginJob(vm) < 0) goto cleanup; /* We know that no other thread has a monitor command active at this point. */ if (virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); goto cleanup; } /* We know that priv->mon is non-NULL at this point */ qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitor(vm); /* 'vm' is now unlocked, but priv->mon cannot be free()d by the I/O thread, because EnterMonitor() acquired an extra reference on it */ ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats); qemuDomainObjExitMonitor(vm); /* 'vm' is now locked again, but priv->mon may now be NULL, since ExitMonitor released a reference */ if (qemuDomainObjEndJob(vm) == 0) vm = NULL; /* vm may be NULL, becuase EndJob released a reference on it, that was acquired by BeginJob */ Provided the call to virDomainObjIsActive() is not forgotten, we can be sure that 'mon' exists in qemuDomainObjEnterMonitor. The qemuDomainObjEnterMonitor() call, acquires an extra reference to guarentee that the monitor is never free'd while the qemuMonitorGetMemoryStats() API is running, even if the I/O thread releases the primary reference on the monitor. The qemuDomainObjExitMonitor() call releases the reference that was acquired by EnterMonitor, so, it is possible that the monitor is now freed. If a second monitor call is to be made now, the virDomainObjIsActive API *must* be checked again, before calling EnterMonitor again. Regards, 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 Wed, Mar 16, 2011 at 06:29:30PM +0800, Hu Tao wrote:
virObject is a base struct that manages reference-counting. structs that need the ability of reference-counting can inherit from virObject and implement ref/unref interface easily.
The goal of this series is to make reference-counting easy to use, and improve the current libvir reference-counting mechanism. The plan is to update all existing structs that use reference-counting to use virObject if virObject is acceptable.
Patch 1 implements virObject, patch 2 is an illstration of usage of virObject. This series is in draft stage, any comments are welcome.
I think this is a good idea. I was looking at glib's GObject code the other day and noticed that they used atomic arithmetic for lockless reference counting. If its good enough for them, it is good enough for us too. Regards, 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 :|
participants (2)
-
Daniel P. Berrange -
Hu Tao