[libvirt] [PATCH v3 3/5] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable

Thursday, 5 October 2017

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(a)redhat.com&gt;
---
 src/conf/virnwfilterobj.c              | 191 +++++++++++++++++++++++----------
 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, 149 insertions(+), 80 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 87d7e72703..6b4758656f 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,8 +265,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr
nwfilters,
     if (virNWFilterObjWantRemoved(obj)) {
         virReportError(VIR_ERR_NO_NWFILTER,
                        _("Filter '%s' is in use."), filtername);
-        virNWFilterObjUnlock(obj);
-        return NULL;
+        virNWFilterObjEndAPI(&obj);
     }
 
     return obj;
@@ -240,7 +299,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
             if (obj) {
                 rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def,
                                                       filtername);
-                virNWFilterObjUnlock(obj);
+                virNWFilterObjEndAPI(&obj);
                 if (rc < 0)
                     break;
             }
@@ -269,17 +328,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 +400,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 +413,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,7 +425,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
     }
 
 
+    /* Get a READ lock and immediately promote to WRITE while we adjust
+     * data within. */
     if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
+        virNWFilterObjPromoteToWrite(obj);
 
         objdef = obj->def;
         if (virNWFilterDefEqual(def, objdef)) {
@@ -357,13 +438,20 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
         }
 
         obj->newDef = def;
+
+        /* Demote while the trigger runs since it may need to grab a read
+         * lock on this object and promote before returning. */
+        virNWFilterObjDemoteFromWrite(obj);
+
         /* trigger the update on VMs referencing the filter */
         if (virNWFilterTriggerVMFilterRebuild() < 0) {
+            virNWFilterObjPromoteToWrite(obj);
             obj->newDef = NULL;
-            virNWFilterObjUnlock(obj);
+            virNWFilterObjEndAPI(&obj);
             return NULL;
         }
 
+        virNWFilterObjPromoteToWrite(obj);
         virNWFilterDefFree(objdef);
         obj->def = def;
         obj->newDef = NULL;
@@ -375,13 +463,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
 
     if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
                                 nwfilters->count, obj) < 0) {
-        virNWFilterObjUnlock(obj);
-        virNWFilterObjFree(obj);
+        virNWFilterObjEndAPI(&obj);
         return NULL;
     }
     obj->def = def;
 
-    return obj;
+    return virObjectRef(obj);
 }
 
 
@@ -395,10 +482,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 +505,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 +551,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 +638,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 8e79518ed3..0281bc5f56 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 7a12d6a146..d91fa92e4b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1004,6 +1004,7 @@ virNodeDeviceObjListRemove;
 
 
 # conf/virnwfilterobj.h
+virNWFilterObjEndAPI;
 virNWFilterObjGetDef;
 virNWFilterObjGetNewDef;
 virNWFilterObjListAssignDef;
@@ -1017,9 +1018,7 @@ virNWFilterObjListLoadAllConfigs;
 virNWFilterObjListNew;
 virNWFilterObjListNumOfNWFilters;
 virNWFilterObjListRemove;
-virNWFilterObjLock;
 virNWFilterObjTestUnassignDef;
-virNWFilterObjUnlock;
 virNWFilterObjWantRemoved;
 
 
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 06d5847ca7..a1d93f7af0 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -406,7 +406,7 @@ nwfilterLookupByUUID(virConnectPtr conn,
     nwfilter = virGetNWFilter(conn, def->name, def->uuid);
 
  cleanup:
-    virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
     return nwfilter;
 }
 
@@ -436,7 +436,7 @@ nwfilterLookupByName(virConnectPtr conn,
     nwfilter = virGetNWFilter(conn, def->name, def->uuid);
 
  cleanup:
-    virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
     return nwfilter;
 }
 
@@ -523,6 +523,8 @@ nwfilterDefineXML(virConnectPtr conn,
 
     if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) {
         virNWFilterObjListRemove(driver->nwfilters, obj);
+        virObjectUnref(obj);
+        obj = NULL;
         goto cleanup;
     }
 
@@ -530,8 +532,7 @@ nwfilterDefineXML(virConnectPtr conn,
 
  cleanup:
     virNWFilterDefFree(def);
-    if (obj)
-        virNWFilterObjUnlock(obj);
+    virNWFilterObjEndAPI(&obj);
 
     virNWFilterCallbackDriversUnlock();
     virNWFilterUnlockFilterUpdates();
@@ -569,12 +570,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();
@@ -607,7 +608,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 840d419bb4..48d0e1769f 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


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH v3 3/5] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable