Up until now we only had virObjectLockable which uses mutexes for
mutually excluding each other in critical section. Well, this is
not enough. Future work will require RW locks so we might as well
have virObjectRWLockable which is introduced here.

Moreover, polymorphism is introduced to our code for the first
time. Yay! More specifically, virObjectLock will grab a write
lock, virObjectLockRead will grab a read lock then (what a
surprise right?). This has great advantage that an object can be
made derived from virObjectRWLockable in a single line and still
continue functioning properly (mutexes can be viewed as grabbing
write locks only). Then just those critical sections that can
grab a read lock need fixing. Therefore the resulting change is
going to be way smaller.

In order to avoid writer starvation, the object initializes RW
lock that prefers writers.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/libvirt_private.syms |   3 +
 src/util/virobject.c     | 144 ++++++++++++++++++++++++++++++++++++-----------
 src/util/virobject.h     |  16 ++++++
 3 files changed, 131 insertions(+), 32 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a792e00c8..f9df35583 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2282,6 +2282,7 @@ virNumaSetupMemoryPolicy;
 # util/virobject.h
 virClassForObject;
 virClassForObjectLockable;
+virClassForObjectRWLockable;
 virClassIsDerivedFrom;
 virClassName;
 virClassNew;
@@ -2292,8 +2293,10 @@ virObjectListFree;
 virObjectListFreeCount;
 virObjectLock;
 virObjectLockableNew;
+virObjectLockRead;
 virObjectNew;
 virObjectRef;
+virObjectRWLockableNew;
 virObjectUnlock;
 virObjectUnref;
 
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 34805d34a..3e7a0719e 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -49,8 +49,10 @@ struct _virClass {
 
 static virClassPtr virObjectClass;
 static virClassPtr virObjectLockableClass;
+static virClassPtr virObjectRWLockableClass;
 
 static void virObjectLockableDispose(void *anyobj);
+static void virObjectRWLockableDispose(void *anyobj);
 
 static int
 virObjectOnceInit(void)
@@ -67,6 +69,12 @@ virObjectOnceInit(void)
                                                virObjectLockableDispose)))
         return -1;
 
+    if (!(virObjectRWLockableClass = virClassNew(virObjectClass,
+                                                 "virObjectRWLockable",
+                                                 sizeof(virObjectRWLockable),
+                                                 virObjectRWLockableDispose)))
+        return -1;
+
     return 0;
 }
 
@@ -103,6 +111,20 @@ virClassForObjectLockable(void)
 }
 
 
