[libvirt] [PATCH 0/3] Introduce RW locks to virDomainObjList
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement. Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-) -- 2.13.0
We already have virRWLockInit. But this uses pthread defaults which prefer reader to initialize the RW lock. This may lead to writer starvation. Therefore we need to have the counterpart that prefers writers. Now, according to the pthread_rwlockattr_setkind_np() man page setting PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP attribute. So much for good enum value names. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virthread.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virthread.h | 1 + 3 files changed, 37 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 187b12b32..a792e00c8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2728,6 +2728,7 @@ virMutexUnlock; virOnce; virRWLockDestroy; virRWLockInit; +virRWLockInitPreferWriter; virRWLockRead; virRWLockUnlock; virRWLockWrite; diff --git a/src/util/virthread.c b/src/util/virthread.c index 6c495158f..a8dd72f8b 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -95,6 +95,15 @@ void virMutexUnlock(virMutexPtr m) } +/** + * virRWLockInit: + * @m: rwlock to init + * + * Initializes RW lock using pthread default attributes (which + * is PTHREAD_RWLOCK_PREFER_READER_NP). + * + * Returns 0 on success, -1 otherwise. + */ int virRWLockInit(virRWLockPtr m) { int ret; @@ -106,6 +115,32 @@ int virRWLockInit(virRWLockPtr m) return 0; } + +/** + * virRWLockInitPreferWriter: + * @m: rwlock to init + * + * Initializes RW lock which prefers writers over readers. + * + * Returns 0 on success, -1 otherwise. + */ +int virRWLockInitPreferWriter(virRWLockPtr m) +{ + int ret; + pthread_rwlockattr_t attr; + + pthread_rwlockattr_init(&attr); + pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); + ret = pthread_rwlock_init(&m->lock, &attr); + pthread_rwlockattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + + void virRWLockDestroy(virRWLockPtr m) { pthread_rwlock_destroy(&m->lock); diff --git a/src/util/virthread.h b/src/util/virthread.h index e466d9bf0..18b785af2 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -136,6 +136,7 @@ void virMutexUnlock(virMutexPtr m); int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +int virRWLockInitPreferWriter(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; void virRWLockDestroy(virRWLockPtr m); void virRWLockRead(virRWLockPtr m); -- 2.13.0
On Wed, Jul 19, 2017 at 04:31:48PM +0200, Michal Privoznik wrote:
We already have virRWLockInit. But this uses pthread defaults which prefer reader to initialize the RW lock. This may lead to writer starvation. Therefore we need to have the counterpart that prefers writers. Now, according to the pthread_rwlockattr_setkind_np() man page setting PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP attribute. So much for good enum value names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virthread.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virthread.h | 1 + 3 files changed, 37 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
We already have virRWLockInit. But this uses pthread defaults which prefer reader to initialize the RW lock. This may lead to writer starvation. Therefore we need to have the counterpart that prefers writers. Now, according to the pthread_rwlockattr_setkind_np() man page setting PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP attribute. So much for good enum value names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virthread.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virthread.h | 1 + 3 files changed, 37 insertions(+)
This has broken the CI build, freebsd is not happy: ../../src/util/virthread.c:133:42: error: use of undeclared identifier 'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP' pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); ^ 1 error generated. John You know what my suggestion is ;-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 187b12b32..a792e00c8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2728,6 +2728,7 @@ virMutexUnlock; virOnce; virRWLockDestroy; virRWLockInit; +virRWLockInitPreferWriter; virRWLockRead; virRWLockUnlock; virRWLockWrite; diff --git a/src/util/virthread.c b/src/util/virthread.c index 6c495158f..a8dd72f8b 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -95,6 +95,15 @@ void virMutexUnlock(virMutexPtr m) }
+/** + * virRWLockInit: + * @m: rwlock to init + * + * Initializes RW lock using pthread default attributes (which + * is PTHREAD_RWLOCK_PREFER_READER_NP). + * + * Returns 0 on success, -1 otherwise. + */ int virRWLockInit(virRWLockPtr m) { int ret; @@ -106,6 +115,32 @@ int virRWLockInit(virRWLockPtr m) return 0; }
+ +/** + * virRWLockInitPreferWriter: + * @m: rwlock to init + * + * Initializes RW lock which prefers writers over readers. + * + * Returns 0 on success, -1 otherwise. + */ +int virRWLockInitPreferWriter(virRWLockPtr m) +{ + int ret; + pthread_rwlockattr_t attr; + + pthread_rwlockattr_init(&attr); + pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); + ret = pthread_rwlock_init(&m->lock, &attr); + pthread_rwlockattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + + void virRWLockDestroy(virRWLockPtr m) { pthread_rwlock_destroy(&m->lock); diff --git a/src/util/virthread.h b/src/util/virthread.h index e466d9bf0..18b785af2 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -136,6 +136,7 @@ void virMutexUnlock(virMutexPtr m);
int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +int virRWLockInitPreferWriter(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; void virRWLockDestroy(virRWLockPtr m);
void virRWLockRead(virRWLockPtr m);
On 07/24/2017 07:12 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
We already have virRWLockInit. But this uses pthread defaults which prefer reader to initialize the RW lock. This may lead to writer starvation. Therefore we need to have the counterpart that prefers writers. Now, according to the pthread_rwlockattr_setkind_np() man page setting PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP attribute. So much for good enum value names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virthread.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virthread.h | 1 + 3 files changed, 37 insertions(+)
This has broken the CI build, freebsd is not happy:
../../src/util/virthread.c:133:42: error: use of undeclared identifier 'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP' pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); ^ 1 error generated.
John
You know what my suggestion is ;-)
Actually, I don't. I am trying to search freebsd documentation on initializing RW locks so that they prefer writers. But no luck so far. Does anybody here on the list know? Although, for the current usage of RW locks it's really hard to starve a writer. But if this is going to be used more broadly it is going to be easier. On the other hand, I just realized that while one can use mutexes + conditions (like we do for domain jobs), using RW locks and conditions is not implemented anywhere, so I will have to come up with some idea. Long story short, for now we don't care if writer will starve. Michal
On Mon, Jul 24, 2017 at 01:12:59PM -0400, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
We already have virRWLockInit. But this uses pthread defaults which prefer reader to initialize the RW lock. This may lead to writer starvation. Therefore we need to have the counterpart that prefers writers. Now, according to the pthread_rwlockattr_setkind_np() man page setting PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP attribute. So much for good enum value names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virthread.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virthread.h | 1 + 3 files changed, 37 insertions(+)
This has broken the CI build, freebsd is not happy:
../../src/util/virthread.c:133:42: error: use of undeclared identifier 'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP' pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); ^ 1 error generated.
It is not just FreeBSD, it also breaks OS-X and Win32. The suffix '_np' / '_NP' is shorthand for nNon portable" - these are glibc inventions. IMHO we should not really use this in our code as if we're going to make assumptions that writers are not starved as a result, because writers will be starved on any non-Linux platform. IOW, I think we need to just revert this. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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; +} + /** * 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 + * + * Acquire a read lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +void +virObjectLockRead(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockRead(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } } /** * virObjectUnlock: - * @anyobj: any instance of virObjectLockablePtr + * @anyobj: any instance of virObjectLockable or virObjectRWLockable * - * Release a lock on @anyobj. The lock must have been - * acquired by virObjectLock. + * Release a lock on @anyobj. The lock must have been acquired by + * virObjectLock or virObjectLockRead. */ void virObjectUnlock(void *anyobj) { - virObjectLockablePtr obj = virObjectGetLockableObj(anyobj); - - if (!obj) - return; - - virMutexUnlock(&obj->lock); + if (virObjectIsClass(anyobj, virObjectLockableClass)) { + virObjectLockablePtr obj = anyobj; + virMutexUnlock(&obj->lock); + } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockUnlock(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectLockable " + "nor virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } } diff --git a/src/util/virobject.h b/src/util/virobject.h index f4c292b16..5985fadb2 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -34,6 +34,9 @@ typedef virObject *virObjectPtr; typedef struct _virObjectLockable virObjectLockable; typedef virObjectLockable *virObjectLockablePtr; +typedef struct _virObjectRWLockable virObjectRWLockable; +typedef virObjectRWLockable *virObjectRWLockablePtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -59,9 +62,14 @@ struct _virObjectLockable { virMutex lock; }; +struct _virObjectRWLockable { + virObject parent; + virRWLock lock; +}; virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); +virClassPtr virClassForObjectRWLockable(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -108,10 +116,18 @@ void * virObjectLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +void * +virObjectRWLockableNew(virClassPtr klass) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); +void +virObjectLockRead(void *lockableobj) + ATTRIBUTE_NONNULL(1); + void virObjectUnlock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.13.0
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>
[...]
/** * 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 + * + * Acquire a read lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +void +virObjectLockRead(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockRead(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } }
Hopefully no one "confuses" which object is which or no one starts using virObjectLockRead for a virObjectLockable and expects that read lock to be in place (or error out) or gasp actually wait for a write lock to release the lock as this does not error out. This is a danger in the long term assumption of having a void function that has callers that can make the assumption that upon return the Lock really was taken. Perhaps the better way to attack this would have been to create a virObjectLockWrite() function called only from vir*ObjListAdd and vir*ObjListRemove leaving the other virObjectLock()'s in tact and having the virObjectLock() code make the decision over whether to use the virRWLockRead or virMutexLock depending on the object class. John
/** * virObjectUnlock: - * @anyobj: any instance of virObjectLockablePtr + * @anyobj: any instance of virObjectLockable or virObjectRWLockable * - * Release a lock on @anyobj. The lock must have been - * acquired by virObjectLock. + * Release a lock on @anyobj. The lock must have been acquired by + * virObjectLock or virObjectLockRead. */ void virObjectUnlock(void *anyobj) { - virObjectLockablePtr obj = virObjectGetLockableObj(anyobj); - - if (!obj) - return; - - virMutexUnlock(&obj->lock); + if (virObjectIsClass(anyobj, virObjectLockableClass)) { + virObjectLockablePtr obj = anyobj; + virMutexUnlock(&obj->lock); + } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockUnlock(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectLockable " + "nor virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } }
[...]
On 07/24/2017 05:22 PM, John Ferlan wrote:
[...]
/** * 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 + * + * Acquire a read lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +void +virObjectLockRead(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockRead(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } }
Hopefully no one "confuses" which object is which or no one starts using virObjectLockRead for a virObjectLockable and expects that read lock to be in place (or error out) or gasp actually wait for a write lock to release the lock as this does not error out.
This could have already happened if one would pass bare virObject to virObjectLock().
This is a danger in the long term assumption of having a void function that has callers that can make the assumption that upon return the Lock really was taken.
Well, I agree that this is one more thing to be cautious about, but no more than making sure object passed to virObjectLock() is virObjectLockable.
Perhaps the better way to attack this would have been to create a virObjectLockWrite() function called only from vir*ObjListAdd and vir*ObjListRemove leaving the other virObjectLock()'s in tact and having the virObjectLock() code make the decision over whether to use the virRWLockRead or virMutexLock depending on the object class.
What's the difference? If virObjectLock() does what you're suggesting (what indeed is implemented too), what's the point in having virObjectLockWrite()? Michal
On 07/25/2017 03:45 AM, Michal Privoznik wrote:
On 07/24/2017 05:22 PM, John Ferlan wrote:
[...]
/** * 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 + * + * Acquire a read lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +void +virObjectLockRead(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockRead(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } }
Hopefully no one "confuses" which object is which or no one starts using virObjectLockRead for a virObjectLockable and expects that read lock to be in place (or error out) or gasp actually wait for a write lock to release the lock as this does not error out.
This could have already happened if one would pass bare virObject to virObjectLock().
Consider passing a non virObject (such as what happened during an early change to the code for me - a virHashTablePtr) to virObjectLock... In any case, we'll have to be more vigilant now to know that only ObjList @obj's for virdomainobjlist can use this new API. Longer term I'll adjust to use them.
This is a danger in the long term assumption of having a void function that has callers that can make the assumption that upon return the Lock really was taken.
Well, I agree that this is one more thing to be cautious about, but no more than making sure object passed to virObjectLock() is virObjectLockable.
There are some ugly ways to handle this including using some kind of macro for virObjectLock that would force an abort of some sort. We have been "fortunate" to have well behaved and reviewed code, but with two lock API's now it's just one of those things to keep track of for reviews.
Perhaps the better way to attack this would have been to create a virObjectLockWrite() function called only from vir*ObjListAdd and vir*ObjListRemove leaving the other virObjectLock()'s in tact and having the virObjectLock() code make the decision over whether to use the virRWLockRead or virMutexLock depending on the object class.
What's the difference? If virObjectLock() does what you're suggesting (what indeed is implemented too), what's the point in having virObjectLockWrite()?
Michal
1. Less code and a lock that could check status... I understand not wanting to modify 10 callers to check status, but modifying two and thus making it clearer to the caller that these are "different" might not be a bad thing. 2. The "default" action being more consumers probably will use the Read locks (as I believe is the premise/impetus of the series). The Add/Remove would seemingly be used less and thus the exception. So why not have the default for ObjList/virObjectRWLockableClass be to have the virObjectLock use a virRWLockRead instead of virRWLockWrite? What's done is done - I obviously have ulterior motives to not wanting virobject to change that much, but I've been considering the downside of the code that has void functions that really don't return a lock on the object if the wrong object was passed. John
On 07/25/2017 02:13 PM, John Ferlan wrote:
On 07/25/2017 03:45 AM, Michal Privoznik wrote:
On 07/24/2017 05:22 PM, John Ferlan wrote:
[...]
/** * 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 + * + * Acquire a read lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +void +virObjectLockRead(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockRead(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } }
Hopefully no one "confuses" which object is which or no one starts using virObjectLockRead for a virObjectLockable and expects that read lock to be in place (or error out) or gasp actually wait for a write lock to release the lock as this does not error out.
This could have already happened if one would pass bare virObject to virObjectLock().
Consider passing a non virObject (such as what happened during an early change to the code for me - a virHashTablePtr) to virObjectLock...
Well, that's what happens in C to functions accepting void pointer. In this sense yes, we are not true OOP - compiler would catch that you're calling a method that is not defined for the class. But since we're implementing OOP in runtime, we are able to catch OOP related problem only at runtime. I'm not sure it is something we can check for at compile time. And I think other C libraries that re-implement OOP (like glib) do just the same as us.
In any case, we'll have to be more vigilant now to know that only ObjList @obj's for virdomainobjlist can use this new API.
Why? Maybe I'm misunderstanding what you're saying, but I think that network driver, and basically any other driver which uses vir*ObjList can use it. And not just lists. Other objects too - e.g. qemuCaps, virDomainCaps, etc (I really roughly skimmed through users of virObjectLockable). And @obj's are untouched with this change, aren't they? I feel like we're not on the same page here. My change just allowed two threads to look up domains at the same time. It doesn't affect domains in any way.
Longer term I'll adjust to use them.
This is a danger in the long term assumption of having a void function that has callers that can make the assumption that upon return the Lock really was taken.
Well, I agree that this is one more thing to be cautious about, but no more than making sure object passed to virObjectLock() is virObjectLockable.
There are some ugly ways to handle this including using some kind of macro for virObjectLock that would force an abort of some sort. We have been "fortunate" to have well behaved and reviewed code, but with two lock API's now it's just one of those things to keep track of for reviews.
I don't find this change that frightening, but I agree that bare VIR_WARN() we have might not be enough. There are plenty ways where malicious pointers can be passed to virObject* APIs, and especially virObjectLock(). Worst case scenario we might pass some "random" address in memory, make the caller think the object is locked while in fact it is not. We are strongly against abort(), but maybe we can revisit that decision in this case because it can lead to data loss/corruption. This error is different to OOM error or 'unable to start a domain' one.
Perhaps the better way to attack this would have been to create a virObjectLockWrite() function called only from vir*ObjListAdd and vir*ObjListRemove leaving the other virObjectLock()'s in tact and having the virObjectLock() code make the decision over whether to use the virRWLockRead or virMutexLock depending on the object class.
What's the difference? If virObjectLock() does what you're suggesting (what indeed is implemented too), what's the point in having virObjectLockWrite()?
Michal
1. Less code and a lock that could check status... I understand not wanting to modify 10 callers to check status, but modifying two and thus making it clearer to the caller that these are "different" might not be a bad thing.
Hold on. If we are going to make virObjectLock() return an error - why do it just for virObjectRWLockable? Doing the following is no less worse: char a[] = "Some random string"; virObjectLock(a);
2. The "default" action being more consumers probably will use the Read locks (as I believe is the premise/impetus of the series). The Add/Remove would seemingly be used less and thus the exception. So why not have the default for ObjList/virObjectRWLockableClass be to have the virObjectLock use a virRWLockRead instead of virRWLockWrite?
I beg to disagree. Whenever I see 'virObjectLock' I am sure that no other thread is changing the object that is guarded by the mutex. If, however, virObjectLock was grabbing read lock, it would be very confusing and I would have to check every time whether the object I'm passing is virObjectLockable or virObjectRWLockable because the virObjectLock() function would behave differently depending on class of the object. Moreover, now I can do the following and the code still works: diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c index ccde72e72..4fe13fc40 100644 --- i/src/conf/virnetworkobj.c +++ w/src/conf/virnetworkobj.c @@ -60 +60 @@ virNetworkObjOnceInit(void) - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(), diff --git i/src/conf/virnetworkobj.h w/src/conf/virnetworkobj.h index 8090c2e24..ee4a939f2 100644 --- i/src/conf/virnetworkobj.h +++ w/src/conf/virnetworkobj.h @@ -30 +30 @@ struct _virNetworkObj { - virObjectLockable parent; + virObjectRWLockable parent; With your suggestion, I'd have to change all virObjectLock() in src/conf/virnetworkobj.c to virObjectLockWrite() (all that lock the hash table, not network object itself). Michal
On 07/25/2017 04:25 PM, Michal Privoznik wrote:
<snip/> Moreover, now I can do the following and the code still works:
diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c index ccde72e72..4fe13fc40 100644 --- i/src/conf/virnetworkobj.c +++ w/src/conf/virnetworkobj.c @@ -60 +60 @@ virNetworkObjOnceInit(void) - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(), diff --git i/src/conf/virnetworkobj.h w/src/conf/virnetworkobj.h index 8090c2e24..ee4a939f2 100644 --- i/src/conf/virnetworkobj.h +++ w/src/conf/virnetworkobj.h @@ -30 +30 @@ struct _virNetworkObj { - virObjectLockable parent; + virObjectRWLockable parent;
Hit 'Send' too soon. This should have been: diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c index ccde72e72..82be62832 100644 --- i/src/conf/virnetworkobj.c +++ w/src/conf/virnetworkobj.c @@ -41 +41 @@ struct _virNetworkObjList { - virObjectLockable parent; + virObjectRWLockable parent; @@ -60 +60 @@ virNetworkObjOnceInit(void) - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(), Obviously, rewriting virNetworkObj to use RW locks is gonna require some more work. Michal
On 07/25/2017 10:25 AM, Michal Privoznik wrote:
On 07/25/2017 02:13 PM, John Ferlan wrote:
On 07/25/2017 03:45 AM, Michal Privoznik wrote:
On 07/24/2017 05:22 PM, John Ferlan wrote:
[...]
/** * 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 + * + * Acquire a read lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +void +virObjectLockRead(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockRead(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } }
Hopefully no one "confuses" which object is which or no one starts using virObjectLockRead for a virObjectLockable and expects that read lock to be in place (or error out) or gasp actually wait for a write lock to release the lock as this does not error out.
This could have already happened if one would pass bare virObject to virObjectLock().
Consider passing a non virObject (such as what happened during an early change to the code for me - a virHashTablePtr) to virObjectLock...
Well, that's what happens in C to functions accepting void pointer. In this sense yes, we are not true OOP - compiler would catch that you're calling a method that is not defined for the class. But since we're implementing OOP in runtime, we are able to catch OOP related problem only at runtime. I'm not sure it is something we can check for at compile time. And I think other C libraries that re-implement OOP (like glib) do just the same as us.
Agreed - I cannot think of a way to capture this at compile time, but as part of that virObject series I did add some code to reduce the chance of some bad things happening during run time. They don't completely eliminate the possibility that someone calls virObject{Lock|Unlock} using an @obj that isn't a virObjectLockable. But it does protect against someone using virObject{Ref|Unref} in order to avoid an virAtomicIntInc or a virAtomicIntDecAndTest on a random @obj field (plus some worse settings on random fields if for some reason @lastRef is true). I hadn't come up with a good way to handle virObject{Lock|Unlock}, but this series jiggled the memory a bit and got me thinking...
In any case, we'll have to be more vigilant now to know that only ObjList @obj's for virdomainobjlist can use this new API.
Why? Maybe I'm misunderstanding what you're saying, but I think that network driver, and basically any other driver which uses vir*ObjList can use it. And not just lists. Other objects too - e.g. qemuCaps, virDomainCaps, etc (I really roughly skimmed through users of virObjectLockable).
Ok - wasn't as clear as I should have been... True, as long as the _vir*ObjList uses virObjectRWLockable and the vir*ObjListNew() uses virObjectRWLockableNew. Similar for other objects that want to do the right thing. I was considering someone that sees virObjectLockRead and tries to use it thinking - great, that's what I want - just a read lock since I'm not changing anything. But, they may not really get the Read lock if they pass a virObjectLockable... If not playing close attention during review to ensure that the call is done on an appropriate @obj (especially when both are used in some same function), then something may be missed. TBH: We may find "other" uses of concurrent read locks even for @obj's. IIUC, if an @objlist has a read/RW lock, then an Add/Remove path cannot get the virMutexLock until that read lock is Unlocked. Likewise, there is @obj code that gets the virMutexLock even though it's not changing anything - it's just reading some field. Shouldn't it be safe to have multiple readers in this case too? I think of all those list traversal functions which grab the lock just to read/check/copy something. Because we have an RW lock on the @objlist, the contention for locks between threads now jumps to the @obj's.
And @obj's are untouched with this change, aren't they? I feel like we're not on the same page here. My change just allowed two threads to look up domains at the same time. It doesn't affect domains in any way.
I understand what the change did... Hopefully the above helps explain my thoughts better.
Longer term I'll adjust to use them.
This is a danger in the long term assumption of having a void function that has callers that can make the assumption that upon return the Lock really was taken.
Well, I agree that this is one more thing to be cautious about, but no more than making sure object passed to virObjectLock() is virObjectLockable.
There are some ugly ways to handle this including using some kind of macro for virObjectLock that would force an abort of some sort. We have been "fortunate" to have well behaved and reviewed code, but with two lock API's now it's just one of those things to keep track of for reviews.
I don't find this change that frightening, but I agree that bare VIR_WARN() we have might not be enough. There are plenty ways where malicious pointers can be passed to virObject* APIs, and especially virObjectLock(). Worst case scenario we might pass some "random" address in memory, make the caller think the object is locked while in fact it is not. We are strongly against abort(), but maybe we can revisit that decision in this case because it can lead to data loss/corruption. This error is different to OOM error or 'unable to start a domain' one.
Currently way too many virObjectLock's and virObjectRef's to make it possible to perform checking reasonably. The abort() is a possibility, but yes, ugly albeit perhaps safer than corruption. Still new code could have added error checking...
Perhaps the better way to attack this would have been to create a virObjectLockWrite() function called only from vir*ObjListAdd and vir*ObjListRemove leaving the other virObjectLock()'s in tact and having the virObjectLock() code make the decision over whether to use the virRWLockRead or virMutexLock depending on the object class.
What's the difference? If virObjectLock() does what you're suggesting (what indeed is implemented too), what's the point in having virObjectLockWrite()?
Michal
1. Less code and a lock that could check status... I understand not wanting to modify 10 callers to check status, but modifying two and thus making it clearer to the caller that these are "different" might not be a bad thing.
Hold on. If we are going to make virObjectLock() return an error - why do it just for virObjectRWLockable? Doing the following is no less worse:
char a[] = "Some random string"; virObjectLock(a);
char a[] = "Some random string"; if (!virObjectLock{Read|Write}(a)) virReportError("you ain't got it"); return -1
2. The "default" action being more consumers probably will use the Read locks (as I believe is the premise/impetus of the series). The Add/Remove would seemingly be used less and thus the exception. So why not have the default for ObjList/virObjectRWLockableClass be to have the virObjectLock use a virRWLockRead instead of virRWLockWrite?
I beg to disagree. Whenever I see 'virObjectLock' I am sure that no other thread is changing the object that is guarded by the mutex. If, however, virObjectLock was grabbing read lock, it would be very confusing and I would have to check every time whether the object I'm passing is virObjectLockable or virObjectRWLockable because the virObjectLock() function would behave differently depending on class of the object.
OK - fair argument for the opposing viewpoint. Makes sense. Still virObject{Lock|Unlock} is overloaded to do different things based on the object type which I recall being something I've been dinged on in the past. Considering the argument about checking the return value, it may {be|have been} a good opportunity to add checking to getting the lock.
Moreover, now I can do the following and the code still works:
diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c index ccde72e72..4fe13fc40 100644 --- i/src/conf/virnetworkobj.c +++ w/src/conf/virnetworkobj.c @@ -60 +60 @@ virNetworkObjOnceInit(void) - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(), diff --git i/src/conf/virnetworkobj.h w/src/conf/virnetworkobj.h index 8090c2e24..ee4a939f2 100644 --- i/src/conf/virnetworkobj.h +++ w/src/conf/virnetworkobj.h @@ -30 +30 @@ struct _virNetworkObj { - virObjectLockable parent; + virObjectRWLockable parent;
With your suggestion, I'd have to change all virObjectLock() in src/conf/virnetworkobj.c to virObjectLockWrite() (all that lock the hash table, not network object itself).
Obviously I hadn't completely through everything... But perhaps this also proves that under the covers we could have just converted virObjectLockable to be a virObjectRWLockable without creating the new class. Especially since the former patch 1 has been reverted and there's no difference between virObjectLockableNew and virObjectRWLockableNew other than which class was used to initialize. If they were combined and just the new API to perform the RW lock was added, then for paths that want to use read locks: if (!virObjectLockRead(obj)) error and fail ... IOW: At this point in time - what is the purpose of a separate virObjectRWLockableClass? John
With your suggestion, I'd have to change all virObjectLock() in src/conf/virnetworkobj.c to virObjectLockWrite() (all that lock the hash table, not network object itself).
Obviously I hadn't completely through everything...
But perhaps this also proves that under the covers we could have just converted virObjectLockable to be a virObjectRWLockable without creating the new class. Especially since the former patch 1 has been reverted and there's no difference between virObjectLockableNew and virObjectRWLockableNew other than which class was used to initialize.
If they were combined and just the new API to perform the RW lock was added, then for paths that want to use read locks:
if (!virObjectLockRead(obj)) error and fail
...
IOW: At this point in time - what is the purpose of a separate virObjectRWLockableClass?
nm: I needed to keep excavating... Still I think not using overloaded Lock/Unlock in order to allow new functions to return a failure status would be better. John
On 07/25/2017 05:47 PM, John Ferlan wrote:
On 07/25/2017 10:25 AM, Michal Privoznik wrote:
On 07/25/2017 02:13 PM, John Ferlan wrote:
On 07/25/2017 03:45 AM, Michal Privoznik wrote:
On 07/24/2017 05:22 PM, John Ferlan wrote:
[...]
/** * 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 + * + * Acquire a read lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +void +virObjectLockRead(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockRead(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } }
Hopefully no one "confuses" which object is which or no one starts using virObjectLockRead for a virObjectLockable and expects that read lock to be in place (or error out) or gasp actually wait for a write lock to release the lock as this does not error out.
This could have already happened if one would pass bare virObject to virObjectLock().
Consider passing a non virObject (such as what happened during an early change to the code for me - a virHashTablePtr) to virObjectLock...
Well, that's what happens in C to functions accepting void pointer. In this sense yes, we are not true OOP - compiler would catch that you're calling a method that is not defined for the class. But since we're implementing OOP in runtime, we are able to catch OOP related problem only at runtime. I'm not sure it is something we can check for at compile time. And I think other C libraries that re-implement OOP (like glib) do just the same as us.
Agreed - I cannot think of a way to capture this at compile time, but as part of that virObject series I did add some code to reduce the chance of some bad things happening during run time. They don't completely eliminate the possibility that someone calls virObject{Lock|Unlock} using an @obj that isn't a virObjectLockable. But it does protect against someone using virObject{Ref|Unref} in order to avoid an virAtomicIntInc or a virAtomicIntDecAndTest on a random @obj field (plus some worse settings on random fields if for some reason @lastRef is true).
I hadn't come up with a good way to handle virObject{Lock|Unlock}, but this series jiggled the memory a bit and got me thinking...
In any case, we'll have to be more vigilant now to know that only ObjList @obj's for virdomainobjlist can use this new API.
Why? Maybe I'm misunderstanding what you're saying, but I think that network driver, and basically any other driver which uses vir*ObjList can use it. And not just lists. Other objects too - e.g. qemuCaps, virDomainCaps, etc (I really roughly skimmed through users of virObjectLockable).
Ok - wasn't as clear as I should have been... True, as long as the _vir*ObjList uses virObjectRWLockable and the vir*ObjListNew() uses virObjectRWLockableNew. Similar for other objects that want to do the right thing.
I was considering someone that sees virObjectLockRead and tries to use it thinking - great, that's what I want - just a read lock since I'm not changing anything. But, they may not really get the Read lock if they pass a virObjectLockable... If not playing close attention during review to ensure that the call is done on an appropriate @obj (especially when both are used in some same function), then something may be missed.
Yes, there is this concern and I understand it. But to me it's at the same level as making sure that object passed to virObjectLock() is virObjectLockable. Then again, warning is produced at runtime, so as a part of developing/review process when testing the patches warning would be produced.
TBH: We may find "other" uses of concurrent read locks even for @obj's. IIUC, if an @objlist has a read/RW lock, then an Add/Remove path cannot get the virMutexLock until that read lock is Unlocked. Likewise, there is @obj code that gets the virMutexLock even though it's not changing anything - it's just reading some field. Shouldn't it be safe to have multiple readers in this case too? I think of all those list traversal functions which grab the lock just to read/check/copy something. Because we have an RW lock on the @objlist, the contention for locks between threads now jumps to the @obj's.
Yes, it definitely looks like it. However, it's slightly more complicated than that. At least in qemu we have these jobs which use condition to wait until the job can be set. The code looks something like this: pthread_cond_timedwait(vm->priv->job.cond, vm->parent.lock, then); where @cond is a pthread condition, @lock is a pthread mutex and @then is some timeout. Unfortunately, in pthread impl @lock really has to be a mutex, not an RW lock (see man pthread_cond_timedwait). I have an idea how to avoid this - move mutex to priv, right next to the condition, but that's completely different discussion. For now it's sufficient to say that we can't simply replace mutex for rw lock in virDomainObj. Some additional work is needed. But it is still feasible, I think.
And @obj's are untouched with this change, aren't they? I feel like we're not on the same page here. My change just allowed two threads to look up domains at the same time. It doesn't affect domains in any way.
I understand what the change did... Hopefully the above helps explain my thoughts better.
Longer term I'll adjust to use them.
This is a danger in the long term assumption of having a void function that has callers that can make the assumption that upon return the Lock really was taken.
Well, I agree that this is one more thing to be cautious about, but no more than making sure object passed to virObjectLock() is virObjectLockable.
There are some ugly ways to handle this including using some kind of macro for virObjectLock that would force an abort of some sort. We have been "fortunate" to have well behaved and reviewed code, but with two lock API's now it's just one of those things to keep track of for reviews.
I don't find this change that frightening, but I agree that bare VIR_WARN() we have might not be enough. There are plenty ways where malicious pointers can be passed to virObject* APIs, and especially virObjectLock(). Worst case scenario we might pass some "random" address in memory, make the caller think the object is locked while in fact it is not. We are strongly against abort(), but maybe we can revisit that decision in this case because it can lead to data loss/corruption. This error is different to OOM error or 'unable to start a domain' one.
Currently way too many virObjectLock's and virObjectRef's to make it possible to perform checking reasonably. The abort() is a possibility, but yes, ugly albeit perhaps safer than corruption.
Still new code could have added error checking...
Perhaps the better way to attack this would have been to create a virObjectLockWrite() function called only from vir*ObjListAdd and vir*ObjListRemove leaving the other virObjectLock()'s in tact and having the virObjectLock() code make the decision over whether to use the virRWLockRead or virMutexLock depending on the object class.
What's the difference? If virObjectLock() does what you're suggesting (what indeed is implemented too), what's the point in having virObjectLockWrite()?
Michal
1. Less code and a lock that could check status... I understand not wanting to modify 10 callers to check status, but modifying two and thus making it clearer to the caller that these are "different" might not be a bad thing.
Hold on. If we are going to make virObjectLock() return an error - why do it just for virObjectRWLockable? Doing the following is no less worse:
char a[] = "Some random string"; virObjectLock(a);
char a[] = "Some random string";
if (!virObjectLock{Read|Write}(a)) virReportError("you ain't got it"); return -1>
2. The "default" action being more consumers probably will use the Read locks (as I believe is the premise/impetus of the series). The Add/Remove would seemingly be used less and thus the exception. So why not have the default for ObjList/virObjectRWLockableClass be to have the virObjectLock use a virRWLockRead instead of virRWLockWrite?
I beg to disagree. Whenever I see 'virObjectLock' I am sure that no other thread is changing the object that is guarded by the mutex. If, however, virObjectLock was grabbing read lock, it would be very confusing and I would have to check every time whether the object I'm passing is virObjectLockable or virObjectRWLockable because the virObjectLock() function would behave differently depending on class of the object.
OK - fair argument for the opposing viewpoint. Makes sense.
Still virObject{Lock|Unlock} is overloaded to do different things based on the object type which I recall being something I've been dinged on in the past.
Considering the argument about checking the return value, it may {be|have been} a good opportunity to add checking to getting the lock.
Moreover, now I can do the following and the code still works:
diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c index ccde72e72..4fe13fc40 100644 --- i/src/conf/virnetworkobj.c +++ w/src/conf/virnetworkobj.c @@ -60 +60 @@ virNetworkObjOnceInit(void) - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(), diff --git i/src/conf/virnetworkobj.h w/src/conf/virnetworkobj.h index 8090c2e24..ee4a939f2 100644 --- i/src/conf/virnetworkobj.h +++ w/src/conf/virnetworkobj.h @@ -30 +30 @@ struct _virNetworkObj { - virObjectLockable parent; + virObjectRWLockable parent;
With your suggestion, I'd have to change all virObjectLock() in src/conf/virnetworkobj.c to virObjectLockWrite() (all that lock the hash table, not network object itself).
Obviously I hadn't completely through everything...
But perhaps this also proves that under the covers we could have just converted virObjectLockable to be a virObjectRWLockable without creating the new class. Especially since the former patch 1 has been reverted and there's no difference between virObjectLockableNew and virObjectRWLockableNew other than which class was used to initialize.
We can't do that because of pthread condition variables. Michal
There is no reason why two threads trying to look up two domains should mutually exclude each other. Utilize new virObjectRWLockable that was just introduced. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virdomainobjlist.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index c121382bc..856f24c35 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -42,7 +42,7 @@ static void virDomainObjListDispose(void *obj); struct _virDomainObjList { - virObjectLockable parent; + virObjectRWLockable parent; /* uuid string -> virDomainObj mapping * for O(1), lockless lookup-by-uuid */ @@ -56,7 +56,7 @@ struct _virDomainObjList { static int virDomainObjListOnceInit(void) { - if (!(virDomainObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virDomainObjListClass = virClassNew(virClassForObjectRWLockable(), "virDomainObjList", sizeof(virDomainObjList), virDomainObjListDispose))) @@ -74,7 +74,7 @@ virDomainObjListPtr virDomainObjListNew(void) if (virDomainObjListInitialize() < 0) return NULL; - if (!(doms = virObjectLockableNew(virDomainObjListClass))) + if (!(doms = virObjectRWLockableNew(virDomainObjListClass))) return NULL; if (!(doms->objs = virHashCreate(50, virObjectFreeHashData)) || @@ -118,7 +118,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, bool ref) { virDomainObjPtr obj; - virObjectLock(doms); + virObjectLockRead(doms); obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id); if (ref) { virObjectRef(obj); @@ -160,7 +160,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj; - virObjectLock(doms); + virObjectLockRead(doms); virUUIDFormat(uuid, uuidstr); obj = virHashLookup(doms->objs, uuidstr); @@ -204,7 +204,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, { virDomainObjPtr obj; - virObjectLock(doms); + virObjectLockRead(doms); obj = virHashLookup(doms->objsName, name); virObjectRef(obj); virObjectUnlock(doms); @@ -653,7 +653,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms, virConnectPtr conn) { struct virDomainObjListData data = { filter, conn, active, 0 }; - virObjectLock(doms); + virObjectLockRead(doms); virHashForEach(doms->objs, virDomainObjListCount, &data); virObjectUnlock(doms); return data.count; @@ -697,7 +697,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms, { struct virDomainIDData data = { filter, conn, 0, maxids, ids }; - virObjectLock(doms); + virObjectLockRead(doms); virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data); virObjectUnlock(doms); return data.numids; @@ -751,7 +751,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms, struct virDomainNameData data = { filter, conn, 0, 0, maxnames, names }; size_t i; - virObjectLock(doms); + virObjectLockRead(doms); virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); virObjectUnlock(doms); if (data.oom) { @@ -792,7 +792,7 @@ virDomainObjListForEach(virDomainObjListPtr doms, struct virDomainListIterData data = { callback, opaque, 0, }; - virObjectLock(doms); + virObjectLockRead(doms); virHashForEach(doms->objs, virDomainObjListHelper, &data); virObjectUnlock(doms); return data.ret; @@ -925,7 +925,7 @@ virDomainObjListCollect(virDomainObjListPtr domlist, { struct virDomainListData data = { NULL, 0 }; - virObjectLock(domlist); + virObjectLockRead(domlist); sa_assert(domlist->objs); if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) { virObjectUnlock(domlist); @@ -962,7 +962,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist, *nvms = 0; *vms = NULL; - virObjectLock(domlist); + virObjectLockRead(domlist); for (i = 0; i < ndoms; i++) { virDomainPtr dom = doms[i]; -- 2.13.0
On Wed, Jul 19, 2017 at 04:31:50PM +0200, Michal Privoznik wrote:
There is no reason why two threads trying to look up two domains should mutually exclude each other. Utilize new virObjectRWLockable that was just introduced.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virdomainobjlist.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On 07/24/2017 03:36 PM, Pavel Hrdina wrote:
On Wed, Jul 19, 2017 at 04:31:50PM +0200, Michal Privoznik wrote:
There is no reason why two threads trying to look up two domains should mutually exclude each other. Utilize new virObjectRWLockable that was just introduced.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virdomainobjlist.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Sweet. I've fixed 2/3 and pushed. Thank you! Michal
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement.
Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable
src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-)
This could be a "next step" in the work I've been doing towards a common object: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html which moves all those driver/vir*obj list API's for Lookup, Search, ForEach, Add, Remove, etc. into object code since they're essentially all following the same pattern. Once there - altering the Lockable lock under the covers should be relatively simple. I would be called a ListLock or HashLock instead of an RWLock and the implementation is such that it's R or W depending on API. Taking a quick refresher look at the series, for a new object I call virObjectLookupHashClass - that could be the integration point to use a local initialization for the class and then the appropriate lock style depending on API. Just thinking off the cuff and of course trying to keep stuff I've been working on fresh ;-) John
On 07/23/2017 02:10 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement.
Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable
src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-)
This could be a "next step" in the work I've been doing towards a common object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
which moves all those driver/vir*obj list API's for Lookup, Search, ForEach, Add, Remove, etc. into object code since they're essentially all following the same pattern.
Once there - altering the Lockable lock under the covers should be relatively simple. I would be called a ListLock or HashLock instead of an RWLock and the implementation is such that it's R or W depending on API. Taking a quick refresher look at the series, for a new object I call virObjectLookupHashClass - that could be the integration point to use a local initialization for the class and then the appropriate lock style depending on API.
I think I still prefer "RWLock" name over "ListLock" or "HashLock" since its name clearly suggests its purpose. I mean, ListLock doesn't say it's an RW lock. Moreover, as I say in the cover letter, I'd like to use RW locks for virDomainObj one day (for instance, there's no reason why two clients cannot fetch XML for the same domain at the same time). Therefore, it looks correct to me to derive virDomainObjClass from virObjectRWLockable instead of ListLock or HashLock or something.
Just thinking off the cuff and of course trying to keep stuff I've been working on fresh ;-)
Sure, the more recent some patches are the more I understand them. Same here :-) Michal
On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
On 07/23/2017 02:10 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement.
Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable
src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-)
This could be a "next step" in the work I've been doing towards a common object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
I'm still not sure about the implementation that you are heading to, I would actually prefer something similar to the current virDomainObjList implementation, create a new module in utils called virObjectList and make it somehow generic that it could be used by our objects. I personally don't like the fact that there will be two new classes, one that enables using the other one.
https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
which moves all those driver/vir*obj list API's for Lookup, Search, ForEach, Add, Remove, etc. into object code since they're essentially all following the same pattern.
Once there - altering the Lockable lock under the covers should be relatively simple. I would be called a ListLock or HashLock instead of an RWLock and the implementation is such that it's R or W depending on API. Taking a quick refresher look at the series, for a new object I call virObjectLookupHashClass - that could be the integration point to use a local initialization for the class and then the appropriate lock style depending on API.
I think I still prefer "RWLock" name over "ListLock" or "HashLock" since its name clearly suggests its purpose. I mean, ListLock doesn't say it's an RW lock. Moreover, as I say in the cover letter, I'd like to use RW locks for virDomainObj one day (for instance, there's no reason why two clients cannot fetch XML for the same domain at the same time). Therefore, it looks correct to me to derive virDomainObjClass from virObjectRWLockable instead of ListLock or HashLock or something.
I agree that RWLock is more descriptive about what it is. And I also agree that deriving virDomainObjClass from virObjectRWLockable is better. As I've already wrote above, the generic listing code should work without a need to somehow modify the existing objects. Just a note, I like the idea that there will be only one implementation for listing object that will be reused, however the current proposed implementation isn't that convincing to me.
Just thinking off the cuff and of course trying to keep stuff I've been working on fresh ;-)
Sure, the more recent some patches are the more I understand them. Same here :-)
I think that this can be easily merged before the work for listing object gets finished since it can be used for object that doesn't require listing. Pavel
On 07/23/2017 07:21 PM, Pavel Hrdina wrote:
On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
On 07/23/2017 02:10 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement.
Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable
src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-)
This could be a "next step" in the work I've been doing towards a common object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
I'm still not sure about the implementation that you are heading to,
"you" as in John or as in me?
I would actually prefer something similar to the current virDomainObjList implementation, create a new module in utils called virObjectList and make it somehow generic that it could be used by our objects. I personally don't like the fact that there will be two new classes, one that enables using the other one.
Well I think we need two classes. A list of objects is something different than objects themselves. You can hardly assign some meaning to virDomainObj.lookupByName() or virDomainObjList.startCPUs(). It's important to differentiate "data an object is working with" and "operations an object can do". The fact that virDomainObjList works with virDomainObj doesn't mean it should be in any sense derived. In OOP, if class B is derived from class A, it means that B has all the attributes/methods that A has, plus something more, e.g. because B is more specialized. For instance, assume we have virCarClass. Then, virTruckClass or virV8TurboCharged3LClass can both be derived from virCarClass. But I can hardly imagine virParkingLotClass doing the same thing. Michal
On 07/23/2017 04:46 PM, Michal Privoznik wrote:
On 07/23/2017 07:21 PM, Pavel Hrdina wrote:
On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
On 07/23/2017 02:10 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement.
Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable
src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-)
This could be a "next step" in the work I've been doing towards a common object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
I'm still not sure about the implementation that you are heading to,
"you" as in John or as in me?
I'm assuming me. Still rather than discuss that here, respond to the cover letter of the other series with you thoughts and concerns. Having no feedback is far worse in my opinion. I really don't mind if someone else picks up some other pieces - that's absolutely fine by me. The consumers and higher counts of objects we have - the more stressed the current algorithms will get. This series starts down the path of altering the objlist locks to allow multiple readers which is good, IMO. I'm OK with the name RW, I would just prefer that it be lower in the stack and reused amongst all object types rather than specific to domainobjlist. As I've already determined, the domainobj code already has flaws with the object that should be fixed first. In particular, callers to virDomainObjListAdd have to decide whether to also virObjectRef the returned object or not based on how they use it and whether they use the virDomainObjEndAPI or (yuck) a direct virObjectUnlock. The ObjListAdd only incremented the Ref count once even though it placed the object in two tables. The corollary ObjListRemove will call virHashRemoveEntry twice - each decrementing the Ref count.
I would actually prefer something similar to the current virDomainObjList implementation, create a new module in utils called virObjectList and make it somehow generic that it could be used by our objects. I personally don't like the fact that there will be two new classes, one that enables using the other one.
Well I think we need two classes. A list of objects is something different than objects themselves. You can hardly assign some meaning to virDomainObj.lookupByName() or virDomainObjList.startCPUs(). It's important to differentiate "data an object is working with" and "operations an object can do". The fact that virDomainObjList works with virDomainObj doesn't mean it should be in any sense derived. In OOP, if class B is derived from class A, it means that B has all the attributes/methods that A has, plus something more, e.g. because B is more specialized. For instance, assume we have virCarClass. Then, virTruckClass or virV8TurboCharged3LClass can both be derived from virCarClass. But I can hardly imagine virParkingLotClass doing the same thing.
Michal
Trying to consider the ramifications of virDomainObjList.startCPUs() - for every (active) domain, start all the CPU's... Wonder how far that will get for a Host with 100 domains using 8 vCPU's each ;-). We may want to think we're OOP, but we're not. If we were it'd be obj.Ref(), obj.Unref(), obj.Lock(), obj.Unlock(), etc. Instead we have virObjectRef(obj), virObjectUnref(obj), virObjectLock(obj), virObjectUnlock(obj). Whether as I've in the other series the code is in virobject.c or it's in some new virobjhashtable.c (or something, IDC) doesn't really matter. I kept it in virobject because that was what was working on the object currently. If we really want to become more OOP like then that's a very different discussion. I think you blur the lines with how ObjList and Obj's are used by us. It's not like any of those virObject* API's are being created for some external general libvirt provided API can use directly. They are specific to the needs of the various drivers/objects. ObjList isn't derived from Obj, but it consumes Obj's for the purposes of API's to be able to add, query, remove. There's a close, familial relationship between the two. John On the highways around here, virParkingLogClass occurs every day from 330P to about 630P. It's even worse when virTruckClass collides with virV8TurboCharged3LClass - that means your virCommuteObject.time() is extended and your virDriverObject.bloodPressure() gets higher. ;-)
On 07/23/2017 11:33 PM, John Ferlan wrote:
On 07/23/2017 04:46 PM, Michal Privoznik wrote:
On 07/23/2017 07:21 PM, Pavel Hrdina wrote:
On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
On 07/23/2017 02:10 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement.
Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable
src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-)
This could be a "next step" in the work I've been doing towards a common object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
I'm still not sure about the implementation that you are heading to,
"you" as in John or as in me?
I'm assuming me. Still rather than discuss that here, respond to the cover letter of the other series with you thoughts and concerns. Having no feedback is far worse in my opinion. I really don't mind if someone else picks up some other pieces - that's absolutely fine by me. The consumers and higher counts of objects we have - the more stressed the current algorithms will get.
This series starts down the path of altering the objlist locks to allow multiple readers which is good, IMO. I'm OK with the name RW, I would just prefer that it be lower in the stack and reused amongst all object types rather than specific to domainobjlist.
Sure. I should have said that out loud - if this gets merged I can copy it to other vir*ObjLists. Hopefully that doesn't cause problems on your side.
As I've already determined, the domainobj code already has flaws with the object that should be fixed first. In particular, callers to virDomainObjListAdd have to decide whether to also virObjectRef the returned object or not based on how they use it and whether they use the virDomainObjEndAPI or (yuck) a direct virObjectUnlock.
Yep. I think we've discussed this (or maybe it was some different type, like nwfilters? doesn't matter really). I recall me saying we should go with the former. That is, vir*ObjListAdd should always return refed & locked object. The reasoning is that *every* API which works with the object should take a reference instead of relying on the one in the list. As a nice result, every API can then just use vir*ObjEndAPI.
The ObjListAdd only incremented the Ref count once even though it placed the object in two tables. The corollary ObjListRemove will call virHashRemoveEntry twice - each decrementing the Ref count.
I would actually prefer something similar to the current virDomainObjList implementation, create a new module in utils called virObjectList and make it somehow generic that it could be used by our objects. I personally don't like the fact that there will be two new classes, one that enables using the other one.
Well I think we need two classes. A list of objects is something different than objects themselves. You can hardly assign some meaning to virDomainObj.lookupByName() or virDomainObjList.startCPUs(). It's important to differentiate "data an object is working with" and "operations an object can do". The fact that virDomainObjList works with virDomainObj doesn't mean it should be in any sense derived. In OOP, if class B is derived from class A, it means that B has all the attributes/methods that A has, plus something more, e.g. because B is more specialized. For instance, assume we have virCarClass. Then, virTruckClass or virV8TurboCharged3LClass can both be derived from virCarClass. But I can hardly imagine virParkingLotClass doing the same thing.
Michal
Trying to consider the ramifications of virDomainObjList.startCPUs() - for every (active) domain, start all the CPU's... Wonder how far that will get for a Host with 100 domains using 8 vCPU's each ;-).
Yeah well. We don't have an API that works over multiple domains. But it might sure be interesting :-)
We may want to think we're OOP, but we're not. If we were it'd be obj.Ref(), obj.Unref(), obj.Lock(), obj.Unlock(), etc. Instead we have virObjectRef(obj), virObjectUnref(obj), virObjectLock(obj), virObjectUnlock(obj).
I think it's just a matter of syntax whether I write obj.method(); or method(obj). The real OOP-ism lies in having compiler taking care of ref/unref.
Whether as I've in the other series the code is in virobject.c or it's in some new virobjhashtable.c (or something, IDC) doesn't really matter. I kept it in virobject because that was what was working on the object currently. If we really want to become more OOP like then that's a very different discussion.
No, that's not what I'm calling for.
I think you blur the lines with how ObjList and Obj's are used by us. It's not like any of those virObject* API's are being created for some external general libvirt provided API can use directly. They are specific to the needs of the various drivers/objects. ObjList isn't derived from Obj, but it consumes Obj's for the purposes of API's to be able to add, query, remove. There's a close, familial relationship between the two.
Well, sure. ObjList is for us storing Objs. But I view ObjList as unaware of what it's storing. Just like our hash tables. For them everything's void *. And it's only because we use ObjList to actually store virObject that it can call 'virObjectRef(vm)' (where vm is say virDomainObj) before actually returning it. Otherwise ObjList is blind (or at least should be) to what it's storing.
John
On the highways around here, virParkingLogClass occurs every day from 330P to about 630P. It's even worse when virTruckClass collides with virV8TurboCharged3LClass - that means your virCommuteObject.time() is extended and your virDriverObject.bloodPressure() gets higher. ;-)
Yep. Same here. Thank God for navigation SW that collects data from other users and re-route to avoid traffic jams (if possible). Michal
On 07/24/2017 09:04 AM, Michal Privoznik wrote:
On 07/23/2017 11:33 PM, John Ferlan wrote:
On 07/23/2017 04:46 PM, Michal Privoznik wrote:
On 07/23/2017 07:21 PM, Pavel Hrdina wrote:
On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
On 07/23/2017 02:10 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote: > While this is not that critical (hash tables have O(1) time complexity for > lookups), it lays down path towards making virDomainObj using RW locks instead > of mutexes. Still, in my limited testing this showed slight improvement. > > Michal Privoznik (3): > virthread: Introduce virRWLockInitPreferWriter > virobject: Introduce virObjectRWLockable > virdomainobjlist: Use virObjectRWLockable > > src/conf/virdomainobjlist.c | 24 ++++---- > src/libvirt_private.syms | 4 ++ > src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- > src/util/virobject.h | 16 +++++ > src/util/virthread.c | 35 +++++++++++ > src/util/virthread.h | 1 + > 6 files changed, 180 insertions(+), 44 deletions(-) >
This could be a "next step" in the work I've been doing towards a common object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
I'm still not sure about the implementation that you are heading to,
"you" as in John or as in me?
I'm assuming me. Still rather than discuss that here, respond to the cover letter of the other series with you thoughts and concerns. Having no feedback is far worse in my opinion. I really don't mind if someone else picks up some other pieces - that's absolutely fine by me. The consumers and higher counts of objects we have - the more stressed the current algorithms will get.
This series starts down the path of altering the objlist locks to allow multiple readers which is good, IMO. I'm OK with the name RW, I would just prefer that it be lower in the stack and reused amongst all object types rather than specific to domainobjlist.
Sure. I should have said that out loud - if this gets merged I can copy it to other vir*ObjLists. Hopefully that doesn't cause problems on your side.
Well I have no virdomainobjlist patches in my trees, but changes to virobject.{c,h} will have 'conflicts'. Of course I see you've already pushed so whatever - I'll have to deal with it now in my branches and will have to put together a new version of what I posted last month <sigh>.
As I've already determined, the domainobj code already has flaws with the object that should be fixed first. In particular, callers to virDomainObjListAdd have to decide whether to also virObjectRef the returned object or not based on how they use it and whether they use the virDomainObjEndAPI or (yuck) a direct virObjectUnlock.
Yep. I think we've discussed this (or maybe it was some different type, like nwfilters? doesn't matter really). I recall me saying we should go with the former. That is, vir*ObjListAdd should always return refed & locked object. The reasoning is that *every* API which works with the object should take a reference instead of relying on the one in the list. As a nice result, every API can then just use vir*ObjEndAPI.
Yes - I think it was during the nwfilter review.
The ObjListAdd only incremented the Ref count once even though it placed the object in two tables. The corollary ObjListRemove will call virHashRemoveEntry twice - each decrementing the Ref count.
I would actually prefer something similar to the current virDomainObjList implementation, create a new module in utils called virObjectList and make it somehow generic that it could be used by our objects. I personally don't like the fact that there will be two new classes, one that enables using the other one.
[1]
Well I think we need two classes. A list of objects is something different than objects themselves. You can hardly assign some meaning to virDomainObj.lookupByName() or virDomainObjList.startCPUs(). It's important to differentiate "data an object is working with" and "operations an object can do". The fact that virDomainObjList works with virDomainObj doesn't mean it should be in any sense derived. In OOP, if class B is derived from class A, it means that B has all the attributes/methods that A has, plus something more, e.g. because B is more specialized. For instance, assume we have virCarClass. Then, virTruckClass or virV8TurboCharged3LClass can both be derived from virCarClass. But I can hardly imagine virParkingLotClass doing the same thing.
Michal
Trying to consider the ramifications of virDomainObjList.startCPUs() - for every (active) domain, start all the CPU's... Wonder how far that will get for a Host with 100 domains using 8 vCPU's each ;-).
Yeah well. We don't have an API that works over multiple domains. But it might sure be interesting :-)
We may want to think we're OOP, but we're not. If we were it'd be obj.Ref(), obj.Unref(), obj.Lock(), obj.Unlock(), etc. Instead we have virObjectRef(obj), virObjectUnref(obj), virObjectLock(obj), virObjectUnlock(obj).
I think it's just a matter of syntax whether I write obj.method(); or method(obj). The real OOP-ism lies in having compiler taking care of ref/unref.
This was one of those points that was on the gray area between this series and the comments about the other series. Calling out usage of obj.FUNCTION() vs. FUNCTION(obj) was just a way to point out that we're really not OOP in the grand scheme of things. We use API names to represent more what a function does and try to group together code as best as possible with the eye towards similar functionality being together. [1] Since we've already gone down the path of having one module for virObjectClass from which virObjectLockable builds off it and is used by both *Obj and *ObjList, then why create a virobjectlist.c (or virobjecthash.c) in order to contain API's for list/hash mgmt. This (now pushed) patch series creates the precedence that Object *ObjList mgmt functions can stay in virobject.c, which works better for me.
Whether as I've in the other series the code is in virobject.c or it's in some new virobjhashtable.c (or something, IDC) doesn't really matter. I kept it in virobject because that was what was working on the object currently. If we really want to become more OOP like then that's a very different discussion.
No, that's not what I'm calling for.
I think you blur the lines with how ObjList and Obj's are used by us. It's not like any of those virObject* API's are being created for some external general libvirt provided API can use directly. They are specific to the needs of the various drivers/objects. ObjList isn't derived from Obj, but it consumes Obj's for the purposes of API's to be able to add, query, remove. There's a close, familial relationship between the two.
Well, sure. ObjList is for us storing Objs. But I view ObjList as unaware of what it's storing. Just like our hash tables. For them
EXACTLY! This is why I started down this path... Of course I want far too generic for some people's preferences by going with primaryKey and secondaryKey type nomenclature, so I was forced by review to use UUID/Name which are far less generic, but "tie" together the various vir*obj.c consumers which is fine although would seemingly go against the premise that ObjList shouldn't be aware of what it's storing. It should only care that it's using a some Key rather than a named Key. I also went with ObjHash since that's what it represents moreso than an ObjList. I suppose we could have both, but who would use a forward linked list for searching when hash table lookups are around? The one value for an ObjList I could see would be ordered operations - e.g. as a way to use as a queue of sorts to append on a tail operation while some other thread pulls off the head. But for storing persistent objects, hash is a better option. I still think the "RW" part should have been hidden by a new object that is to be primarily used for list/hash objects... What's to stop someone from thinking that virObjectRWLockable could be used for a vir*Obj object that goes into a list (or hash table)? I also have some specific comments for patch 2 which I'll leave there as a response... John
everything's void *. And it's only because we use ObjList to actually store virObject that it can call 'virObjectRef(vm)' (where vm is say virDomainObj) before actually returning it. Otherwise ObjList is blind (or at least should be) to what it's storing.
John
On the highways around here, virParkingLogClass occurs every day from 330P to about 630P. It's even worse when virTruckClass collides with virV8TurboCharged3LClass - that means your virCommuteObject.time() is extended and your virDriverObject.bloodPressure() gets higher. ;-)
Yep. Same here. Thank God for navigation SW that collects data from other users and re-route to avoid traffic jams (if possible).
Michal
On Mon, Jul 24, 2017 at 11:12:01AM -0400, John Ferlan wrote:
On 07/24/2017 09:04 AM, Michal Privoznik wrote:
On 07/23/2017 11:33 PM, John Ferlan wrote:
On 07/23/2017 04:46 PM, Michal Privoznik wrote:
On 07/23/2017 07:21 PM, Pavel Hrdina wrote:
On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
On 07/23/2017 02:10 PM, John Ferlan wrote: > > > On 07/19/2017 10:31 AM, Michal Privoznik wrote: >> While this is not that critical (hash tables have O(1) time complexity for >> lookups), it lays down path towards making virDomainObj using RW locks instead >> of mutexes. Still, in my limited testing this showed slight improvement. >> >> Michal Privoznik (3): >> virthread: Introduce virRWLockInitPreferWriter >> virobject: Introduce virObjectRWLockable >> virdomainobjlist: Use virObjectRWLockable >> >> src/conf/virdomainobjlist.c | 24 ++++---- >> src/libvirt_private.syms | 4 ++ >> src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- >> src/util/virobject.h | 16 +++++ >> src/util/virthread.c | 35 +++++++++++ >> src/util/virthread.h | 1 + >> 6 files changed, 180 insertions(+), 44 deletions(-) >> > > This could be a "next step" in the work I've been doing towards a common > object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
I'm still not sure about the implementation that you are heading to,
"you" as in John or as in me?
I'm assuming me. Still rather than discuss that here, respond to the cover letter of the other series with you thoughts and concerns. Having no feedback is far worse in my opinion. I really don't mind if someone else picks up some other pieces - that's absolutely fine by me. The consumers and higher counts of objects we have - the more stressed the current algorithms will get.
This series starts down the path of altering the objlist locks to allow multiple readers which is good, IMO. I'm OK with the name RW, I would just prefer that it be lower in the stack and reused amongst all object types rather than specific to domainobjlist.
Sure. I should have said that out loud - if this gets merged I can copy it to other vir*ObjLists. Hopefully that doesn't cause problems on your side.
Well I have no virdomainobjlist patches in my trees, but changes to virobject.{c,h} will have 'conflicts'. Of course I see you've already pushed so whatever - I'll have to deal with it now in my branches and will have to put together a new version of what I posted last month <sigh>.
As I've already determined, the domainobj code already has flaws with the object that should be fixed first. In particular, callers to virDomainObjListAdd have to decide whether to also virObjectRef the returned object or not based on how they use it and whether they use the virDomainObjEndAPI or (yuck) a direct virObjectUnlock.
Yep. I think we've discussed this (or maybe it was some different type, like nwfilters? doesn't matter really). I recall me saying we should go with the former. That is, vir*ObjListAdd should always return refed & locked object. The reasoning is that *every* API which works with the object should take a reference instead of relying on the one in the list. As a nice result, every API can then just use vir*ObjEndAPI.
Yes - I think it was during the nwfilter review.
The ObjListAdd only incremented the Ref count once even though it placed the object in two tables. The corollary ObjListRemove will call virHashRemoveEntry twice - each decrementing the Ref count.
I would actually prefer something similar to the current virDomainObjList implementation, create a new module in utils called virObjectList and make it somehow generic that it could be used by our objects. I personally don't like the fact that there will be two new classes, one that enables using the other one.
[1]
Well I think we need two classes. A list of objects is something different than objects themselves. You can hardly assign some meaning to virDomainObj.lookupByName() or virDomainObjList.startCPUs(). It's important to differentiate "data an object is working with" and "operations an object can do". The fact that virDomainObjList works with virDomainObj doesn't mean it should be in any sense derived. In OOP, if class B is derived from class A, it means that B has all the attributes/methods that A has, plus something more, e.g. because B is more specialized. For instance, assume we have virCarClass. Then, virTruckClass or virV8TurboCharged3LClass can both be derived from virCarClass. But I can hardly imagine virParkingLotClass doing the same thing.
Michal
Trying to consider the ramifications of virDomainObjList.startCPUs() - for every (active) domain, start all the CPU's... Wonder how far that will get for a Host with 100 domains using 8 vCPU's each ;-).
Yeah well. We don't have an API that works over multiple domains. But it might sure be interesting :-)
We may want to think we're OOP, but we're not. If we were it'd be obj.Ref(), obj.Unref(), obj.Lock(), obj.Unlock(), etc. Instead we have virObjectRef(obj), virObjectUnref(obj), virObjectLock(obj), virObjectUnlock(obj).
I think it's just a matter of syntax whether I write obj.method(); or method(obj). The real OOP-ism lies in having compiler taking care of ref/unref.
This was one of those points that was on the gray area between this series and the comments about the other series. Calling out usage of obj.FUNCTION() vs. FUNCTION(obj) was just a way to point out that we're really not OOP in the grand scheme of things. We use API names to represent more what a function does and try to group together code as best as possible with the eye towards similar functionality being together.
[1] Since we've already gone down the path of having one module for virObjectClass from which virObjectLockable builds off it and is used by both *Obj and *ObjList, then why create a virobjectlist.c (or virobjecthash.c) in order to contain API's for list/hash mgmt. This (now pushed) patch series creates the precedence that Object *ObjList mgmt functions can stay in virobject.c, which works better for me.
No, there is a difference, the virObject and virObjectLockable and virObjectRWLockable are generic objects that are newer used directly and are used only as parents for other child objects like virDomainObj. The virHashTable isn't even derived from virObject, it only operates with virObject. The same way virObjectList should be implemented.
Whether as I've in the other series the code is in virobject.c or it's in some new virobjhashtable.c (or something, IDC) doesn't really matter. I kept it in virobject because that was what was working on the object currently. If we really want to become more OOP like then that's a very different discussion.
No, that's not what I'm calling for.
I think you blur the lines with how ObjList and Obj's are used by us. It's not like any of those virObject* API's are being created for some external general libvirt provided API can use directly. They are specific to the needs of the various drivers/objects. ObjList isn't derived from Obj, but it consumes Obj's for the purposes of API's to be able to add, query, remove. There's a close, familial relationship between the two.
Well, sure. ObjList is for us storing Objs. But I view ObjList as unaware of what it's storing. Just like our hash tables. For them
EXACTLY! This is why I started down this path... Of course I want far too generic for some people's preferences by going with primaryKey and secondaryKey type nomenclature, so I was forced by review to use UUID/Name which are far less generic, but "tie" together the various vir*obj.c consumers which is fine although would seemingly go against the premise that ObjList shouldn't be aware of what it's storing. It should only care that it's using a some Key rather than a named Key.
You are saying exactly and yet you are introducing new virObjectLookupKeys which is used instead of virObjectLockable just to make the new listing virObjectLookupHash to work and that's wrong. It should work even with virObject. Pavel
I also went with ObjHash since that's what it represents moreso than an ObjList. I suppose we could have both, but who would use a forward linked list for searching when hash table lookups are around? The one value for an ObjList I could see would be ordered operations - e.g. as a way to use as a queue of sorts to append on a tail operation while some other thread pulls off the head. But for storing persistent objects, hash is a better option.
I still think the "RW" part should have been hidden by a new object that is to be primarily used for list/hash objects... What's to stop someone from thinking that virObjectRWLockable could be used for a vir*Obj object that goes into a list (or hash table)?
I also have some specific comments for patch 2 which I'll leave there as a response...
John
everything's void *. And it's only because we use ObjList to actually store virObject that it can call 'virObjectRef(vm)' (where vm is say virDomainObj) before actually returning it. Otherwise ObjList is blind (or at least should be) to what it's storing.
John
On the highways around here, virParkingLogClass occurs every day from 330P to about 630P. It's even worse when virTruckClass collides with virV8TurboCharged3LClass - that means your virCommuteObject.time() is extended and your virDriverObject.bloodPressure() gets higher. ;-)
Yep. Same here. Thank God for navigation SW that collects data from other users and re-route to avoid traffic jams (if possible).
Michal
On 07/24/2017 02:33 PM, Pavel Hrdina wrote:
EXACTLY! This is why I started down this path... Of course I want far too generic for some people's preferences by going with primaryKey and secondaryKey type nomenclature, so I was forced by review to use UUID/Name which are far less generic, but "tie" together the various vir*obj.c consumers which is fine although would seemingly go against the premise that ObjList shouldn't be aware of what it's storing. It should only care that it's using a some Key rather than a named Key.
You are saying exactly and yet you are introducing new virObjectLookupKeys which is used instead of virObjectLockable just to make the new listing virObjectLookupHash to work and that's wrong. It should work even with virObject.
Pavel
This really isn't the right place for a discussion about the other series - the merits of that solution should be discussed there... Still, I must be missing something. Why is it wrong to create a new object that would have a specific use? virObjectLockable was created at some point in history and then used as a way to provide locking for a virObject that only had a @ref. Someone could still create a virObject as well and just get @refs. With the new objects based on those two objects you get a few more features that allow for add, search, lookup, remove. But you call that wrong, which is confusing to me. It doesn't make much sense to have a hash table with objects that have no way of being looked up does it? How do you add the object? Even the virnode* uses a hash table that has objects that have UUID and Name used for lookup. A virObjectLockable has a @ref and @lock - there's *nothing* in there that can be used for a key for a hash table. So what in your opinion would be the use of an object in a table that cannot be fetched. So "something" has to create an object that uses the existing virObjectLockable as a base and allows for it to be build upon in order to be more useful. A new object class needs to be created and used. If something still wants to use virObjectLockable and build their own who knows what in order to manage the virObjectLockable's they still can. The virObjectLockable isn't going away, it's being extended. After initial series way back in February: https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html I was encouraged to take a more object oriented view, thus the follow up RFC in April: https://www.redhat.com/archives/libvir-list/2017-April/msg00321.html which got no feedback, so in late May it's: https://www.redhat.com/archives/libvir-list/2017-May/msg01178.html which got some feedback and a quick v2 in early June: https://www.redhat.com/archives/libvir-list/2017-June/msg00070.html that got more feedback, but mainly that the generic primaryKey and secondaryKey were not favored. So, v3 was generated in mid June that was less abstract: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html which again gets zero feedback, but apparently has been read. Still no where in any of this work has it been said using these types of objects was wrong. John
On Sun, Jul 23, 2017 at 10:46:15PM +0200, Michal Privoznik wrote:
On 07/23/2017 07:21 PM, Pavel Hrdina wrote:
On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
On 07/23/2017 02:10 PM, John Ferlan wrote:
On 07/19/2017 10:31 AM, Michal Privoznik wrote:
While this is not that critical (hash tables have O(1) time complexity for lookups), it lays down path towards making virDomainObj using RW locks instead of mutexes. Still, in my limited testing this showed slight improvement.
Michal Privoznik (3): virthread: Introduce virRWLockInitPreferWriter virobject: Introduce virObjectRWLockable virdomainobjlist: Use virObjectRWLockable
src/conf/virdomainobjlist.c | 24 ++++---- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 144 ++++++++++++++++++++++++++++++++++---------- src/util/virobject.h | 16 +++++ src/util/virthread.c | 35 +++++++++++ src/util/virthread.h | 1 + 6 files changed, 180 insertions(+), 44 deletions(-)
This could be a "next step" in the work I've been doing towards a common object:
Sure. If we have just one common object the change can be done in one place instead of many. I don't care in what order are the changes merged.
I'm still not sure about the implementation that you are heading to,
"you" as in John or as in me?
Oh right, I should be exact, John.
I would actually prefer something similar to the current virDomainObjList implementation, create a new module in utils called virObjectList and make it somehow generic that it could be used by our objects. I personally don't like the fact that there will be two new classes, one that enables using the other one.
Well I think we need two classes. A list of objects is something different than objects themselves. You can hardly assign some meaning to virDomainObj.lookupByName() or virDomainObjList.startCPUs(). It's important to differentiate "data an object is working with" and "operations an object can do". The fact that virDomainObjList works with virDomainObj doesn't mean it should be in any sense derived. In OOP, if class B is derived from class A, it means that B has all the attributes/methods that A has, plus something more, e.g. because B is more specialized. For instance, assume we have virCarClass. Then, virTruckClass or virV8TurboCharged3LClass can both be derived from virCarClass. But I can hardly imagine virParkingLotClass doing the same thing.
Michal
Well, I know how all this work. What I meant is that virDomainObj would be still derived from virObjectLockable or virObjectRWLockable and there would be virObjectList class which would implement the lookup functions, addObj, removeObj, and so on. You would create a new instance of virObjectList class and fill that instance with domain objects that you need to list. The domain object itself doesn't have to know anything about the virObjectList class. To use the similar explanation, you have virObjectClass (virObjectLockableClass) and you have virCarClass (virDomainObjClass) and virTruckClass (virStoragePoolClass) and there would be virParkingClass (virObjectListClass). The virCarClass is not derived from virVehicleParkableClass (virObjectLookupKeys) in order to have virParkingClass (virObjectLookupHash) to allow virCarClass to park. That's what I don't like about the current implementation. Pavel
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Michal Privoznik -
Pavel Hrdina