The comment against the 'updateMutex' refers to a problem with
lock ordering when looking up filters in the virNWFilterObjList
which uses an array. That problem does indeed exist.
Unfortunately it claims that switching to a hash table would
solve the lock ordering problems during instantiation. That
is not correct because there is a second lock ordering
problem related to how we traverse related filters when
instantiating filters. Consider a set of filters:
Filter A:
Reference Filter C
Reference Filter D
Filter B:
Reference Filter D
Reference Filter C
In one example, we lock A, C, D, in the other example
we lock A, D, C.
Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
---
src/nwfilter/nwfilter_gentech_driver.c | 57 ++++++++++++++++++++------
1 file changed, 45 insertions(+), 12 deletions(-)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
index 7bbf1e12fb..9f4a755252 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -55,19 +55,52 @@ static virNWFilterTechDriver *filter_tech_drivers[] = {
NULL
};
-/* Serializes instantiation of filters. This is necessary
- * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate
- * will hold a lock on a virNWFilterObj *. This in turn invokes
- * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec
- * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over
- * every single virNWFilterObj *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.
+/*
+ * Serializes instantiation of filters.
+ *
+ * When instantiating a filter, we need to resolve references
+ * to other filters and acquire locks on them. The act of
+ * looking up a filter requires traversing an array, locking
+ * each filter in turn until we find the one we want.
+ *
+ * The mere act of finding a filter by name, while holding
+ * a lock on another filter, introduces deadlocks due to
+ * varying lock ordering.
+ *
+ * We retain a lock on the referenced filter once found.
+ * The order in which the locks are acquired depends on
+ * the order in which filters reference each other.
+ *
+ * Filter A:
+ * Reference Filter C
+ * Reference Filter D
+ *
+ * Filter B:
+ * Reference Filter D
+ * Reference Filter C
+ *
+ * In one example, we lock A, C, D, in the other example
+ * we lock A, D, C.
+ *
+ * Because C & D are locked in differing orders we are
+ * once again at risk of deadlocks.
+ *
+ * There can be multiple levels of recursion, so it is
+ * not viable to determine the lock order upfront, it
+ * has to be done as we traverse the tree.
+ *
+ * Thus we serialize any code that needs to traverse
+ * the filter references.
+ *
+ * This covers the following APIs:
+ *
+ * virNWFilterDefineXML
+ * virNWFilterUndefine
+ * virNWFilterBindingCreate
+ * virNWFilterBindingDelete
*
- * 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.
+ * In addition to the asynchronous filter instantiation
+ * triggered by the IP address learning backends.
*/
static virMutex updateMutex;
--
2.35.1
