9:31 a.m.
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
9:31 a.m.
New subject: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
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(a)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
12:25 p.m.
New subject: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
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(a)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(a)redhat.com>
12:12 p.m.
New subject: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
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(a)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);
2:45 a.m.
New subject: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
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(a)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
3:23 a.m.
New subject: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
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(a)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 :|
9:31 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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(a)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
8:13 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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(a)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(a)redhat.com>
10:22 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
[...]
/**
* 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)");
+ }
}
[...]
2:45 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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
7:13 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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
9:25 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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
9:32 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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
10:47 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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
11:26 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
> 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
2:33 a.m.
New subject: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
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
9:31 a.m.
New subject: [libvirt] [PATCH 3/3] virdomainobjlist: Use virObjectRWLockable
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(a)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
8:36 a.m.
New subject: [libvirt] [PATCH 3/3] virdomainobjlist: Use virObjectRWLockable
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(a)redhat.com>
---
src/conf/virdomainobjlist.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
8:56 a.m.
New subject: [libvirt] [PATCH 3/3] virdomainobjlist: Use virObjectRWLockable
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(a)redhat.com>
> ---
> src/conf/virdomainobjlist.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
Sweet. I've fixed 2/3 and pushed. Thank you!
Michal
7:10 a.m.
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
7:33 a.m.
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
12:21 p.m.
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
3:46 p.m.
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
4:33 p.m.
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. ;-)
8:04 a.m.
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
10:12 a.m.
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
1:33 p.m.
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
>
7:35 p.m.
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
4:39 p.m.
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
2676
days inactive
2683
days old
28 comments
4 participants
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Michal Privoznik -
Pavel Hrdina