Unlike it's counterparts, the nwfilter object code needs to be able
to support recursive read locks while processing and checking new
filters against the existing environment. Thus instead of using a
virObjectLockable which uses pure mutexes, use the virObjectRWLockable
and primarily use RWLockRead when obtaining the object lock since
RWLockRead locks can be recursively obtained (up to a limit) as long
as there's a corresponding unlock.

Since all the object management is within the virnwfilterobj code, we
can limit the number of Write locks on the object to very small areas
of code to ensure we don't run into deadlock's with other threads and
domain code that will check/use the filters (it's a very delicate
balance). This limits the write locks to AssignDef and Remove processing.

This patch will introduce a new API virNWFilterObjEndAPI to unlock
and deref the object.

This patch introduces two helpers to promote/demote the read/write lock.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
 src/conf/virnwfilterobj.h              |   9 +-
 src/libvirt_private.syms               |   3 +-
 src/nwfilter/nwfilter_driver.c         |  15 +--
 src/nwfilter/nwfilter_gentech_driver.c |  11 +-
 5 files changed, 153 insertions(+), 78 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 87d7e7270..cd52706ec 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -34,7 +34,7 @@
 VIR_LOG_INIT("conf.virnwfilterobj");
 
 struct _virNWFilterObj {
-    virMutex lock;
+    virObjectRWLockable parent;
 
     bool wantRemoved;
 
@@ -47,27 +47,69 @@ struct _virNWFilterObjList {
     virNWFilterObjPtr *objs;
 };
 
+static virClassPtr virNWFilterObjClass;
+static void virNWFilterObjDispose(void *opaque);
+
+
+static int
+virNWFilterObjOnceInit(void)
+{
+    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
+                                            "virNWFilterObj",
+                                            sizeof(virNWFilterObj),
+                                            virNWFilterObjDispose)))
+        return -1;
+
+    return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
+
 
 static virNWFilterObjPtr
 virNWFilterObjNew(void)
 {
     virNWFilterObjPtr obj;
 
-    if (VIR_ALLOC(obj) < 0)
+    if (virNWFilterObjInitialize() < 0)
         return NULL;
 
-    if (virMutexInitRecursive(&obj->lock) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "%s", _("cannot initialize mutex"));
-        VIR_FREE(obj);
+    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
         return NULL;
-    }
 
-    virNWFilterObjLock(obj);
+    virObjectRWLockWrite(obj);
     return obj;
 }
 
 
+static void
+virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
+{
+    virObjectRWUnlock(obj);
+    virObjectRWLockWrite(obj);
+}
+
+
+static void
+virNWFilterObjDemoteFromWrite(virNWFilterObjPtr obj)
+{
+    virObjectRWUnlock(obj);
+    virObjectRWLockRead(obj);
+}
+
+
+void
+virNWFilterObjEndAPI(virNWFilterObjPtr *obj)
+{
+    if (!*obj)
+        return;
+
+    virObjectRWUnlock(*obj);
+    virObjectUnref(*obj);
+    *obj = NULL;
+}
+
+
 virNWFilterDefPtr
 virNWFilterObjGetDef(virNWFilterObjPtr obj)
 {
@@ -90,17 +132,15 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj)
 
 
 static void
-virNWFilterObjFree(virNWFilterObjPtr obj)
+virNWFilterObjDispose(void *opaque)
 {
+    virNWFilterObjPtr obj = opaque;
+
     if (!obj)
         return;
 
     virNWFilterDefFree(obj->def);
     virNWFilterDefFree(obj->newDef);
-
-    virMutexDestroy(&obj->lock);
-
-    VIR_FREE(obj);
 }
 
 
@@ -109,7 +149,7 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
 {
     size_t i;
     for (i = 0; i < nwfilters->count; i++)
-        virNWFilterObjFree(nwfilters->objs[i]);
+        virObjectUnref(nwfilters->objs[i]);
     VIR_FREE(nwfilters->objs);
     VIR_FREE(nwfilters);
 }
@@ -132,22 +172,32 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
 {
     size_t i;
 
-    virNWFilterObjUnlock(obj);
+    virObjectRWUnlock(obj);
 
     for (i = 0; i < nwfilters->count; i++) {
-        virNWFilterObjLock(nwfilters->objs[i]);
+        virObjectRWLockWrite(nwfilters->objs[i]);
         if (nwfilters->objs[i] == obj) {
-            virNWFilterObjUnlock(nwfilters->objs[i]);
-            virNWFilterObjFree(nwfilters->objs[i]);
+            virObjectRWUnlock(nwfilters->objs[i]);
+            virObjectUnref(nwfilters->objs[i]);
 
             VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
             break;
         }
-        virNWFilterObjUnlock(nwfilters->objs[i]);
+        virObjectRWUnlock(nwfilters->objs[i]);
     }
 }
 
 
+/**
+ * virNWFilterObjListFindByUUID
+ * @nwfilters: Pointer to filter list
+ * @uuid: UUID to use to lookup the object
+ *
+ * Search for the object by uuidstr in the hash table and return a read
+ * locked copy of the object.
+ *
+ * Returns: A reffed and read locked object or NULL on error
+ */
 virNWFilterObjPtr
 virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
                              const unsigned char *uuid)
@@ -158,17 +208,27 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
 
     for (i = 0; i < nwfilters->count; i++) {
         obj = nwfilters->objs[i];
-        virNWFilterObjLock(obj);
+        virObjectRWLockRead(obj);
         def = obj->def;
         if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
-            return obj;
-        virNWFilterObjUnlock(obj);
+            return virObjectRef(obj);
+        virObjectRWUnlock(obj);
     }
 
     return NULL;
 }
 
 
+/**
+ * virNWFilterObjListFindByName
+ * @nwfilters: Pointer to filter list
+ * @name: filter name to use to lookup the object
+ *
+ * Search for the object by name in the hash table and return a read
+ * locked copy of the object.
+ *
+ * Returns: A reffed and read locked object or NULL on error
+ */
 virNWFilterObjPtr
 virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
                              const char *name)
@@ -179,11 +239,11 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
 
     for (i = 0; i < nwfilters->count; i++) {
         obj = nwfilters->objs[i];
-        virNWFilterObjLock(obj);
+        virObjectRWLockRead(obj);
         def = obj->def;
         if (STREQ_NULLABLE(def->name, name))
-            return obj;
-        virNWFilterObjUnlock(obj);
+            return virObjectRef(obj);
+        virObjectRWUnlock(obj);
     }
 
     return NULL;
@@ -205,7 +265,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
     if (virNWFilterObjWantRemoved(obj)) {
         virReportError(VIR_ERR_NO_NWFILTER,
                        _("Filter '%s' is in use."), filtername);
-        virNWFilterObjUnlock(obj);
+        virNWFilterObjEndAPI(&obj);
         return NULL;
     }
 
@@ -240,7 +300,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
             if (obj) {
                 rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def,
                                                       filtername);
-                virNWFilterObjUnlock(obj);
+                virNWFilterObjEndAPI(&obj);
                 if (rc < 0)
                     break;
             }
@@ -269,17 +329,36 @@ virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
 }
 
 
+/* virNWFilterObjTestUnassignDef
+ * @obj: A read locked nwfilter object
+ *
+ * Cause the rebuild to occur because we're about to undefine the nwfilter.
+ * The rebuild code is designed to check if the filter is currently in use
+ * by a domain and thus disallow the unassign.
+ *
+ * NB: Although we enter with the UPDATE lock from UNDEFINE, let's still
+ *     promote to a WRITE lock before changing *this* object's wantRemoved
+ *     value that will be used in the virNWFilterObjListFindInstantiateFilter
+ *     processing to determine whether we can really remove this filter or not.
+ *
+ * Returns 0 if we can continue with the unassign, -1 if it's in use
+ */
 int
 virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj)
 {
     int rc = 0;
 
+    virNWFilterObjPromoteToWrite(obj);
     obj->wantRemoved = true;
+    virNWFilterObjDemoteFromWrite(obj);
+
     /* trigger the update on VMs referencing the filter */
     if (virNWFilterTriggerVMFilterRebuild() < 0)
         rc = -1;
 
+    virNWFilterObjPromoteToWrite(obj);
     obj->wantRemoved = false;
+    virNWFilterObjDemoteFromWrite(obj);
 
     return rc;
 }
@@ -322,10 +401,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
                            _("filter with same UUID but different name "
                              "('%s') already exists"),
                            objdef->name);