+/**
+ * virClassForObjectRWLockable:
+ *
+ * Returns the class instance for the virObjectRWLockable type
+ */
+virClassPtr
+virClassForObjectRWLockable(void)
+{
+    if (virObjectInitialize() < 0)
+        return NULL;
+
+    return virObjectRWLockableClass;
+}
+
 /**
  * virClassNew:
  * @parent: the parent class
@@ -237,6 +259,32 @@ virObjectLockableNew(virClassPtr klass)
 }
 
 
+void *
+virObjectRWLockableNew(virClassPtr klass)
+{
+    virObjectRWLockablePtr obj;
+
+    if (!virClassIsDerivedFrom(klass, virClassForObjectRWLockable())) {
+        virReportInvalidArg(klass,
+                            _("Class %s must derive from virObjectRWLockable"),
+                            virClassName(klass));
+        return NULL;
+    }
+
+    if (!(obj = virObjectNew(klass)))
+        return NULL;
+
+    if (virRWLockInitPreferWriter(&obj->lock) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Unable to initialize RW lock"));
+        virObjectUnref(obj);
+        return NULL;
+    }
+
+    return obj;
+}
+
+
 static void
 virObjectLockableDispose(void *anyobj)
 {
@@ -246,6 +294,15 @@ virObjectLockableDispose(void *anyobj)
 }
 
 
+static void
+virObjectRWLockableDispose(void *anyobj)
+{
+    virObjectRWLockablePtr obj = anyobj;
+
+    virRWLockDestroy(&obj->lock);
+}
+
+
 /**
  * virObjectUnref:
  * @anyobj: any instance of virObjectPtr
@@ -309,28 +366,13 @@ virObjectRef(void *anyobj)
 }
 
 
-static virObjectLockablePtr
-virObjectGetLockableObj(void *anyobj)
-{
-    virObjectPtr obj;
-
-    if (virObjectIsClass(anyobj, virObjectLockableClass))
-        return anyobj;
-
-    obj = anyobj;
-    VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
-              anyobj, obj ? obj->klass->name : "(unknown)");
-
-    return NULL;
-}
-
-
 /**
  * virObjectLock:
- * @anyobj: any instance of virObjectLockablePtr
+ * @anyobj: any instance of virObjectLockable or virObjectRWLockable
  *
- * Acquire a lock on @anyobj. The lock must be
- * released by virObjectUnlock.
+ * Acquire a lock on @anyobj. The lock must be released by
+ * virObjectUnlock. In case the passed object is instance of
+ * virObjectRWLockable a write lock is acquired.
  *
  * The caller is expected to have acquired a reference
  * on the object before locking it (eg virObjectRef).
@@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
 void
 virObjectLock(void *anyobj)
 {
-    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+    if (virObjectIsClass(anyobj, virObjectLockableClass)) {
+        virObjectLockablePtr obj = anyobj;
+        virMutexLock(&obj->lock);
+    } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+        virObjectRWLockablePtr obj = anyobj;
+        virRWLockWrite(&obj->lock);
+    } else {
+        virObjectPtr obj = anyobj;
+        VIR_WARN("Object %p (%s) is not a virObjectLockable "
+                 "nor virObjectRWLockable instance",
+                 anyobj, obj ? obj->klass->name : "(unknown)");
+    }
+}
 
-    if (!obj)
-        return;
 
-    virMutexLock(&obj->lock);
+/**
+ * virObjectLockRead:
+ * @anyobj: any instance of virObjectRWLockablePtr
+ *
+ * Acquire a read lock on @anyobj. The lock must be
+ * released by virObjectUnlock.
+ *
+ * The caller is expected to have acquired a reference
+ * on the object before locking it (eg virObjectRef).
+ * The object must be unlocked before releasing this
+ * reference.
+ */
+void
+virObjectLockRead(void *anyobj)
+{
+    if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+        virObjectRWLockablePtr obj = anyobj;
+        virRWLockRead(&obj->lock);
+    } else {
+        virObjectPtr obj = anyobj;
+        VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+                 anyobj, obj ? obj->klass->name : "(unknown)");
+    }
 }
 
 
 /**
  * virObjectUnlock:
- * @anyobj: any instance of virObjectLockablePtr
+ * @anyobj: any instance of virObjectLockable or virObjectRWLockable
  *
- * Release a lock on @anyobj. The lock must have been
- * acquired by virObjectLock.
+ * Release a lock on @anyobj. The lock must have been acquired by
+ * virObjectLock or virObjectLockRead.
  */
 void
 virObjectUnlock(void *anyobj)
 {
-    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
-
-    if (!obj)
-        return;
-
-    virMutexUnlock(&obj->lock);
+    if (virObjectIsClass(anyobj, virObjectLockableClass)) {
+        virObjectLockablePtr obj = anyobj;
+        virMutexUnlock(&obj->lock);
+    } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+        virObjectRWLockablePtr obj = anyobj;
+        virRWLockUnlock(&obj->lock);
+    } else {
+        virObjectPtr obj = anyobj;
+        VIR_WARN("Object %p (%s) is not a virObjectLockable "
+                 "nor virObjectRWLockable instance",
+                 anyobj, obj ? obj->klass->name : "(unknown)");
+    }
 }
 
 
diff --git a/src/util/virobject.h b/src/util/virobject.h
index f4c292b16..5985fadb2 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -34,6 +34,9 @@ typedef virObject *virObjectPtr;
 typedef struct _virObjectLockable virObjectLockable;
 typedef virObjectLockable *virObjectLockablePtr;
 
