[libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable

[Pavel Hrdina]

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 at 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 at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170724/268dbe3b/attachment-0001.sig>