-            virNWFilterObjUnlock(obj);
+            virNWFilterObjEndAPI(&obj);
             return NULL;
         }
-        virNWFilterObjUnlock(obj);
+        virNWFilterObjEndAPI(&obj);
     } else {
         if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
             char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -335,7 +414,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("filter '%s' already exists with uuid %s"),
                            def->name, uuidstr);
-            virNWFilterObjUnlock(obj);
+            virNWFilterObjEndAPI(&obj);
             return NULL;
         }
     }
@@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
     }
 
 
+    /* Get a READ lock and immediately promote to WRITE while we adjust
+     * data within. */
     if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
 
         objdef = obj->def;
         if (virNWFilterDefEqual(def, objdef)) {
+            virNWFilterObjPromoteToWrite(obj);
             virNWFilterDefFree(objdef);
             obj->def = def;
+            virNWFilterObjDemoteFromWrite(obj);
             return obj;
         }
 
+        /* Set newDef and run the trigger with a demoted lock since it may need
+         * to grab a read lock on this object and promote before returning. */
+        virNWFilterObjPromoteToWrite(obj);
         obj->newDef = def;
+        virNWFilterObjDemoteFromWrite(obj);
+
         /* trigger the update on VMs referencing the filter */
         if (virNWFilterTriggerVMFilterRebuild() < 0) {
+            virNWFilterObjPromoteToWrite(obj);
             obj->newDef = NULL;
-            virNWFilterObjUnlock(obj);
+            /* NB: We're done, no need to Demote and End, just End */
+            virNWFilterObjEndAPI(&obj);
             return NULL;
         }
 
+        virNWFilterObjPromoteToWrite(obj);
         virNWFilterDefFree(objdef);
         obj->def = def;
         obj->newDef = NULL;
+        virNWFilterObjDemoteFromWrite(obj);
         return obj;
     }
 
@@ -375,11 +467,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
 
     if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
                                 nwfilters->count, obj) < 0) {
-        virNWFilterObjUnlock(obj);
-        virNWFilterObjFree(obj);
+        virNWFilterObjEndAPI(&obj);
         return NULL;
     }
+    virObjectRef(obj);
     obj->def = def;
+    virNWFilterObjDemoteFromWrite(obj);
 
     return obj;
 }
@@ -395,10 +488,10 @@ virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters,
 
     for (i = 0; i < nwfilters->count; i++) {
         virNWFilterObjPtr obj = nwfilters->objs[i];
-        virNWFilterObjLock(obj);
+        virObjectRWLockRead(obj);
         if (!filter || filter(conn, obj->def))
             nfilters++;
-        virNWFilterObjUnlock(obj);
+        virObjectRWUnlock(obj);
     }
 
     return nfilters;
@@ -418,16 +511,16 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters,
 
     for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
         virNWFilterObjPtr obj = nwfilters->objs[i];
-        virNWFilterObjLock(obj);
+        virObjectRWLockRead(obj);
         def = obj->def;
         if (!filter || filter(conn, def)) {
             if (VIR_STRDUP(names[nnames], def->name) < 0) {
-                virNWFilterObjUnlock(obj);
+                virObjectRWUnlock(obj);
                 goto failure;
             }
             nnames++;
         }
-        virNWFilterObjUnlock(obj);
+        virObjectRWUnlock(obj);
     }
 
     return nnames;
@@ -464,16 +557,16 @@ virNWFilterObjListExport(virConnectPtr conn,
 
     for (i = 0; i < nwfilters->count; i++) {
         obj = nwfilters->objs[i];
-        virNWFilterObjLock(obj);
+        virObjectRWLockRead(obj);
         def = obj->def;
         if (!filter || filter(conn, def)) {
             if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) {
-                virNWFilterObjUnlock(obj);
+                virObjectRWUnlock(obj);
                 goto cleanup;
             }
             tmp_filters[nfilters++] = nwfilter;
         }
-        virNWFilterObjUnlock(obj);
+        virObjectRWUnlock(obj);
     }
 
     *filters = tmp_filters;
@@ -551,24 +644,10 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
             continue;
 
         obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name);
-        if (obj)
-            virNWFilterObjUnlock(obj);
+
+        virNWFilterObjEndAPI(&obj);
     }
 
     VIR_DIR_CLOSE(dir);
     return ret;
 }
-
-
-void
-virNWFilterObjLock(virNWFilterObjPtr obj)
-{
-    virMutexLock(&obj->lock);
-}
-
-
-void
-virNWFilterObjUnlock(virNWFilterObjPtr obj)
-{
-    virMutexUnlock(&obj->lock);
-}
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
index 8e79518ed..0281bc5f5 100644
--- a/src/conf/virnwfilterobj.h
+++ b/src/conf/virnwfilterobj.h
@@ -41,6 +41,9 @@ struct _virNWFilterDriverState {
     bool watchingFirewallD;
 };
 
+void
+virNWFilterObjEndAPI(virNWFilterObjPtr *obj);
+
 virNWFilterDefPtr
 virNWFilterObjGetDef(virNWFilterObjPtr obj);
 
@@ -105,10 +108,4 @@ int
 virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
                                  const char *configDir);
 
-void
-virNWFilterObjLock(virNWFilterObjPtr obj);
-
-void
-virNWFilterObjUnlock(virNWFilterObjPtr obj);
-
 #endif /* VIRNWFILTEROBJ_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0bce0bbfb..2f7fc550b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1029,6 +1029,7 @@ virNodeDeviceObjListRemove;
 
 
 # conf/virnwfilterobj.h
+virNWFilterObjEndAPI;
 virNWFilterObjGetDef;
 virNWFilterObjGetNewDef;
 virNWFilterObjListAssignDef;
@@ -1042,9 +1043,7 @@ virNWFilterObjListLoadAllConfigs;
 virNWFilterObjListNew;
 virNWFilterObjListNumOfNWFilters;
 virNWFilterObjListRemove;
-virNWFilterObjLock;
 virNWFilterObjTestUnassignDef;
-virNWFilterObjUnlock;
 virNWFilterObjWantRemoved;
 
 
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 717bce269..3a0447627 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -466,7 +466,7 @@ nwfilterLookupByUUID(virConnectPtr conn,
     nwfilter = virGetNWFilter(conn, def->name, def->uuid);
 
  cleanup:
-    virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
     return nwfilter;
 }
 
@@ -496,7 +496,7 @@ nwfilterLookupByName(virConnectPtr conn,
     nwfilter = virGetNWFilter(conn, def->name, def->uuid);
 
  cleanup:
-    virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
     return nwfilter;
 }
 
@@ -583,6 +583,8 @@ nwfilterDefineXML(virConnectPtr conn,
 
     if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) {
         virNWFilterObjListRemove(driver->nwfilters, obj);
+        virObjectUnref(obj);
+        obj = NULL;
         goto cleanup;
     }
 
@@ -590,8 +592,7 @@ nwfilterDefineXML(virConnectPtr conn,
 
  cleanup:
     virNWFilterDefFree(def);
-    if (obj)
-        virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
 
     virNWFilterCallbackDriversUnlock();
     virNWFilterUnlockFilterUpdates();
@@ -629,12 +630,12 @@ nwfilterUndefine(virNWFilterPtr nwfilter)
         goto cleanup;
 
     virNWFilterObjListRemove(driver->nwfilters, obj);
+    virObjectUnref(obj);
     obj = NULL;
     ret = 0;
 
  cleanup:
-    if (obj)
-        virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
 
     virNWFilterCallbackDriversUnlock();
     virNWFilterUnlockFilterUpdates();
@@ -667,7 +668,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
     ret = virNWFilterDefFormat(def);
 
  cleanup:
-    virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
     return ret;
 }
 
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
index 840d419bb..48d0e1769 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -316,7 +316,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst)
     size_t i;
 
     for (i = 0; i < inst->nfilters; i++)
-        virNWFilterObjUnlock(inst->filters[i]);
+        virNWFilterObjEndAPI(&inst->filters[i]);
     VIR_FREE(inst->filters);
     inst->nfilters = 0;
 
@@ -426,8 +426,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver,
     if (ret < 0)
         virNWFilterInstReset(inst);
     virNWFilterHashTableFree(tmpvars);
-    if (obj)
-        virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
     return ret;
 }
 
@@ -541,7 +540,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
 
             /* create a temporary hashmap for depth-first tree traversal */
             if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, vars))) {
-                virNWFilterObjUnlock(obj);
+                virNWFilterObjEndAPI(&obj);
                 return -1;
             }
 
@@ -565,7 +564,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
 
             virNWFilterHashTableFree(tmpvars);
 
-            virNWFilterObjUnlock(obj);
+            virNWFilterObjEndAPI(&obj);
             if (rc < 0)
                 return -1;
         }
@@ -839,7 +838,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver,
     virNWFilterHashTableFree(vars1);
 
  err_exit:
-    virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
 
     VIR_FREE(str_ipaddr);
     VIR_FREE(str_macaddr);
-- 
2.13.6

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

On 02/06/2018 08:20 PM, John Ferlan wrote:
> Unlike it's counterparts, the nwfilter object code needs to be able
> to support recursive read locks while processing and checking new
> filters against the existing environment. Thus instead of using a
> virObjectLockable which uses pure mutexes, use the virObjectRWLockable
> and primarily use RWLockRead when obtaining the object lock since
> RWLockRead locks can be recursively obtained (up to a limit) as long
> as there's a corresponding unlock.
> 
> Since all the object management is within the virnwfilterobj code, we
> can limit the number of Write locks on the object to very small areas
> of code to ensure we don't run into deadlock's with other threads and
> domain code that will check/use the filters (it's a very delicate
> balance). This limits the write locks to AssignDef and Remove processing.
> 
> This patch will introduce a new API virNWFilterObjEndAPI to unlock
> and deref the object.
> 
> This patch introduces two helpers to promote/demote the read/write lock.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
>  src/conf/virnwfilterobj.h              |   9 +-
>  src/libvirt_private.syms               |   3 +-
>  src/nwfilter/nwfilter_driver.c         |  15 +--
>  src/nwfilter/nwfilter_gentech_driver.c |  11 +-
>  5 files changed, 153 insertions(+), 78 deletions(-)
> 
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 87d7e7270..cd52706ec 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -34,7 +34,7 @@
>  VIR_LOG_INIT("conf.virnwfilterobj");
>  
>  struct _virNWFilterObj {
> -    virMutex lock;
> +    virObjectRWLockable parent;
>  
>      bool wantRemoved;
>  
> @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
>      virNWFilterObjPtr *objs;
>  };
>  
> +static virClassPtr virNWFilterObjClass;
> +static void virNWFilterObjDispose(void *opaque);
> +
> +
> +static int
> +virNWFilterObjOnceInit(void)
> +{
> +    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
> +                                            "virNWFilterObj",
> +                                            sizeof(virNWFilterObj),
> +                                            virNWFilterObjDispose)))
> +        return -1;
> +
> +    return 0;
> +}
> +
> +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
> +
>  
>  static virNWFilterObjPtr
>  virNWFilterObjNew(void)
>  {
>      virNWFilterObjPtr obj;
>  
> -    if (VIR_ALLOC(obj) < 0)
> +    if (virNWFilterObjInitialize() < 0)
>          return NULL;
>  
> -    if (virMutexInitRecursive(&obj->lock) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       "%s", _("cannot initialize mutex"));
> -        VIR_FREE(obj);
> +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
>          return NULL;
> -    }
>  
> -    virNWFilterObjLock(obj);
> +    virObjectRWLockWrite(obj);
>      return obj;
>  }
>  
>  
> +static void
> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
> +{
> +    virObjectRWUnlock(obj);
> +    virObjectRWLockWrite(obj);
> +}

How can this not deadlock? This will work only if @obj is locked exactly
once. But since we allow recursive locks any lock() that happens in the
2nd level must deadlock with this. On the other hand, there's no such
case in the code currently. But that doesn't stop anybody from calling
PromoteWrite() without understanding its limitations.

Maybe the fact that we need recursive lock for NWFilterObj means it's
not a good candidate for virObjectRWLocable? It is still a good
candidate for virObject though.

Or if you want to spend extra time on this, maybe introducing
virObjectRecursiveLockable may be worth it (terrible name, I know).


> @@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>      }
>  
>  
> +    /* Get a READ lock and immediately promote to WRITE while we adjust
> +     * data within. */
>      if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>  
>          objdef = obj->def;
>          if (virNWFilterDefEqual(def, objdef)) {
> +            virNWFilterObjPromoteToWrite(obj);
>              virNWFilterDefFree(objdef);
>              obj->def = def;
> +            virNWFilterObjDemoteFromWrite(obj);
>              return obj;
>          }

What is the idea behind this if()? I don't understand it. There doesn't
seem to be any fields in @objdef or

>  
> +        /* Set newDef and run the trigger with a demoted lock since it may need
> +         * to grab a read lock on this object and promote before returning. */
> +        virNWFilterObjPromoteToWrite(obj);
>          obj->newDef = def;
> +        virNWFilterObjDemoteFromWrite(obj);
> +
>          /* trigger the update on VMs referencing the filter */
>          if (virNWFilterTriggerVMFilterRebuild() < 0) {
> +            virNWFilterObjPromoteToWrite(obj);
>              obj->newDef = NULL;
> -            virNWFilterObjUnlock(obj);
> +            /* NB: We're done, no need to Demote and End, just End */
> +            virNWFilterObjEndAPI(&obj);
>              return NULL;
>          }
>  
> +        virNWFilterObjPromoteToWrite(obj);
>          virNWFilterDefFree(objdef);
>          obj->def = def;
>          obj->newDef = NULL;
> +        virNWFilterObjDemoteFromWrite(obj);
>          return obj;
>      }
>  

Michal

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

On 02/08/2018 08:13 AM, Michal Privoznik wrote:
> On 02/06/2018 08:20 PM, John Ferlan wrote:
>> Unlike it's counterparts, the nwfilter object code needs to be able
>> to support recursive read locks while processing and checking new
>> filters against the existing environment. Thus instead of using a
>> virObjectLockable which uses pure mutexes, use the virObjectRWLockable
>> and primarily use RWLockRead when obtaining the object lock since
>> RWLockRead locks can be recursively obtained (up to a limit) as long
>> as there's a corresponding unlock.
>>
>> Since all the object management is within the virnwfilterobj code, we
>> can limit the number of Write locks on the object to very small areas
>> of code to ensure we don't run into deadlock's with other threads and
>> domain code that will check/use the filters (it's a very delicate
>> balance). This limits the write locks to AssignDef and Remove processing.
>>
>> This patch will introduce a new API virNWFilterObjEndAPI to unlock
>> and deref the object.
>>
>> This patch introduces two helpers to promote/demote the read/write lock.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
>>  src/conf/virnwfilterobj.h              |   9 +-
>>  src/libvirt_private.syms               |   3 +-
>>  src/nwfilter/nwfilter_driver.c         |  15 +--
>>  src/nwfilter/nwfilter_gentech_driver.c |  11 +-
>>  5 files changed, 153 insertions(+), 78 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index 87d7e7270..cd52706ec 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -34,7 +34,7 @@
>>  VIR_LOG_INIT("conf.virnwfilterobj");
>>  
>>  struct _virNWFilterObj {
>> -    virMutex lock;
>> +    virObjectRWLockable parent;
>>  
>>      bool wantRemoved;
>>  
>> @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
>>      virNWFilterObjPtr *objs;
>>  };
>>  
>> +static virClassPtr virNWFilterObjClass;
>> +static void virNWFilterObjDispose(void *opaque);
>> +
>> +
>> +static int
>> +virNWFilterObjOnceInit(void)
>> +{
>> +    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
>> +                                            "virNWFilterObj",
>> +                                            sizeof(virNWFilterObj),
>> +                                            virNWFilterObjDispose)))
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
>> +
>>  
>>  static virNWFilterObjPtr
>>  virNWFilterObjNew(void)
>>  {
>>      virNWFilterObjPtr obj;
>>  
>> -    if (VIR_ALLOC(obj) < 0)
>> +    if (virNWFilterObjInitialize() < 0)
>>          return NULL;
>>  
>> -    if (virMutexInitRecursive(&obj->lock) < 0) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       "%s", _("cannot initialize mutex"));
>> -        VIR_FREE(obj);
>> +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
>>          return NULL;
>> -    }
>>  
>> -    virNWFilterObjLock(obj);
>> +    virObjectRWLockWrite(obj);
>>      return obj;
>>  }
>>  
>>  
>> +static void
>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>> +{
>> +    virObjectRWUnlock(obj);
>> +    virObjectRWLockWrite(obj);
>> +}
> 
> How can this not deadlock? This will work only if @obj is locked exactly
> once. But since we allow recursive locks any lock() that happens in the
> 2nd level must deadlock with this. On the other hand, there's no such
> case in the code currently. But that doesn't stop anybody from calling
> PromoteWrite() without understanding its limitations.
> 
> Maybe the fact that we need recursive lock for NWFilterObj means it's
> not a good candidate for virObjectRWLocable? It is still a good
> candidate for virObject though.
> 
> Or if you want to spend extra time on this, maybe introducing
> virObjectRecursiveLockable may be worth it (terrible name, I know).
> 
> 

I dunno, worked for me. The helper is being called from a thread that
has taken out a READ lock at most one time and needs to get the WRITE
lock in order to change things. If something else has the READ lock that
thread waits until the other thread releases the READ lock as far as I
understand things.

Back when I first did this I had quite a lot of debug code and I have a
vague recollection of seeing output from this particular path and seeing
a couple of unlocks before the WRITE was taken while running the avocado
tests.

I have zero interest in spending more time on this. That ship sailed. If
the decision is to drop the patches, then fine. I tried.  I disagree
that NWFilterObj is not a candidate for RWLockable - in fact it's quite
the opposite *because* of the recursive locks.

Additionally, because of the recursive locks we cannot use the
virObjectLockable and quite frankly I see zero benefit from going down
the path of just virObject. As they say, patches welcome.

Somewhere along the line, I recall trying to post patches that were
essentially virObjectRecursiveLockable and got NACK'd.

>> @@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>>      }
>>  
>>  
>> +    /* Get a READ lock and immediately promote to WRITE while we adjust
>> +     * data within. */
>>      if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>>  
>>          objdef = obj->def;
>>          if (virNWFilterDefEqual(def, objdef)) {
>> +            virNWFilterObjPromoteToWrite(obj);
>>              virNWFilterDefFree(objdef);
>>              obj->def = def;
>> +            virNWFilterObjDemoteFromWrite(obj);
>>              return obj;
>>          }
> 
> What is the idea behind this if()? I don't understand it. There doesn't
> seem to be any fields in @objdef or
> 

Or you lost your train of thought? Happens with this code. The *DefEqual
ensures that the new definition is the exact same as the old, but
replaces the def for whatever reason - kind of the redefine type logic.
If something is different, then we're stuck going through the
FilterRebuild logic.

That's about the extent of what I understand.

John

>>  
>> +        /* Set newDef and run the trigger with a demoted lock since it may need
>> +         * to grab a read lock on this object and promote before returning. */
>> +        virNWFilterObjPromoteToWrite(obj);
>>          obj->newDef = def;
>> +        virNWFilterObjDemoteFromWrite(obj);
>> +
>>          /* trigger the update on VMs referencing the filter */
>>          if (virNWFilterTriggerVMFilterRebuild() < 0) {
>> +            virNWFilterObjPromoteToWrite(obj);
>>              obj->newDef = NULL;
>> -            virNWFilterObjUnlock(obj);
>> +            /* NB: We're done, no need to Demote and End, just End */
>> +            virNWFilterObjEndAPI(&obj);
>>              return NULL;
>>          }
>>  
>> +        virNWFilterObjPromoteToWrite(obj);
>>          virNWFilterDefFree(objdef);
>>          obj->def = def;
>>          obj->newDef = NULL;
>> +        virNWFilterObjDemoteFromWrite(obj);
>>          return obj;
>>      }
>>  
> 
> Michal
> 

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

On 02/08/2018 02:32 PM, John Ferlan wrote:
> 
> 
> On 02/08/2018 08:13 AM, Michal Privoznik wrote:
>> On 02/06/2018 08:20 PM, John Ferlan wrote:
>>> Unlike it's counterparts, the nwfilter object code needs to be able
>>> to support recursive read locks while processing and checking new
>>> filters against the existing environment. Thus instead of using a
>>> virObjectLockable which uses pure mutexes, use the virObjectRWLockable
>>> and primarily use RWLockRead when obtaining the object lock since
>>> RWLockRead locks can be recursively obtained (up to a limit) as long
>>> as there's a corresponding unlock.
>>>
>>> Since all the object management is within the virnwfilterobj code, we
>>> can limit the number of Write locks on the object to very small areas
>>> of code to ensure we don't run into deadlock's with other threads and
>>> domain code that will check/use the filters (it's a very delicate
>>> balance). This limits the write locks to AssignDef and Remove processing.
>>>
>>> This patch will introduce a new API virNWFilterObjEndAPI to unlock
>>> and deref the object.
>>>
>>> This patch introduces two helpers to promote/demote the read/write lock.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> ---
>>>  src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
>>>  src/conf/virnwfilterobj.h              |   9 +-
>>>  src/libvirt_private.syms               |   3 +-
>>>  src/nwfilter/nwfilter_driver.c         |  15 +--
>>>  src/nwfilter/nwfilter_gentech_driver.c |  11 +-
>>>  5 files changed, 153 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>>> index 87d7e7270..cd52706ec 100644
>>> --- a/src/conf/virnwfilterobj.c
>>> +++ b/src/conf/virnwfilterobj.c
>>> @@ -34,7 +34,7 @@
>>>  VIR_LOG_INIT("conf.virnwfilterobj");
>>>  
>>>  struct _virNWFilterObj {
>>> -    virMutex lock;
>>> +    virObjectRWLockable parent;
>>>  
>>>      bool wantRemoved;
>>>  
>>> @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
>>>      virNWFilterObjPtr *objs;
>>>  };
>>>  
>>> +static virClassPtr virNWFilterObjClass;
>>> +static void virNWFilterObjDispose(void *opaque);
>>> +
>>> +
>>> +static int
>>> +virNWFilterObjOnceInit(void)
>>> +{
>>> +    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
>>> +                                            "virNWFilterObj",
>>> +                                            sizeof(virNWFilterObj),
>>> +                                            virNWFilterObjDispose)))
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
>>> +
>>>  
>>>  static virNWFilterObjPtr
>>>  virNWFilterObjNew(void)
>>>  {
>>>      virNWFilterObjPtr obj;
>>>  
>>> -    if (VIR_ALLOC(obj) < 0)
>>> +    if (virNWFilterObjInitialize() < 0)
>>>          return NULL;
>>>  
>>> -    if (virMutexInitRecursive(&obj->lock) < 0) {
>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -                       "%s", _("cannot initialize mutex"));
>>> -        VIR_FREE(obj);
>>> +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
>>>          return NULL;
>>> -    }
>>>  
>>> -    virNWFilterObjLock(obj);
>>> +    virObjectRWLockWrite(obj);
>>>      return obj;
>>>  }
>>>  
>>>  
>>> +static void
>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>> +{
>>> +    virObjectRWUnlock(obj);
>>> +    virObjectRWLockWrite(obj);
>>> +}
>>
>> How can this not deadlock? This will work only if @obj is locked exactly
>> once. But since we allow recursive locks any lock() that happens in the
>> 2nd level must deadlock with this. On the other hand, there's no such
>> case in the code currently. But that doesn't stop anybody from calling
>> PromoteWrite() without understanding its limitations.
>>
>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>> not a good candidate for virObjectRWLocable? It is still a good
>> candidate for virObject though.
>>
>> Or if you want to spend extra time on this, maybe introducing
>> virObjectRecursiveLockable may be worth it (terrible name, I know).
>>
>>
> 
> I dunno, worked for me. The helper is being called from a thread that
> has taken out a READ lock at most one time and needs to get the WRITE
> lock in order to change things. If something else has the READ lock that
> thread waits until the other thread releases the READ lock as far as I
> understand things.

Yes, I can see that. It's just that since the original lock is recursive
I expected things to get recursive and thus I've been thinking how would
this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
places where lock promoting is used are kind of safe since all the
locking is done at the first level.

> 
> Back when I first did this I had quite a lot of debug code and I have a
> vague recollection of seeing output from this particular path and seeing
> a couple of unlocks before the WRITE was taken while running the avocado
> tests.
> 
> I have zero interest in spending more time on this. That ship sailed. If
> the decision is to drop the patches, then fine. I tried.  I disagree
> that NWFilterObj is not a candidate for RWLockable - in fact it's quite
> the opposite *because* of the recursive locks.

Well, there's a difference between recursive locks and rwlocks. And it's
exactly in what happens in recursion. With recursive locks we don't need
any lock promoting and thus it's safe to do read/write after lock().
With lock promoting (esp. done in unlock()+lockWrite() manner) we get
TOCTOU bugs.

> 
> Additionally, because of the recursive locks we cannot use the
> virObjectLockable and quite frankly I see zero benefit from going down
> the path of just virObject. As they say, patches welcome.
> 
> Somewhere along the line, I recall trying to post patches that were
> essentially virObjectRecursiveLockable and got NACK'd.

Why is that? IUUC the aim here is to unify all the vir*ObjectList
implementations so that we can drop code duplication. And so far what
I've seen only a couple of virObject* functions are needed
(virObjectRef, virObjectUnref, virObjectLock and virObjectUnlock). So if
we have virObjectRecursiveLockable class then we can still use those 4
functions safely and unify the code here. If you're not interested, I
can cook the patches.

> 
>>> @@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>>>      }
>>>  
>>>  
>>> +    /* Get a READ lock and immediately promote to WRITE while we adjust
>>> +     * data within. */
>>>      if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>>>  
>>>          objdef = obj->def;
>>>          if (virNWFilterDefEqual(def, objdef)) {
>>> +            virNWFilterObjPromoteToWrite(obj);
>>>              virNWFilterDefFree(objdef);
>>>              obj->def = def;
>>> +            virNWFilterObjDemoteFromWrite(obj);
>>>              return obj;
>>>          }
>>
>> What is the idea behind this if()? I don't understand it. There doesn't
>> seem to be any fields in @objdef or
>>
> 
> Or you lost your train of thought? Happens with this code. The *DefEqual
> ensures that the new definition is the exact same as the old, but
> replaces the def for whatever reason - kind of the redefine type logic.
> If something is different, then we're stuck going through the
> FilterRebuild logic.
> 
> That's about the extent of what I understand.

Yes, but the part that I'm not understanding is why we are replacing the
definition in the first place. I mean, if this were some integers
instead of pointers:

int x = 42;

if (struct.member == x) {
  struct.member = x;
}

It makes exactly zero sense to me. But whatever, it's pre-existing.

Michal

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

[...]

>>>> +static void
>>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>>> +{
>>>> +    virObjectRWUnlock(obj);
>>>> +    virObjectRWLockWrite(obj);
>>>> +}
>>>
>>> How can this not deadlock? This will work only if @obj is locked exactly
>>> once. But since we allow recursive locks any lock() that happens in the
>>> 2nd level must deadlock with this. On the other hand, there's no such
>>> case in the code currently. But that doesn't stop anybody from calling
>>> PromoteWrite() without understanding its limitations.
>>>
>>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>>> not a good candidate for virObjectRWLocable? It is still a good
>>> candidate for virObject though.
>>>
>>> Or if you want to spend extra time on this, maybe introducing
>>> virObjectRecursiveLockable may be worth it (terrible name, I know).
>>>
>>>
>>
>> I dunno, worked for me. The helper is being called from a thread that
>> has taken out a READ lock at most one time and needs to get the WRITE
>> lock in order to change things. If something else has the READ lock that
>> thread waits until the other thread releases the READ lock as far as I
>> understand things.
> 
> Yes, I can see that. It's just that since the original lock is recursive
> I expected things to get recursive and thus I've been thinking how would
> this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
> places where lock promoting is used are kind of safe since all the
> locking is done at the first level.
> 

The reason I chose rwlocks and taking a read lock by default in the
FindByUUID and FindByName (as opposed to other vir*FindBy* API's which
return a write locked object) is because I know there's the potential
for other code to be doing Reads and rwlocks allowed for that without
the need to create any sort of recursive lockable object or any sort of
API to perform "trylock"'s (all things I tried until the epiphany to use
rwlocks was reached when you added them for other drivers and their list
protection).

In the long/short run I figured that having a common way to get the lock
and then deciding to change it from Read to Write as necessary would
work just fine for the purpose that was needed.

>>
>> Back when I first did this I had quite a lot of debug code and I have a
>> vague recollection of seeing output from this particular path and seeing
>> a couple of unlocks before the WRITE was taken while running the avocado
>> tests.
>>
>> I have zero interest in spending more time on this. That ship sailed. If
>> the decision is to drop the patches, then fine. I tried.  I disagree
>> that NWFilterObj is not a candidate for RWLockable - in fact it's quite
>> the opposite *because* of the recursive locks.
> 
> Well, there's a difference between recursive locks and rwlocks. And it's
> exactly in what happens in recursion. With recursive locks we don't need
> any lock promoting and thus it's safe to do read/write after lock().
> With lock promoting (esp. done in unlock()+lockWrite() manner) we get
> TOCTOU bugs.
> 

If we had a real lock manager available this wouldn't be a problem ;-)
[sorry just my OpenVMS roots and bias showing].  Still if you read up a
bit on "dlm" you'll get an idea of what I mean.

There's so many locks in the nwfilter code it's a wonder it all works.

It's truly unfortunate that the wrlock mechanism is undefined for
promoting a read lock to write leaving the consumer with few options.

Since we can limit the "updating" of the fields in @obj and getting a
write lock to very specific code paths, hopefully we can limit damage
and/or deadlock type and/or TOCTOU issues

>>
>> Additionally, because of the recursive locks we cannot use the
>> virObjectLockable and quite frankly I see zero benefit from going down
>> the path of just virObject. As they say, patches welcome.
>>
>> Somewhere along the line, I recall trying to post patches that were
>> essentially virObjectRecursiveLockable and got NACK'd.
> 
> Why is that? IUUC the aim here is to unify all the vir*ObjectList
> implementations so that we can drop code duplication. And so far what
> I've seen only a couple of virObject* functions are needed
> (virObjectRef, virObjectUnref, virObjectLock and virObjectUnlock). So if
> we have virObjectRecursiveLockable class then we can still use those 4
> functions safely and unify the code here. If you're not interested, I
> can cook the patches.
> 

Yes, reduction of duplication of vir*ObjectList is/was a goal. How to
get there is where opinions start to vary - my code was essentially
NACK'd, but getting to a point where someone else could try something
different I figured would at least be helpful. This nwfilter code is the
black sheep of the libvirt family in more ways than one.

BTW: We're still not at a point where common List mgmt code could be
written because the domain list mgmt varies greatly between the various
hypervisor's. I started writing some patches to work through those
issues, but have been side tracked by other higher priority items.

BTW, see:

https://www.redhat.com/archives/libvir-list/2017-May/msg01183.html

for my pass at virObjectLockableRecursiveNew


>>
>>>> @@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>>>>      }
>>>>  
>>>>  
>>>> +    /* Get a READ lock and immediately promote to WRITE while we adjust
>>>> +     * data within. */
>>>>      if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>>>>  
>>>>          objdef = obj->def;
>>>>          if (virNWFilterDefEqual(def, objdef)) {
>>>> +            virNWFilterObjPromoteToWrite(obj);
>>>>              virNWFilterDefFree(objdef);
>>>>              obj->def = def;
>>>> +            virNWFilterObjDemoteFromWrite(obj);
>>>>              return obj;
>>>>          }
>>>
>>> What is the idea behind this if()? I don't understand it. There doesn't
>>> seem to be any fields in @objdef or
>>>
>>
>> Or you lost your train of thought? Happens with this code. The *DefEqual
>> ensures that the new definition is the exact same as the old, but
>> replaces the def for whatever reason - kind of the redefine type logic.
>> If something is different, then we're stuck going through the
>> FilterRebuild logic.
>>
>> That's about the extent of what I understand.
> 
> Yes, but the part that I'm not understanding is why we are replacing the
> definition in the first place. I mean, if this were some integers
> instead of pointers:
> 
> int x = 42;
> 
> if (struct.member == x) {
>   struct.member = x;
> }
> 
> It makes exactly zero sense to me. But whatever, it's pre-existing.
> 

Right, pre-existing is what I'm going with 0-)

John

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

On 02/08/2018 04:06 PM, John Ferlan wrote:
> [...]
> 
>>>>> +static void
>>>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>>>> +{
>>>>> +    virObjectRWUnlock(obj);
>>>>> +    virObjectRWLockWrite(obj);
>>>>> +}
>>>>
>>>> How can this not deadlock? This will work only if @obj is locked exactly
>>>> once. But since we allow recursive locks any lock() that happens in the
>>>> 2nd level must deadlock with this. On the other hand, there's no such
>>>> case in the code currently. But that doesn't stop anybody from calling
>>>> PromoteWrite() without understanding its limitations.
>>>>
>>>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>>>> not a good candidate for virObjectRWLocable? It is still a good
>>>> candidate for virObject though.
>>>>
>>>> Or if you want to spend extra time on this, maybe introducing
>>>> virObjectRecursiveLockable may be worth it (terrible name, I know).
>>>>
>>>>
>>>
>>> I dunno, worked for me. The helper is being called from a thread that
>>> has taken out a READ lock at most one time and needs to get the WRITE
>>> lock in order to change things. If something else has the READ lock that
>>> thread waits until the other thread releases the READ lock as far as I
>>> understand things.
>>
>> Yes, I can see that. It's just that since the original lock is recursive
>> I expected things to get recursive and thus I've been thinking how would
>> this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
>> places where lock promoting is used are kind of safe since all the
>> locking is done at the first level.
>>
> 
> The reason I chose rwlocks and taking a read lock by default in the
> FindByUUID and FindByName (as opposed to other vir*FindBy* API's which
> return a write locked object) is because I know there's the potential
> for other code to be doing Reads and rwlocks allowed for that without
> the need to create any sort of recursive lockable object or any sort of
> API to perform "trylock"'s (all things I tried until the epiphany to use
> rwlocks was reached when you added them for other drivers and their list
> protection).
> 
> In the long/short run I figured that having a common way to get the lock
> and then deciding to change it from Read to Write as necessary would
> work just fine for the purpose that was needed.

This is where I disagree. I don't think it's safe to promote a lock by
unlocking it. The moment the lock is unlocked another thread will lock
the object and all the assumptions we made/checked about the object we
made when we had the object locked are gone. So we would need to
reiterate the decision making process.

> 
>>>
>>> Back when I first did this I had quite a lot of debug code and I have a
>>> vague recollection of seeing output from this particular path and seeing
>>> a couple of unlocks before the WRITE was taken while running the avocado
>>> tests.
>>>
>>> I have zero interest in spending more time on this. That ship sailed. If
>>> the decision is to drop the patches, then fine. I tried.  I disagree
>>> that NWFilterObj is not a candidate for RWLockable - in fact it's quite
>>> the opposite *because* of the recursive locks.
>>
>> Well, there's a difference between recursive locks and rwlocks. And it's
>> exactly in what happens in recursion. With recursive locks we don't need
>> any lock promoting and thus it's safe to do read/write after lock().
>> With lock promoting (esp. done in unlock()+lockWrite() manner) we get
>> TOCTOU bugs.
>>
> 
> If we had a real lock manager available this wouldn't be a problem ;-)
> [sorry just my OpenVMS roots and bias showing].  Still if you read up a
> bit on "dlm" you'll get an idea of what I mean.
> 
> There's so many locks in the nwfilter code it's a wonder it all works.

Totally agreed.

> 
> It's truly unfortunate that the wrlock mechanism is undefined for
> promoting a read lock to write leaving the consumer with few options.

I believe pthread developers tried to avoid having any lock promoting
because it is hard to do right. It might break the definition of mutual
exclusion of readers and writers. I mean, imagine one thread would do:

lockRead();
lockRead();
lockPromote();

There are two possible solutions:
a) both read locks are promoted to write locks
b) only the second lock is promoted.

At any rate, either the lock is locked two times for writing, or for
reading and writing at the same time. On the other hand, since this is
done in one thread I guess it doesn't matter that much.


> 
> Since we can limit the "updating" of the fields in @obj and getting a
> write lock to very specific code paths, hopefully we can limit damage
> and/or deadlock type and/or TOCTOU issues

I'm working on what I suggested earlier - making this
virObjectRecursiveLockable. Let's see how that works out.

Michal

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

On 02/09/2018 03:41 AM, Michal Privoznik wrote:
> On 02/08/2018 04:06 PM, John Ferlan wrote:
>> [...]
>>
>>>>>> +static void
>>>>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>>>>> +{
>>>>>> +    virObjectRWUnlock(obj);
>>>>>> +    virObjectRWLockWrite(obj);
>>>>>> +}
>>>>>
>>>>> How can this not deadlock? This will work only if @obj is locked exactly
>>>>> once. But since we allow recursive locks any lock() that happens in the
>>>>> 2nd level must deadlock with this. On the other hand, there's no such
>>>>> case in the code currently. But that doesn't stop anybody from calling
>>>>> PromoteWrite() without understanding its limitations.
>>>>>
>>>>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>>>>> not a good candidate for virObjectRWLocable? It is still a good
>>>>> candidate for virObject though.
>>>>>
>>>>> Or if you want to spend extra time on this, maybe introducing
>>>>> virObjectRecursiveLockable may be worth it (terrible name, I know).
>>>>>
>>>>>
>>>>
>>>> I dunno, worked for me. The helper is being called from a thread that
>>>> has taken out a READ lock at most one time and needs to get the WRITE
>>>> lock in order to change things. If something else has the READ lock that
>>>> thread waits until the other thread releases the READ lock as far as I
>>>> understand things.
>>>
>>> Yes, I can see that. It's just that since the original lock is recursive
>>> I expected things to get recursive and thus I've been thinking how would
>>> this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
>>> places where lock promoting is used are kind of safe since all the
>>> locking is done at the first level.
>>>
>>
>> The reason I chose rwlocks and taking a read lock by default in the
>> FindByUUID and FindByName (as opposed to other vir*FindBy* API's which
>> return a write locked object) is because I know there's the potential
>> for other code to be doing Reads and rwlocks allowed for that without
>> the need to create any sort of recursive lockable object or any sort of
>> API to perform "trylock"'s (all things I tried until the epiphany to use
>> rwlocks was reached when you added them for other drivers and their list
>> protection).
>>
>> In the long/short run I figured that having a common way to get the lock
>> and then deciding to change it from Read to Write as necessary would
>> work just fine for the purpose that was needed.
> 
> This is where I disagree. I don't think it's safe to promote a lock by
> unlocking it. The moment the lock is unlocked another thread will lock
> the object and all the assumptions we made/checked about the object we
> made when we had the object locked are gone. So we would need to
> reiterate the decision making process.
> 

OK - so let's think about where/why/when we promote the lock and what
the checks/balances/consequences are.  There are 3 things being
protected by the lock in _virNWFilterObj (@wantRemoved, @def, and
@newDef). Each of promotion opportunities occur because the
virNWFilterTriggerVMFilterRebuild must be called when we attempt to
update or remove a filter.

1. virNWFilterObjTestUnassignDef

Changes the obj->wantRemoved flag. This is the only API that sets that
flag. The flag is checked by the virNWFilterObjWantRemoved that checked
as part of the virNWFilterObjListFindInstantiateFilter processing that
would would fail if it was determined the flag was set.

This essentially allows/disallows the nwfilterUndefine code to proceed
or not. The nwfilterUndefine code is further protected by a series of
locks that ensures one Undefine at a time. IOW: It's not possible to
have two threads mucking with this (from the comments in Undefine):

/* Serialization of *one* Undefine consumer is extremely important
 * as it relates to virNWFilterObjListFindInstantiateFilter processing
 * via virNWFilterTriggerVMFilterRebuild that occurs during
 * virNWFilterObjTestUnassignDef */
nwfilterDriverLock();
virNWFilterWriteLockFilterUpdates();
virNWFilterCallbackDriversLock();

2. virNWFilterObjListAssignDef

Changes to the obj->def/obj->newDef are made here.

Calls to the function are made from:

  2a. nwfilterDefineXML

Which not surprisingly has the following similar sequence:

/* Serialization of *one* DefineXML consumer is extremely important
 * as it relates to virNWFilterObjListFindInstantiateFilter processing
 * via virNWFilterTriggerVMFilterRebuild that occurs during
 * virNWFilterObjListAssignDef */
nwfilterDriverLock();
virNWFilterWriteLockFilterUpdates();
virNWFilterCallbackDriversLock();


  2b. virNWFilterObjListLoadConfig via virNWFilterObjListLoadAllConfigs

  Code path is from StateInitialization or StateReload. For Initialize I
think/hope we can agree - it's one path, no ability for define/undefine
to impede.

  For the Reload path, our similar sequence:

