On Wed, Jul 19, 2017 at 04:31:49PM +0200, Michal Privoznik wrote:
Up until now we only had virObjectLockable which uses mutexes for mutually excluding each other in critical section. Well, this is not enough. Future work will require RW locks so we might as well have virObjectRWLockable which is introduced here.
Moreover, polymorphism is introduced to our code for the first time. Yay! More specifically, virObjectLock will grab a write lock, virObjectLockRead will grab a read lock then (what a surprise right?). This has great advantage that an object can be made derived from virObjectRWLockable in a single line and still continue functioning properly (mutexes can be viewed as grabbing write locks only). Then just those critical sections that can grab a read lock need fixing. Therefore the resulting change is going to be way smaller.
In order to avoid writer starvation, the object initializes RW lock that prefers writers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 3 + src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++++----------- src/util/virobject.h | 16 ++++++ 3 files changed, 131 insertions(+), 32 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a792e00c8..f9df35583 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2282,6 +2282,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectRWLockable; virClassIsDerivedFrom; virClassName; virClassNew; @@ -2292,8 +2293,10 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLockRead; virObjectNew; virObjectRef; +virObjectRWLockableNew; virObjectUnlock; virObjectUnref;
diff --git a/src/util/virobject.c b/src/util/virobject.c index 34805d34a..3e7a0719e 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -49,8 +49,10 @@ struct _virClass {
static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; +static virClassPtr virObjectRWLockableClass;
static void virObjectLockableDispose(void *anyobj); +static void virObjectRWLockableDispose(void *anyobj);
static int virObjectOnceInit(void) @@ -67,6 +69,12 @@ virObjectOnceInit(void) virObjectLockableDispose))) return -1;
+ if (!(virObjectRWLockableClass = virClassNew(virObjectClass, + "virObjectRWLockable", + sizeof(virObjectRWLockable), + virObjectRWLockableDispose))) + return -1; + return 0; }
@@ -103,6 +111,20 @@ virClassForObjectLockable(void) }
+/** + * virClassForObjectRWLockable: + * + * Returns the class instance for the virObjectRWLockable type + */ +virClassPtr +virClassForObjectRWLockable(void) +{ + if (virObjectInitialize() < 0) + return NULL; + + return virObjectRWLockableClass; +} +
There should be two empty lines.
/** * virClassNew: * @parent: the parent class @@ -237,6 +259,32 @@ virObjectLockableNew(virClassPtr klass) }
+void * +virObjectRWLockableNew(virClassPtr klass) +{ + virObjectRWLockablePtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectRWLockable())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectRWLockable"), + virClassName(klass)); + return NULL; + } + + if (!(obj = virObjectNew(klass))) + return NULL; + + if (virRWLockInitPreferWriter(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize RW lock")); + virObjectUnref(obj); + return NULL; + } + + return obj; +} + + static void virObjectLockableDispose(void *anyobj) { @@ -246,6 +294,15 @@ virObjectLockableDispose(void *anyobj) }
+static void +virObjectRWLockableDispose(void *anyobj) +{ + virObjectRWLockablePtr obj = anyobj; + + virRWLockDestroy(&obj->lock); +} + + /** * virObjectUnref: * @anyobj: any instance of virObjectPtr @@ -309,28 +366,13 @@ virObjectRef(void *anyobj) }
-static virObjectLockablePtr -virObjectGetLockableObj(void *anyobj) -{ - virObjectPtr obj; - - if (virObjectIsClass(anyobj, virObjectLockableClass)) - return anyobj; - - obj = anyobj; - VIR_WARN("Object %p (%s) is not a virObjectLockable instance", - anyobj, obj ? obj->klass->name : "(unknown)"); - - return NULL; -} - - /** * virObjectLock: - * @anyobj: any instance of virObjectLockablePtr + * @anyobj: any instance of virObjectLockable or virObjectRWLockable * - * Acquire a lock on @anyobj. The lock must be - * released by virObjectUnlock. + * Acquire a lock on @anyobj. The lock must be released by + * virObjectUnlock. In case the passed object is instance of + * virObjectRWLockable a write lock is acquired. * * The caller is expected to have acquired a reference * on the object before locking it (eg virObjectRef). @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj) void virObjectLock(void *anyobj) { - virObjectLockablePtr obj = virObjectGetLockableObj(anyobj); + if (virObjectIsClass(anyobj, virObjectLockableClass)) { + virObjectLockablePtr obj = anyobj; + virMutexLock(&obj->lock); + } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockWrite(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectLockable " + "nor virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } +}
- if (!obj) - return;
- virMutexLock(&obj->lock); +/** + * virObjectLockRead: + * @anyobj: any instance of virObjectRWLockablePtr
s/virObjectRWLockablePtr/virObjectRWLockable/ Reviewed-by: Pavel Hrdina <phrdina@redhat.com>