+typedef struct _virObjectRWLockable virObjectRWLockable;
+typedef virObjectRWLockable *virObjectRWLockablePtr;
+
 typedef void (*virObjectDisposeCallback)(void *obj);
 
 /* Most code should not play with the contents of this struct; however,
@@ -59,9 +62,14 @@ struct _virObjectLockable {
     virMutex lock;
 };
 
+struct _virObjectRWLockable {
+    virObject parent;
+    virRWLock lock;
+};
 
 virClassPtr virClassForObject(void);
 virClassPtr virClassForObjectLockable(void);
+virClassPtr virClassForObjectRWLockable(void);
 
 # ifndef VIR_PARENT_REQUIRED
 #  define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1)
@@ -108,10 +116,18 @@ void *
 virObjectLockableNew(virClassPtr klass)
     ATTRIBUTE_NONNULL(1);
 
+void *
+virObjectRWLockableNew(virClassPtr klass)
+    ATTRIBUTE_NONNULL(1);
+
 void
 virObjectLock(void *lockableobj)
     ATTRIBUTE_NONNULL(1);
 
+void
+virObjectLockRead(void *lockableobj)
+    ATTRIBUTE_NONNULL(1);
+
 void
 virObjectUnlock(void *lockableobj)
     ATTRIBUTE_NONNULL(1);
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Jul 19, 2017 at 04:31:49PM +0200, Michal Privoznik wrote:
> Up until now we only had virObjectLockable which uses mutexes for
> mutually excluding each other in critical section. Well, this is
> not enough. Future work will require RW locks so we might as well
> have virObjectRWLockable which is introduced here.
> 
> Moreover, polymorphism is introduced to our code for the first
> time. Yay! More specifically, virObjectLock will grab a write
> lock, virObjectLockRead will grab a read lock then (what a
> surprise right?). This has great advantage that an object can be
> made derived from virObjectRWLockable in a single line and still
> continue functioning properly (mutexes can be viewed as grabbing
> write locks only). Then just those critical sections that can
> grab a read lock need fixing. Therefore the resulting change is
> going to be way smaller.
> 
> In order to avoid writer starvation, the object initializes RW
> lock that prefers writers.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/libvirt_private.syms |   3 +
>  src/util/virobject.c     | 144 ++++++++++++++++++++++++++++++++++++-----------
>  src/util/virobject.h     |  16 ++++++
>  3 files changed, 131 insertions(+), 32 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a792e00c8..f9df35583 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2282,6 +2282,7 @@ virNumaSetupMemoryPolicy;
>  # util/virobject.h
>  virClassForObject;
>  virClassForObjectLockable;
> +virClassForObjectRWLockable;
>  virClassIsDerivedFrom;
>  virClassName;
>  virClassNew;
> @@ -2292,8 +2293,10 @@ virObjectListFree;
>  virObjectListFreeCount;
>  virObjectLock;
>  virObjectLockableNew;
> +virObjectLockRead;
>  virObjectNew;
>  virObjectRef;
> +virObjectRWLockableNew;
>  virObjectUnlock;
>  virObjectUnref;
>  
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 34805d34a..3e7a0719e 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -49,8 +49,10 @@ struct _virClass {
>  
>  static virClassPtr virObjectClass;
>  static virClassPtr virObjectLockableClass;
> +static virClassPtr virObjectRWLockableClass;
>  
>  static void virObjectLockableDispose(void *anyobj);
> +static void virObjectRWLockableDispose(void *anyobj);
>  
>  static int
>  virObjectOnceInit(void)
> @@ -67,6 +69,12 @@ virObjectOnceInit(void)
>                                                 virObjectLockableDispose)))
>          return -1;
>  
> +    if (!(virObjectRWLockableClass = virClassNew(virObjectClass,
> +                                                 "virObjectRWLockable",
> +                                                 sizeof(virObjectRWLockable),
> +                                                 virObjectRWLockableDispose)))
> +        return -1;
> +
>      return 0;
>  }
>  
> @@ -103,6 +111,20 @@ virClassForObjectLockable(void)
>  }
>  
>  
> +/**
> + * virClassForObjectRWLockable:
> + *
> + * Returns the class instance for the virObjectRWLockable type
> + */
> +virClassPtr
> +virClassForObjectRWLockable(void)
> +{
> +    if (virObjectInitialize() < 0)
> +        return NULL;
> +
> +    return virObjectRWLockableClass;
> +}
> +