/* Serialization of virNWFilterObjListLoadAllConfigs is extremely
 * important as it relates to virNWFilterObjListFindInstantiateFilter
 * processing via virNWFilterTriggerVMFilterRebuild that occurs during
 * virNWFilterObjListAssignDef */
nwfilterDriverLock();
virNWFilterWriteLockFilterUpdates();
virNWFilterCallbackDriversLock();


So in the long run I'm not sure this Promotion means much of anything.
We could keep RWRead locks and not have a single chance that something
is altering the fields we're touching.  At least the Promotion shows
when we do touch fields.

>>
>>>>
>>>> Back when I first did this I had quite a lot of debug code and I have a
>>>> vague recollection of seeing output from this particular path and seeing
>>>> a couple of unlocks before the WRITE was taken while running the avocado
>>>> tests.
>>>>
>>>> I have zero interest in spending more time on this. That ship sailed. If
>>>> the decision is to drop the patches, then fine. I tried.  I disagree
>>>> that NWFilterObj is not a candidate for RWLockable - in fact it's quite
>>>> the opposite *because* of the recursive locks.
>>>
>>> Well, there's a difference between recursive locks and rwlocks. And it's
>>> exactly in what happens in recursion. With recursive locks we don't need
>>> any lock promoting and thus it's safe to do read/write after lock().
>>> With lock promoting (esp. done in unlock()+lockWrite() manner) we get
>>> TOCTOU bugs.
>>>
>>
>> If we had a real lock manager available this wouldn't be a problem ;-)
>> [sorry just my OpenVMS roots and bias showing].  Still if you read up a
>> bit on "dlm" you'll get an idea of what I mean.
>>
>> There's so many locks in the nwfilter code it's a wonder it all works.
> 
> Totally agreed.
> 
>>
>> It's truly unfortunate that the wrlock mechanism is undefined for
>> promoting a read lock to write leaving the consumer with few options.
> 
> I believe pthread developers tried to avoid having any lock promoting
> because it is hard to do right. It might break the definition of mutual
> exclusion of readers and writers. I mean, imagine one thread would do:
> 
> lockRead();
> lockRead();
> lockPromote();
> 
> There are two possible solutions:
> a) both read locks are promoted to write locks
> b) only the second lock is promoted.
> 
> At any rate, either the lock is locked two times for writing, or for
> reading and writing at the same time. On the other hand, since this is
> done in one thread I guess it doesn't matter that much.
> 
> 

Yep it's hard, but not impossible. Still a lot easier to pass the buck
to someone else to figure out how/what to do for their own application
or library needs.

The really hard part is deadlock detection problems where multiple locks
are in play. It's not the single lock that's so hard. Attempting to
promote a lock while holding a write lock where if you have to wait to
get the promotion because some other thread is doing it's lock promotion
in a different order.

>>
>> Since we can limit the "updating" of the fields in @obj and getting a
>> write lock to very specific code paths, hopefully we can limit damage
>> and/or deadlock type and/or TOCTOU issues
> 
> I'm working on what I suggested earlier - making this
> virObjectRecursiveLockable. Let's see how that works out.
> 

And why is it felt it will be different than the last time I tried this
as I noted in my last response in this thread which was below the hunk
that you snipped out, see:

https://www.redhat.com/archives/libvir-list/2017-May/msg01183.html

?

John

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

On 02/09/2018 02:00 PM, John Ferlan wrote:
> 
> 
> On 02/09/2018 03:41 AM, Michal Privoznik wrote:
>> On 02/08/2018 04:06 PM, John Ferlan wrote:
>>> [...]
>>>
>>>>>>> +static void
>>>>>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>>>>>> +{
>>>>>>> +    virObjectRWUnlock(obj);
>>>>>>> +    virObjectRWLockWrite(obj);
>>>>>>> +}
>>>>>>
>>>>>> How can this not deadlock? This will work only if @obj is locked exactly
>>>>>> once. But since we allow recursive locks any lock() that happens in the
>>>>>> 2nd level must deadlock with this. On the other hand, there's no such
>>>>>> case in the code currently. But that doesn't stop anybody from calling
>>>>>> PromoteWrite() without understanding its limitations.
>>>>>>
>>>>>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>>>>>> not a good candidate for virObjectRWLocable? It is still a good
>>>>>> candidate for virObject though.
>>>>>>
>>>>>> Or if you want to spend extra time on this, maybe introducing
>>>>>> virObjectRecursiveLockable may be worth it (terrible name, I know).
>>>>>>
>>>>>>
>>>>>
>>>>> I dunno, worked for me. The helper is being called from a thread that
>>>>> has taken out a READ lock at most one time and needs to get the WRITE
>>>>> lock in order to change things. If something else has the READ lock that
>>>>> thread waits until the other thread releases the READ lock as far as I
>>>>> understand things.
>>>>
>>>> Yes, I can see that. It's just that since the original lock is recursive
>>>> I expected things to get recursive and thus I've been thinking how would
>>>> this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
>>>> places where lock promoting is used are kind of safe since all the
>>>> locking is done at the first level.
>>>>
>>>
>>> The reason I chose rwlocks and taking a read lock by default in the
>>> FindByUUID and FindByName (as opposed to other vir*FindBy* API's which
>>> return a write locked object) is because I know there's the potential
>>> for other code to be doing Reads and rwlocks allowed for that without
>>> the need to create any sort of recursive lockable object or any sort of
>>> API to perform "trylock"'s (all things I tried until the epiphany to use
>>> rwlocks was reached when you added them for other drivers and their list
>>> protection).
>>>
>>> In the long/short run I figured that having a common way to get the lock
>>> and then deciding to change it from Read to Write as necessary would
>>> work just fine for the purpose that was needed.
>>
>> This is where I disagree. I don't think it's safe to promote a lock by
>> unlocking it. The moment the lock is unlocked another thread will lock
>> the object and all the assumptions we made/checked about the object we
>> made when we had the object locked are gone. So we would need to
>> reiterate the decision making process.
>>
> 
> OK - so let's think about where/why/when we promote the lock and what
> the checks/balances/consequences are. 

I know. I am saying that all along that with the current situation it is
safe to have lock promoting. But in general (and possibly to save future
us from nasty deadlocks) it is not. And as I say in comment for 2/3 this
code is guarded by nwfilter driver lock anyway. So there is no race
possible anyway. However, I think I have some patches ready that
implement what is needed here. I'm gonna post them shortly and give you
all the credit - they are heavily based on your work.

Michal

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

On Thu, Feb 08, 2018 at 02:13:58PM +0100, Michal Privoznik wrote:
> On 02/06/2018 08:20 PM, John Ferlan wrote:
> > Unlike it's counterparts, the nwfilter object code needs to be able
> > to support recursive read locks while processing and checking new
> > filters against the existing environment. Thus instead of using a
> > virObjectLockable which uses pure mutexes, use the virObjectRWLockable
> > and primarily use RWLockRead when obtaining the object lock since
> > RWLockRead locks can be recursively obtained (up to a limit) as long
> > as there's a corresponding unlock.
> > 
> > Since all the object management is within the virnwfilterobj code, we
> > can limit the number of Write locks on the object to very small areas
> > of code to ensure we don't run into deadlock's with other threads and
> > domain code that will check/use the filters (it's a very delicate
> > balance). This limits the write locks to AssignDef and Remove processing.
> > 
> > This patch will introduce a new API virNWFilterObjEndAPI to unlock
> > and deref the object.
> > 
> > This patch introduces two helpers to promote/demote the read/write lock.
> > 
> > Signed-off-by: John Ferlan <jferlan@redhat.com>
> > ---
> >  src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
> >  src/conf/virnwfilterobj.h              |   9 +-
> >  src/libvirt_private.syms               |   3 +-
> >  src/nwfilter/nwfilter_driver.c         |  15 +--
> >  src/nwfilter/nwfilter_gentech_driver.c |  11 +-
> >  5 files changed, 153 insertions(+), 78 deletions(-)
> > 
> > diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> > index 87d7e7270..cd52706ec 100644
> > --- a/src/conf/virnwfilterobj.c
> > +++ b/src/conf/virnwfilterobj.c
> > @@ -34,7 +34,7 @@
> >  VIR_LOG_INIT("conf.virnwfilterobj");
> >  
> >  struct _virNWFilterObj {
> > -    virMutex lock;
> > +    virObjectRWLockable parent;
> >  
> >      bool wantRemoved;
> >  
> > @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
> >      virNWFilterObjPtr *objs;
> >  };
> >  
> > +static virClassPtr virNWFilterObjClass;
> > +static void virNWFilterObjDispose(void *opaque);
> > +
> > +
> > +static int
> > +virNWFilterObjOnceInit(void)
> > +{
> > +    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
> > +                                            "virNWFilterObj",
> > +                                            sizeof(virNWFilterObj),
> > +                                            virNWFilterObjDispose)))
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
> > +
> >  
> >  static virNWFilterObjPtr
> >  virNWFilterObjNew(void)
> >  {
> >      virNWFilterObjPtr obj;
> >  
> > -    if (VIR_ALLOC(obj) < 0)
> > +    if (virNWFilterObjInitialize() < 0)
> >          return NULL;
> >  
> > -    if (virMutexInitRecursive(&obj->lock) < 0) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       "%s", _("cannot initialize mutex"));
> > -        VIR_FREE(obj);
> > +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
> >          return NULL;
> > -    }
> >  
> > -    virNWFilterObjLock(obj);
> > +    virObjectRWLockWrite(obj);
> >      return obj;
> >  }
> >  
> >  
> > +static void
> > +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
> > +{
> > +    virObjectRWUnlock(obj);
> > +    virObjectRWLockWrite(obj);
> > +}
> 
> How can this not deadlock? This will work only if @obj is locked exactly
> once. But since we allow recursive locks any lock() that happens in the
> 2nd level must deadlock with this. On the other hand, there's no such
> case in the code currently. But that doesn't stop anybody from calling
> PromoteWrite() without understanding its limitations.
> 
> Maybe the fact that we need recursive lock for NWFilterObj means it's
> not a good candidate for virObjectRWLocable? It is still a good
> candidate for virObject though.
> 
> Or if you want to spend extra time on this, maybe introducing
> virObjectRecursiveLockable may be worth it (terrible name, I know).

I can't remember exactly call trace scenarios that meant we required
recursive locking. I vaguely recall is was related to fact that we
have an alternative entry point into the nwfilter code that's triggered
by the virt drivers. I'm thus vaguely hoping that when we split nwfilter
off into a separate daemon from virt driver, the need for recursive
lockingn would magically disappear. I could be completely wrong though :-)


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

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

[...]


>>>  }
>>>  
>>>  
>>> +static void
>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>> +{
>>> +    virObjectRWUnlock(obj);
>>> +    virObjectRWLockWrite(obj);
>>> +}
>>
>> How can this not deadlock? This will work only if @obj is locked exactly
>> once. But since we allow recursive locks any lock() that happens in the
>> 2nd level must deadlock with this. On the other hand, there's no such
>> case in the code currently. But that doesn't stop anybody from calling
>> PromoteWrite() without understanding its limitations.
>>
>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>> not a good candidate for virObjectRWLocable? It is still a good
>> candidate for virObject though.
>>
>> Or if you want to spend extra time on this, maybe introducing
>> virObjectRecursiveLockable may be worth it (terrible name, I know).
> 
> I can't remember exactly call trace scenarios that meant we required
> recursive locking. I vaguely recall is was related to fact that we
> have an alternative entry point into the nwfilter code that's triggered
> by the virt drivers. I'm thus vaguely hoping that when we split nwfilter
> off into a separate daemon from virt driver, the need for recursive
> lockingn would magically disappear. I could be completely wrong though :-)
> 
> 

There's multiple recursive locks. I tried to deal only with the
NWFilterObj locks.

There some "magical" entry points with domain start/stop via
nwfilterInstantiateFilter and nwfilterTeardownFilter.... There's also
interactions via libvirtd start/stop and of course whatever magical
entry points for firewalld. Lot's of chances for issues when the
virNWFilter{ReadLock|Unlock}FilterUpdates calls are made. Finally
there's an @updateMutex in nwfilter_gentech_driver.c which truly brings
great joy and happiness to debugging lock issues.

I have this piece of paper which I tried to keep track of various locks
and paths - suffice to say it got messy very quickly.

John

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

On 02/08/2018 08:30 AM, Daniel P. Berrangé wrote:
> On Thu, Feb 08, 2018 at 02:13:58PM +0100, Michal Privoznik wrote:
>> On 02/06/2018 08:20 PM, John Ferlan wrote:
>>> Unlike it's counterparts, the nwfilter object code needs to be able
>>> to support recursive read locks while processing and checking new
>>> filters against the existing environment. Thus instead of using a
>>> virObjectLockable which uses pure mutexes, use the virObjectRWLockable
>>> and primarily use RWLockRead when obtaining the object lock since
>>> RWLockRead locks can be recursively obtained (up to a limit) as long
>>> as there's a corresponding unlock.
>>>
>>> Since all the object management is within the virnwfilterobj code, we
>>> can limit the number of Write locks on the object to very small areas
>>> of code to ensure we don't run into deadlock's with other threads and
>>> domain code that will check/use the filters (it's a very delicate
>>> balance). This limits the write locks to AssignDef and Remove processing.
>>>
>>> This patch will introduce a new API virNWFilterObjEndAPI to unlock
>>> and deref the object.
>>>
>>> This patch introduces two helpers to promote/demote the read/write lock.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> ---
>>>   src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
>>>   src/conf/virnwfilterobj.h              |   9 +-
>>>   src/libvirt_private.syms               |   3 +-
>>>   src/nwfilter/nwfilter_driver.c         |  15 +--
>>>   src/nwfilter/nwfilter_gentech_driver.c |  11 +-
>>>   5 files changed, 153 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>>> index 87d7e7270..cd52706ec 100644
>>> --- a/src/conf/virnwfilterobj.c
>>> +++ b/src/conf/virnwfilterobj.c
>>> @@ -34,7 +34,7 @@
>>>   VIR_LOG_INIT("conf.virnwfilterobj");
>>>   
>>>   struct _virNWFilterObj {
>>> -    virMutex lock;
>>> +    virObjectRWLockable parent;
>>>   
>>>       bool wantRemoved;
>>>   
>>> @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
>>>       virNWFilterObjPtr *objs;
>>>   };
>>>   
>>> +static virClassPtr virNWFilterObjClass;
>>> +static void virNWFilterObjDispose(void *opaque);
>>> +
>>> +
>>> +static int
>>> +virNWFilterObjOnceInit(void)
>>> +{
>>> +    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
>>> +                                            "virNWFilterObj",
>>> +                                            sizeof(virNWFilterObj),
>>> +                                            virNWFilterObjDispose)))
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
>>> +
>>>   
>>>   static virNWFilterObjPtr
>>>   virNWFilterObjNew(void)
>>>   {
>>>       virNWFilterObjPtr obj;
>>>   
>>> -    if (VIR_ALLOC(obj) < 0)
>>> +    if (virNWFilterObjInitialize() < 0)
>>>           return NULL;
>>>   
>>> -    if (virMutexInitRecursive(&obj->lock) < 0) {
>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -                       "%s", _("cannot initialize mutex"));
>>> -        VIR_FREE(obj);
>>> +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
>>>           return NULL;
>>> -    }
>>>   
>>> -    virNWFilterObjLock(obj);
>>> +    virObjectRWLockWrite(obj);
>>>       return obj;
>>>   }
>>>   
>>>   
>>> +static void
>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>> +{
>>> +    virObjectRWUnlock(obj);
>>> +    virObjectRWLockWrite(obj);
>>> +}
>> How can this not deadlock? This will work only if @obj is locked exactly
>> once. But since we allow recursive locks any lock() that happens in the
>> 2nd level must deadlock with this. On the other hand, there's no such
>> case in the code currently. But that doesn't stop anybody from calling
>> PromoteWrite() without understanding its limitations.
>>
>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>> not a good candidate for virObjectRWLocable? It is still a good
>> candidate for virObject though.
>>
>> Or if you want to spend extra time on this, maybe introducing
>> virObjectRecursiveLockable may be worth it (terrible name, I know).
> I can't remember exactly call trace scenarios that meant we required
> recursive locking. I vaguely recall is was related to fact that we
> have an alternative entry point into the nwfilter code that's triggered
> by the virt drivers. I'm thus vaguely hoping that when we split nwfilter
> off into a separate daemon from virt driver, the need for recursive
> lockingn would magically disappear. I could be completely wrong though :-)

The nwfilter code tries to detect circular references of filters, like 
a->b->c->a, and while testing for that needs to grab a read lock twice.

    Stefan

>
>
> Regards,
> Daniel


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