[PATCH 16/17] nwfilter: Remove recursive locking for nwfilter instantiation

[John Ferlan <jferlan@xxxxxxxxxx>]

The current algorithm required usage of recursive locks because there
was no other mechanism available to ensure that the object wasn't deleted
whilst the instantiation was taking place.

Now that we have object references, lets use those to ensure the object
isn't deleted whilst we're working through the instantiation thus removing
the need for recursive locks.

Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
 src/conf/virnwfilterobj.c              | 13 +++++++++--
 src/nwfilter/nwfilter_gentech_driver.c | 40 +++++++++++++++++++---------------
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 84ef7d1..ea1323d 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
 }
 
 
+/**
+ * To avoid the need to have recursive locks as a result of how the
+ * virNWFilterInstantiateFilter processing works, this API will not
+ * lock the returned object, instead just increase the refcnt of the
+ * object to the caller to allow the caller to handle.
+ */
 virNWFilterObjPtr
 virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
                                         const char *filtername)
 {
     virNWFilterObjPtr obj;
 
-    if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) {
+    virObjectLock(nwfilters);
+    obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername);
+    virObjectUnlock(nwfilters);
+    if (!obj) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("referenced filter '%s' is missing"), filtername);
         return NULL;
@@ -318,7 +327,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
     if (virNWFilterObjWantRemoved(obj)) {
         virReportError(VIR_ERR_NO_NWFILTER,
                        _("Filter '%s' is in use."), filtername);
-        virNWFilterObjEndAPI(&obj);
+        virNWFilterObjUnref(obj);
         return NULL;
     }
 
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
index 4c228ea..40da4cc 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -56,19 +56,25 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = {
     NULL
 };
 
-/* Serializes instantiation of filters. This is necessary
- * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate
- * will hold a lock on a virNWFilterObjPtr. This in turn invokes
- * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec
- * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over
- * every single virNWFilterObjPtr in the list. So if 2 threads try to
- * instantiate a filter in parallel, they'll both hold 1 lock at the top level
- * in virNWFilterInstantiateFilterUpdate which will cause the other thread
- * to deadlock in virNWFilterObjListFindInstantiateFilter.
+/* NB: Usage of virNWFilterObjListFindInstantiateFilter will only not lock
+ * the returned object - only increase the refcnt. This is necessary to avoid
+ * lock ordering deadlocks or the need for recursive locks for filter objects.
+ * Since the objects are only used to access the @def for <filterref> and
+ * <rule> elements and having the @def disappear would not be good, but
+ * keeping the locks causes too many problems.
  *
- * XXX better long term solution is to make virNWFilterObjList use a
- * hash table as is done for virDomainObjList. You can then get
- * lockless lookup of objects by name.
+ * During virNWFilterDefToInst processing an object is fetched and placed
+ * into the inst->filters lookaside list during virNWFilterIncludeDefToRuleInst
+ * processing which then recursively calls virNWFilterDefToInst. When the
+ * object is removed during virNWFilterInstReset, the refcnt is decremented.
+ *
+ * Since virNWFilterDefToInst is called during virNWFilterDoInstantiate
+ * processing which can also invoke virNWFilterDetermineMissingVarsRec
+ * which invokes virNWFilterObjListFindInstantiateFilter also taking
+ * reference. In addition the virNWFilterInstantiateFilterUpdate could
+ * have caused locking collisions, so removing the locks from the
+ * equation and replacing with just references ensures that the object
+ * cannot be removed whilst this instantiation is taking place.
  */
 static virMutex updateMutex;
 
@@ -315,7 +321,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst)
     size_t i;
 
     for (i = 0; i < inst->nfilters; i++)
-        virNWFilterObjEndAPI(&inst->filters[i]);
+        virNWFilterObjUnref(inst->filters[i]);
     VIR_FREE(inst->filters);
     inst->nfilters = 0;
 
@@ -425,7 +431,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver,
     if (ret < 0)
         virNWFilterInstReset(inst);
     virNWFilterHashTableFree(tmpvars);
-    virNWFilterObjEndAPI(&obj);
+    virNWFilterObjUnref(obj);
     return ret;
 }
 
@@ -546,7 +552,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
                                                             vars);
             if (!tmpvars) {
                 rc = -1;
-                virNWFilterObjEndAPI(&obj);
+                virNWFilterObjUnref(obj);
                 break;
             }
 
@@ -570,7 +576,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
 
             virNWFilterHashTableFree(tmpvars);
 
-            virNWFilterObjEndAPI(&obj);
+            virNWFilterObjUnref(obj);
             if (rc < 0)
                 break;
         }
@@ -844,7 +850,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver,
     virNWFilterHashTableFree(vars1);
 
  err_exit:
-    virNWFilterObjEndAPI(&obj);
+    virNWFilterObjUnref(obj);
 
     VIR_FREE(str_ipaddr);
     VIR_FREE(str_macaddr);
-- 
2.9.4

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