There should be two empty lines.

>  /**
>   * virClassNew:
>   * @parent: the parent class
> @@ -237,6 +259,32 @@ virObjectLockableNew(virClassPtr klass)
>  }
>  
>  
> +void *
> +virObjectRWLockableNew(virClassPtr klass)
> +{
> +    virObjectRWLockablePtr obj;
> +
> +    if (!virClassIsDerivedFrom(klass, virClassForObjectRWLockable())) {
> +        virReportInvalidArg(klass,
> +                            _("Class %s must derive from virObjectRWLockable"),
> +                            virClassName(klass));
> +        return NULL;
> +    }
> +
> +    if (!(obj = virObjectNew(klass)))
> +        return NULL;
> +
> +    if (virRWLockInitPreferWriter(&obj->lock) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Unable to initialize RW lock"));
> +        virObjectUnref(obj);
> +        return NULL;
> +    }
> +
> +    return obj;
> +}
> +
> +
>  static void
>  virObjectLockableDispose(void *anyobj)
>  {
> @@ -246,6 +294,15 @@ virObjectLockableDispose(void *anyobj)
>  }
>  
>  
> +static void
> +virObjectRWLockableDispose(void *anyobj)
> +{
> +    virObjectRWLockablePtr obj = anyobj;
> +
> +    virRWLockDestroy(&obj->lock);
> +}
> +
> +
>  /**
>   * virObjectUnref:
>   * @anyobj: any instance of virObjectPtr
> @@ -309,28 +366,13 @@ virObjectRef(void *anyobj)
>  }
>  
>  
> -static virObjectLockablePtr
> -virObjectGetLockableObj(void *anyobj)
> -{
> -    virObjectPtr obj;
> -
> -    if (virObjectIsClass(anyobj, virObjectLockableClass))
> -        return anyobj;
> -
> -    obj = anyobj;
> -    VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
> -              anyobj, obj ? obj->klass->name : "(unknown)");
> -
> -    return NULL;
> -}
> -
> -
>  /**
>   * virObjectLock:
> - * @anyobj: any instance of virObjectLockablePtr
> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>   *
> - * Acquire a lock on @anyobj. The lock must be
> - * released by virObjectUnlock.
> + * Acquire a lock on @anyobj. The lock must be released by
> + * virObjectUnlock. In case the passed object is instance of
> + * virObjectRWLockable a write lock is acquired.
>   *
>   * The caller is expected to have acquired a reference
>   * on the object before locking it (eg virObjectRef).
> @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
>  void
>  virObjectLock(void *anyobj)
>  {
> -    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
> +    if (virObjectIsClass(anyobj, virObjectLockableClass)) {
> +        virObjectLockablePtr obj = anyobj;
> +        virMutexLock(&obj->lock);
> +    } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> +        virObjectRWLockablePtr obj = anyobj;
> +        virRWLockWrite(&obj->lock);
> +    } else {
> +        virObjectPtr obj = anyobj;
> +        VIR_WARN("Object %p (%s) is not a virObjectLockable "
> +                 "nor virObjectRWLockable instance",
> +                 anyobj, obj ? obj->klass->name : "(unknown)");
> +    }
> +}
>  
> -    if (!obj)
> -        return;
>  
> -    virMutexLock(&obj->lock);
> +/**
> + * virObjectLockRead:
> + * @anyobj: any instance of virObjectRWLockablePtr

s/virObjectRWLockablePtr/virObjectRWLockable/

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>  /**
>   * 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)");
> +    }
>  }
>  
[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

>> 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list