diff mbox series

[ovs-dev,v3,2/8] northd: Add thread safety analysis to lflow hash locks.

Message ID 20230209180111.2214872-3-i.maximets@ovn.org
State Accepted
Headers show
Series northd: Hash locks and lflow creation with dp groups. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_ovn-kubernetes success github build: passed
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Ilya Maximets Feb. 9, 2023, 6:01 p.m. UTC 
Even though it's not possible to enable a full thread-safety analysis
due to conditional mutex taking and unpredictable order [1], we can add
most of the functionality with a fake mutex.  It can detect nesting
of hash locks, incorrect locking sequences and some other issues.
More details are given in a form of a comment in the code to give this
fake mutex more visibility.

The fake mutex is declared as extern to not actually use any memory
or trigger 'unused' warnings.

No variables or structure fields are marked as GUARDED, because we
still need a lockless access once the parallel part is over.

[1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-conditionally-held-locks

Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 northd/northd.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index f5e18ef15..4ddfc3af3 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5112,8 +5112,28 @@  lflow_hash_lock_destroy(void)
     lflow_hash_lock_initialized = false;
 }
 
+/* Full thread safety analysis is not possible with hash locks, because
+ * they are taken conditionally based on the 'parallelization_state' and
+ * a flow hash.  Also, the order in which two hash locks are taken is not
+ * predictable during the static analysis.
+ *
+ * Since the order of taking two locks depends on a random hash, to avoid
+ * ABBA deadlocks, no two hash locks can be nested.  In that sense an array
+ * of hash locks is similar to a single mutex.
+ *
+ * Using a fake mutex to partially simulate thread safety restrictions, as
+ * if it were actually a single mutex.
+ *
+ * OVS_NO_THREAD_SAFETY_ANALYSIS below allows us to ignore conditional
+ * nature of the lock.  Unlike other attributes, it applies to the
+ * implementation and not to the interface.  So, we can define a function
+ * that acquires the lock without analysing the way it does that.
+ */
+extern struct ovs_mutex fake_hash_mutex;
+
 static struct ovs_mutex *
 lflow_hash_lock(const struct hmap *lflow_map, uint32_t hash)
+    OVS_ACQUIRES(fake_hash_mutex)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     struct ovs_mutex *hash_lock = NULL;
@@ -5128,6 +5148,7 @@  lflow_hash_lock(const struct hmap *lflow_map, uint32_t hash)
 
 static void
 lflow_hash_unlock(struct ovs_mutex *hash_lock)
+    OVS_RELEASES(fake_hash_mutex)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     if (hash_lock) {
@@ -5154,7 +5175,7 @@  static thread_local size_t thread_lflow_counter = 0;
 static bool
 ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
                                 struct ovn_datapath *od)
-    OVS_NO_THREAD_SAFETY_ANALYSIS
+    OVS_REQUIRES(fake_hash_mutex)
 {
     if (!lflow_ref) {
         return false;
@@ -5173,6 +5194,7 @@  do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,
                  const char *match, const char *actions, const char *io_port,
                  const struct ovsdb_idl_row *stage_hint,
                  const char *where, const char *ctrl_meter)
+    OVS_REQUIRES(fake_hash_mutex)
 {
 
     struct ovn_lflow *old_lflow;
@@ -5214,6 +5236,7 @@  ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od,
                            const char *io_port, const char *ctrl_meter,
                            const struct ovsdb_idl_row *stage_hint,
                            const char *where, uint32_t hash)
+    OVS_REQUIRES(fake_hash_mutex)
 {
     struct ovn_lflow *lflow;
 
@@ -5231,6 +5254,7 @@  ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
                  const char *match, const char *actions, const char *io_port,
                  const char *ctrl_meter,
                  const struct ovsdb_idl_row *stage_hint, const char *where)
+    OVS_EXCLUDED(fake_hash_mutex)
 {
     struct ovs_mutex *hash_lock;
     uint32_t hash;