[ovs-dev] [PATCH v3 2/7] lib/classifier: Simplify iteration with C99 declaration.

Jarno Rajahalme jrajahalme at nicira.com
Mon Jul 7 21:26:44 UTC 2014


Hide the cursor from the classifier iteration users and move locking to
the iterators.  This will make following RCU changes simpler, as the call
sites of the iterators need not be changed at that point.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
v3: No change.

 lib/classifier.c        |   70 +++++++++++++++++++++++++++++++++--------------
 lib/classifier.h        |   66 ++++++++++++++++++++++++++++++++++----------
 ofproto/ofproto-dpif.c  |    7 +----
 ofproto/ofproto.c       |   42 +++++-----------------------
 tests/test-classifier.c |   67 ++++++++++++++++++++++++++++-----------------
 utilities/ovs-ofctl.c   |   23 ++++------------
 6 files changed, 156 insertions(+), 119 deletions(-)

diff --git a/lib/classifier.c b/lib/classifier.c
index 69b5abd..5380c80 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -588,11 +588,12 @@ trie_init(struct cls_classifier *cls, int trie_idx,
     }
 }
 
-/* Returns true if 'cls' contains no classification rules, false otherwise. */
+/* Returns true if 'cls' contains no classification rules, false otherwise.
+ * Checking the cmap requires no locking. */
 bool
 classifier_is_empty(const struct classifier *cls)
 {
-    return cls->cls->n_rules == 0;
+    return cmap_is_empty(&cls->cls->subtables_map);
 }
 
 /* Returns the number of rules in 'cls'. */
@@ -1154,7 +1155,8 @@ search_subtable(const struct cls_subtable *subtable,
     return NULL;
 }
 
-/* Initializes 'cursor' for iterating through rules in 'cls':
+/* Initializes 'cursor' for iterating through rules in 'cls', and returns the
+ * first matching cls_rule via '*pnode', or NULL if there are no matches.
  *
  *     - If 'target' is null, the cursor will visit every rule in 'cls'.
  *
@@ -1162,45 +1164,68 @@ search_subtable(const struct cls_subtable *subtable,
  *       such that cls_rule_is_loose_match(rule, target) returns true.
  *
  * Ignores target->priority. */
-void
-cls_cursor_init(struct cls_cursor *cursor, const struct classifier *cls,
-                const struct cls_rule *target)
-{
-    cursor->cls = cls->cls;
-    cursor->target = target && !cls_rule_is_catchall(target) ? target : NULL;
-}
-
-/* Returns the first matching cls_rule in 'cursor''s iteration, or a null
- * pointer if there are no matches. */
-struct cls_rule *
-cls_cursor_first(struct cls_cursor *cursor)
+struct cls_cursor cls_cursor_init(const struct classifier *cls,
+                                  const struct cls_rule *target,
+                                  void **pnode, const void *offset, bool safe)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
+    struct cls_cursor cursor;
     struct cls_subtable *subtable;
+    struct cls_rule *cls_rule = NULL;
 
-    CMAP_CURSOR_FOR_EACH (subtable, cmap_node, &cursor->subtables,
-                          &cursor->cls->subtables_map) {
-        struct cls_match *rule = search_subtable(subtable, cursor);
+    cursor.safe = safe;
+    cursor.cls = cls;
+    cursor.target = target && !cls_rule_is_catchall(target) ? target : NULL;
+
+    /* Find first rule. */
+    fat_rwlock_rdlock(&cursor.cls->rwlock);
+    CMAP_CURSOR_FOR_EACH (subtable, cmap_node, &cursor.subtables,
+                          &cursor.cls->cls->subtables_map) {
+        struct cls_match *rule = search_subtable(subtable, &cursor);
 
         if (rule) {
-            cursor->subtable = subtable;
-            return rule->cls_rule;
+            cursor.subtable = subtable;
+            cls_rule = rule->cls_rule;
+            break;
         }
     }
+    *pnode = (char *)cls_rule + (ptrdiff_t)offset;
 
-    return NULL;
+    /* Leave locked if requested and have a rule. */
+    if (safe || !cls_rule) {
+        fat_rwlock_unlock(&cls->rwlock);
+    }
+    return cursor;
+}
+
+static void
+cls_cursor_next_unlock(struct cls_cursor *cursor, struct cls_rule *rule)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
+{
+    /* Release the lock if no rule, or 'safe' mode. */
+    if (!rule || cursor->safe) {
+        fat_rwlock_unlock(&cursor->cls->rwlock);
+    }
 }
 
 /* Returns the next matching cls_rule in 'cursor''s iteration, or a null
  * pointer if there are no more matches. */
 struct cls_rule *
 cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     struct cls_match *rule = CONST_CAST(struct cls_match *, rule_->cls_match);
     const struct cls_subtable *subtable;
     struct cls_match *next;
 
+    /* Lock if not locked already. */
+    if (cursor->safe) {
+        fat_rwlock_rdlock(&cursor->cls->rwlock);
+    }
+
     next = next_rule_in_list__(rule);
     if (next->priority < rule->priority) {
+        cls_cursor_next_unlock(cursor, next->cls_rule);
         return next->cls_rule;
     }
 
@@ -1210,6 +1235,7 @@ cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
     rule = next;
     CMAP_CURSOR_FOR_EACH_CONTINUE (rule, cmap_node, &cursor->rules) {
         if (rule_matches(rule, cursor->target)) {
+            cls_cursor_next_unlock(cursor, rule->cls_rule);
             return rule->cls_rule;
         }
     }
@@ -1219,10 +1245,12 @@ cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
         rule = search_subtable(subtable, cursor);
         if (rule) {
             cursor->subtable = subtable;
+            cls_cursor_next_unlock(cursor, rule->cls_rule);
             return rule->cls_rule;
         }
     }
 
+    fat_rwlock_unlock(&cursor->cls->rwlock);
     return NULL;
 }
 
diff --git a/lib/classifier.h b/lib/classifier.h
index 2964755..4e1fe21 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -273,8 +273,7 @@ void classifier_set_prefix_fields(struct classifier *cls,
                                   unsigned int n_trie_fields)
     OVS_REQ_WRLOCK(cls->rwlock);
 
-bool classifier_is_empty(const struct classifier *cls)
-    OVS_REQ_RDLOCK(cls->rwlock);
+bool classifier_is_empty(const struct classifier *cls);
 int classifier_count(const struct classifier *cls)
     OVS_REQ_RDLOCK(cls->rwlock);
 void classifier_insert(struct classifier *cls, struct cls_rule *)
@@ -306,32 +305,69 @@ struct cls_rule *classifier_find_match_exactly(const struct classifier *cls,
 /* Iteration. */
 
 struct cls_cursor {
-    const struct cls_classifier *cls;
+    const struct classifier *cls;
     const struct cls_subtable *subtable;
     const struct cls_rule *target;
     struct cmap_cursor subtables;
     struct cmap_cursor rules;
+    bool safe;
 };
 
-void cls_cursor_init(struct cls_cursor *cursor, const struct classifier *cls,
-                     const struct cls_rule *match) OVS_REQ_RDLOCK(cls->rwlock);
-struct cls_rule *cls_cursor_first(struct cls_cursor *cursor);
-struct cls_rule *cls_cursor_next(struct cls_cursor *, const struct cls_rule *);
+/* Iteration requires mutual exclusion of the writers.  We do this by taking
+ * the classifier read lock for the duration of the iteration, except for the
+ * 'SAFE' variant, where we release the lock for the body of the loop. */
+struct cls_cursor cls_cursor_init(const struct classifier *cls,
+                                  const struct cls_rule *target,
+                                  void **pnode, const void *offset, bool safe);
+
+struct cls_rule *cls_cursor_next(struct cls_cursor *cursor,
+                                 const struct cls_rule *);
+
+#define CLS_CURSOR_START(RULE, MEMBER, CLS, TARGET)                     \
+    cls_cursor_init(CLS, (TARGET), (void **)&(RULE),                    \
+                    OBJECT_CONTAINING(NULL, RULE, MEMBER), false)
 
-#define CLS_CURSOR_FOR_EACH(RULE, MEMBER, CURSOR)                       \
-    for (ASSIGN_CONTAINER(RULE, cls_cursor_first(CURSOR), MEMBER);      \
+#define CLS_CURSOR_START_SAFE(RULE, MEMBER, CLS, TARGET)                \
+    cls_cursor_init(CLS, (TARGET), (void **)&(RULE),                    \
+                    OBJECT_CONTAINING(NULL, RULE, MEMBER), true)
+
+#define CLS_FOR_EACH(RULE, MEMBER, CLS)                                 \
+    for (struct cls_cursor cursor__ = CLS_CURSOR_START(RULE, MEMBER, CLS, \
+                                                       NULL);           \
          RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER);                 \
-         ASSIGN_CONTAINER(RULE, cls_cursor_next(CURSOR, &(RULE)->MEMBER), \
+         ASSIGN_CONTAINER(RULE, cls_cursor_next(&cursor__, &(RULE)->MEMBER), \
                           MEMBER))
 
-#define CLS_CURSOR_FOR_EACH_SAFE(RULE, NEXT, MEMBER, CURSOR)            \
-    for (ASSIGN_CONTAINER(RULE, cls_cursor_first(CURSOR), MEMBER);      \
+#define CLS_FOR_EACH_TARGET(RULE, MEMBER, CLS, TARGET)                  \
+    for (struct cls_cursor cursor__ = CLS_CURSOR_START(RULE, MEMBER, CLS, \
+                                                       TARGET);         \
+         RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER);                 \
+         ASSIGN_CONTAINER(RULE, cls_cursor_next(&cursor__, &(RULE)->MEMBER), \
+                          MEMBER))
+
+/* This form allows classifier_remove() to be called within the loop. */
+#define CLS_FOR_EACH_SAFE(RULE, NEXT, MEMBER, CLS)                      \
+    for (struct cls_cursor cursor__ = CLS_CURSOR_START_SAFE(RULE, MEMBER, \
+                                                            CLS, NULL); \
          (RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER)                 \
-          ? ASSIGN_CONTAINER(NEXT, cls_cursor_next(CURSOR, &(RULE)->MEMBER), \
-                             MEMBER), 1                                 \
-          : 0);                                                         \
+          ? ASSIGN_CONTAINER(NEXT, cls_cursor_next(&cursor__,           \
+                                                   &(RULE)->MEMBER),    \
+                             MEMBER), true                              \
+          : false);                                                     \
          (RULE) = (NEXT))
 
+/* This form allows classifier_remove() to be called within the loop. */
+#define CLS_FOR_EACH_TARGET_SAFE(RULE, NEXT, MEMBER, CLS, TARGET)       \
+    for (struct cls_cursor cursor__ = CLS_CURSOR_START_SAFE(RULE, MEMBER, \
+                                                            CLS, TARGET); \
+         (RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER)                 \
+          ? ASSIGN_CONTAINER(NEXT, cls_cursor_next(&cursor__,           \
+                                                   &(RULE)->MEMBER),    \
+                             MEMBER), true                              \
+          : false);                                                     \
+         (RULE) = (NEXT))
+
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c86a3f6..5aa597e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1297,12 +1297,7 @@ destruct(struct ofproto *ofproto_)
     hmap_remove(&all_ofproto_dpifs, &ofproto->all_ofproto_dpifs_node);
 
     OFPROTO_FOR_EACH_TABLE (table, &ofproto->up) {
-        struct cls_cursor cursor;
-
-        fat_rwlock_rdlock(&table->cls.rwlock);
-        cls_cursor_init(&cursor, &table->cls, NULL);
-        fat_rwlock_unlock(&table->cls.rwlock);
-        CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, up.cr, &cursor) {
+        CLS_FOR_EACH_SAFE (rule, next_rule, up.cr, &table->cls) {
             ofproto_rule_delete(&ofproto->up, &rule->up);
         }
     }
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bf4be5e..af64f71 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1283,16 +1283,12 @@ ofproto_flush__(struct ofproto *ofproto)
     ovs_mutex_lock(&ofproto_mutex);
     OFPROTO_FOR_EACH_TABLE (table, ofproto) {
         struct rule *rule, *next_rule;
-        struct cls_cursor cursor;
 
         if (table->flags & OFTABLE_HIDDEN) {
             continue;
         }
 
-        fat_rwlock_rdlock(&table->cls.rwlock);
-        cls_cursor_init(&cursor, &table->cls, NULL);
-        fat_rwlock_unlock(&table->cls.rwlock);
-        CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cr, &cursor) {
+        CLS_FOR_EACH_SAFE (rule, next_rule, cr, &table->cls) {
             ofproto_rule_delete__(rule, OFPRR_DELETE);
         }
     }
@@ -1456,7 +1452,6 @@ ofproto_run(struct ofproto *p)
         for (i = 0; i < p->n_tables; i++) {
             struct oftable *table = &p->tables[i];
             struct eviction_group *evg;
-            struct cls_cursor cursor;
             struct rule *rule;
 
             if (!table->eviction_fields) {
@@ -1464,9 +1459,7 @@ ofproto_run(struct ofproto *p)
             }
 
             ovs_mutex_lock(&ofproto_mutex);
-            fat_rwlock_rdlock(&table->cls.rwlock);
-            cls_cursor_init(&cursor, &table->cls, NULL);
-            CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
+            CLS_FOR_EACH (rule, cr, &table->cls) {
                 if (rule->idle_timeout || rule->hard_timeout) {
                     if (!rule->eviction_group) {
                         eviction_group_add_rule(rule);
@@ -1476,7 +1469,6 @@ ofproto_run(struct ofproto *p)
                     }
                 }
             }
-            fat_rwlock_unlock(&table->cls.rwlock);
 
             HEAP_FOR_EACH (evg, size_node, &table->eviction_groups_by_size) {
                 heap_rebuild(&evg->rules);
@@ -3501,15 +3493,11 @@ collect_rules_loose(struct ofproto *ofproto,
         }
     } else {
         FOR_EACH_MATCHING_TABLE (table, criteria->table_id, ofproto) {
-            struct cls_cursor cursor;
             struct rule *rule;
 
-            fat_rwlock_rdlock(&table->cls.rwlock);
-            cls_cursor_init(&cursor, &table->cls, &criteria->cr);
-            CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
+            CLS_FOR_EACH_TARGET (rule, cr, &table->cls, &criteria->cr) {
                 collect_rule(rule, criteria, rules, &n_readonly);
             }
-            fat_rwlock_unlock(&table->cls.rwlock);
         }
     }
 
@@ -3708,15 +3696,11 @@ ofproto_get_all_flows(struct ofproto *p, struct ds *results)
     struct oftable *table;
 
     OFPROTO_FOR_EACH_TABLE (table, p) {
-        struct cls_cursor cursor;
         struct rule *rule;
 
-        fat_rwlock_rdlock(&table->cls.rwlock);
-        cls_cursor_init(&cursor, &table->cls, NULL);
-        CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
+        CLS_FOR_EACH (rule, cr, &table->cls) {
             flow_stats_ds(rule, results);
         }
-        fat_rwlock_unlock(&table->cls.rwlock);
     }
 }
 
@@ -4832,15 +4816,11 @@ ofproto_collect_ofmonitor_refresh_rules(const struct ofmonitor *m,
 
     cls_rule_init_from_minimatch(&target, &m->match, 0);
     FOR_EACH_MATCHING_TABLE (table, m->table_id, ofproto) {
-        struct cls_cursor cursor;
         struct rule *rule;
 
-        fat_rwlock_rdlock(&table->cls.rwlock);
-        cls_cursor_init(&cursor, &table->cls, &target);
-        CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
+        CLS_FOR_EACH_TARGET (rule, cr, &table->cls, &target) {
             ofproto_collect_ofmonitor_refresh_rule(m, rule, seqno, rules);
         }
-        fat_rwlock_unlock(&table->cls.rwlock);
     }
     cls_rule_destroy(&target);
 }
@@ -6468,7 +6448,6 @@ oftable_enable_eviction(struct oftable *table,
                         const struct mf_subfield *fields, size_t n_fields)
     OVS_REQUIRES(ofproto_mutex)
 {
-    struct cls_cursor cursor;
     struct rule *rule;
 
     if (table->eviction_fields
@@ -6489,12 +6468,9 @@ oftable_enable_eviction(struct oftable *table,
     hmap_init(&table->eviction_groups_by_id);
     heap_init(&table->eviction_groups_by_size);
 
-    fat_rwlock_rdlock(&table->cls.rwlock);
-    cls_cursor_init(&cursor, &table->cls, NULL);
-    CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
+    CLS_FOR_EACH (rule, cr, &table->cls) {
         eviction_group_add_rule(rule);
     }
-    fat_rwlock_unlock(&table->cls.rwlock);
 }
 
 /* Removes 'rule' from the oftable that contains it. */
@@ -6596,12 +6572,9 @@ ofproto_get_vlan_usage(struct ofproto *ofproto, unsigned long int *vlan_bitmap)
     ofproto->vlans_changed = false;
 
     OFPROTO_FOR_EACH_TABLE (oftable, ofproto) {
-        struct cls_cursor cursor;
         struct rule *rule;
 
-        fat_rwlock_rdlock(&oftable->cls.rwlock);
-        cls_cursor_init(&cursor, &oftable->cls, &target);
-        CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
+        CLS_FOR_EACH_TARGET (rule, cr, &oftable->cls, &target) {
             if (minimask_get_vid_mask(&rule->cr.match.mask) == VLAN_VID_MASK) {
                 uint16_t vid = miniflow_get_vid(&rule->cr.match.flow);
 
@@ -6609,7 +6582,6 @@ ofproto_get_vlan_usage(struct ofproto *ofproto, unsigned long int *vlan_bitmap)
                 bitmap_set1(ofproto->vlan_bitmap, vid);
             }
         }
-        fat_rwlock_unlock(&oftable->cls.rwlock);
     }
 
     cls_rule_destroy(&target);
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index 7179a09..8c0c1bf 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -454,15 +454,13 @@ static void
 destroy_classifier(struct classifier *cls)
 {
     struct test_rule *rule, *next_rule;
-    struct cls_cursor cursor;
 
-    fat_rwlock_wrlock(&cls->rwlock);
-    cls_cursor_init(&cursor, cls, NULL);
-    CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
+    CLS_FOR_EACH_SAFE (rule, next_rule, cls_rule, cls) {
+        fat_rwlock_wrlock(&cls->rwlock);
         classifier_remove(cls, &rule->cls_rule);
+        fat_rwlock_unlock(&cls->rwlock);
         free_rule(rule);
     }
-    fat_rwlock_unlock(&cls->rwlock);
     classifier_destroy(cls);
 }
 
@@ -484,11 +482,10 @@ pvector_verify(struct pvector *pvec)
 
 static void
 check_tables(const struct classifier *cls, int n_tables, int n_rules,
-             int n_dups) OVS_REQ_RDLOCK(cls->rwlock)
+             int n_dups) OVS_EXCLUDED(cls->rwlock)
 {
     const struct cls_subtable *table;
     struct test_rule *test_rule;
-    struct cls_cursor cursor;
     int found_tables = 0;
     int found_rules = 0;
     int found_dups = 0;
@@ -539,8 +536,10 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
                 prev_priority = rule->priority;
                 found_rules++;
                 found_dups++;
+                fat_rwlock_rdlock(&cls->rwlock);
                 assert(classifier_find_rule_exactly(cls, rule->cls_rule)
                        == rule->cls_rule);
+                fat_rwlock_unlock(&cls->rwlock);
             }
         }
         assert(table->max_priority == max_priority);
@@ -553,8 +552,7 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
     assert(n_rules == -1 || found_rules == n_rules);
     assert(n_dups == -1 || found_dups == n_dups);
 
-    cls_cursor_init(&cursor, cls, NULL);
-    CLS_CURSOR_FOR_EACH (test_rule, cls_rule, &cursor) {
+    CLS_FOR_EACH (test_rule, cls_rule, cls) {
         found_rules2++;
     }
     assert(found_rules == found_rules2);
@@ -704,17 +702,19 @@ test_single_rule(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
 
         tcls_rule = tcls_insert(&tcls, rule);
         classifier_insert(&cls, &rule->cls_rule);
-        check_tables(&cls, 1, 1, 0);
         compare_classifiers(&cls, &tcls);
+        fat_rwlock_unlock(&cls.rwlock);
+        check_tables(&cls, 1, 1, 0);
 
+        fat_rwlock_wrlock(&cls.rwlock);
         classifier_remove(&cls, &rule->cls_rule);
         tcls_remove(&tcls, tcls_rule);
         assert(classifier_is_empty(&cls));
         assert(tcls_is_empty(&tcls));
         compare_classifiers(&cls, &tcls);
+        fat_rwlock_unlock(&cls.rwlock);
 
         free_rule(rule);
-        fat_rwlock_unlock(&cls.rwlock);
         classifier_destroy(&cls);
         tcls_destroy(&tcls);
     }
@@ -744,19 +744,23 @@ test_rule_replacement(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         tcls_init(&tcls);
         tcls_insert(&tcls, rule1);
         classifier_insert(&cls, &rule1->cls_rule);
-        check_tables(&cls, 1, 1, 0);
         compare_classifiers(&cls, &tcls);
+        fat_rwlock_unlock(&cls.rwlock);
+        check_tables(&cls, 1, 1, 0);
         tcls_destroy(&tcls);
 
         tcls_init(&tcls);
         tcls_insert(&tcls, rule2);
+
+        fat_rwlock_wrlock(&cls.rwlock);
         assert(test_rule_from_cls_rule(
                    classifier_replace(&cls, &rule2->cls_rule)) == rule1);
         free_rule(rule1);
-        check_tables(&cls, 1, 1, 0);
         compare_classifiers(&cls, &tcls);
-        tcls_destroy(&tcls);
         fat_rwlock_unlock(&cls.rwlock);
+        check_tables(&cls, 1, 1, 0);
+
+        tcls_destroy(&tcls);
         destroy_classifier(&cls);
     }
 }
@@ -855,12 +859,14 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
             fat_rwlock_wrlock(&cls.rwlock);
             classifier_set_prefix_fields(&cls, trie_fields,
                                          ARRAY_SIZE(trie_fields));
+            fat_rwlock_unlock(&cls.rwlock);
             tcls_init(&tcls);
 
             for (i = 0; i < ARRAY_SIZE(ops); i++) {
                 int j = ops[i];
                 int m, n;
 
+                fat_rwlock_wrlock(&cls.rwlock);
                 if (!tcls_rules[j]) {
                     struct test_rule *displaced_rule;
 
@@ -883,23 +889,23 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
                     tcls_rules[j] = NULL;
                     pri_rules[pris[j]] = -1;
                 }
+                compare_classifiers(&cls, &tcls);
+                fat_rwlock_unlock(&cls.rwlock);
 
                 n = 0;
                 for (m = 0; m < N_RULES; m++) {
                     n += tcls_rules[m] != NULL;
                 }
                 check_tables(&cls, n > 0, n, n - 1);
-
-                compare_classifiers(&cls, &tcls);
             }
 
+            fat_rwlock_wrlock(&cls.rwlock);
             for (i = 0; i < N_RULES; i++) {
                 if (rules[i]->cls_rule.cls_match) {
                     classifier_remove(&cls, &rules[i]->cls_rule);
                 }
                 free_rule(rules[i]);
             }
-
             fat_rwlock_unlock(&cls.rwlock);
             classifier_destroy(&cls);
             tcls_destroy(&tcls);
@@ -962,6 +968,7 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         fat_rwlock_wrlock(&cls.rwlock);
         classifier_set_prefix_fields(&cls, trie_fields,
                                      ARRAY_SIZE(trie_fields));
+        fat_rwlock_unlock(&cls.rwlock);
         tcls_init(&tcls);
 
         for (i = 0; i < N_RULES; i++) {
@@ -973,22 +980,26 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
 
             rules[i] = make_rule(wcf, priority, value_pats[i]);
             tcls_rules[i] = tcls_insert(&tcls, rules[i]);
+
+            fat_rwlock_wrlock(&cls.rwlock);
             classifier_insert(&cls, &rules[i]->cls_rule);
+            compare_classifiers(&cls, &tcls);
+            fat_rwlock_unlock(&cls.rwlock);
 
             check_tables(&cls, 1, i + 1, 0);
-            compare_classifiers(&cls, &tcls);
         }
 
         for (i = 0; i < N_RULES; i++) {
             tcls_remove(&tcls, tcls_rules[i]);
+            fat_rwlock_wrlock(&cls.rwlock);
             classifier_remove(&cls, &rules[i]->cls_rule);
+            compare_classifiers(&cls, &tcls);
+            fat_rwlock_unlock(&cls.rwlock);
             free_rule(rules[i]);
 
             check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0);
-            compare_classifiers(&cls, &tcls);
         }
 
-        fat_rwlock_unlock(&cls.rwlock);
         classifier_destroy(&cls);
         tcls_destroy(&tcls);
     }
@@ -1026,6 +1037,7 @@ test_many_rules_in_n_tables(int n_tables)
         fat_rwlock_wrlock(&cls.rwlock);
         classifier_set_prefix_fields(&cls, trie_fields,
                                      ARRAY_SIZE(trie_fields));
+        fat_rwlock_unlock(&cls.rwlock);
         tcls_init(&tcls);
 
         for (i = 0; i < MAX_RULES; i++) {
@@ -1035,30 +1047,35 @@ test_many_rules_in_n_tables(int n_tables)
             int value_pat = random_uint32() & ((1u << CLS_N_FIELDS) - 1);
             rule = make_rule(wcf, priority, value_pat);
             tcls_insert(&tcls, rule);
+            fat_rwlock_wrlock(&cls.rwlock);
             classifier_insert(&cls, &rule->cls_rule);
-            check_tables(&cls, -1, i + 1, -1);
             compare_classifiers(&cls, &tcls);
+            fat_rwlock_unlock(&cls.rwlock);
+            check_tables(&cls, -1, i + 1, -1);
         }
 
         while (!classifier_is_empty(&cls)) {
             struct test_rule *rule, *next_rule;
             struct test_rule *target;
-            struct cls_cursor cursor;
 
             target = clone_rule(tcls.rules[random_range(tcls.n_rules)]);
 
-            cls_cursor_init(&cursor, &cls, &target->cls_rule);
-            CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
+            CLS_FOR_EACH_TARGET_SAFE (rule, next_rule, cls_rule, &cls,
+                                      &target->cls_rule) {
+                fat_rwlock_wrlock(&cls.rwlock);
                 classifier_remove(&cls, &rule->cls_rule);
+                fat_rwlock_unlock(&cls.rwlock);
                 free_rule(rule);
             }
+
             tcls_delete_matches(&tcls, &target->cls_rule);
+            fat_rwlock_rdlock(&cls.rwlock);
             compare_classifiers(&cls, &tcls);
+            fat_rwlock_unlock(&cls.rwlock);
             check_tables(&cls, -1, -1, -1);
             free_rule(target);
         }
 
-        fat_rwlock_unlock(&cls.rwlock);
         destroy_classifier(&cls);
         tcls_destroy(&tcls);
     }
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index c4034a0..b001c24 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2316,16 +2316,14 @@ fte_free(struct fte *fte)
 static void
 fte_free_all(struct classifier *cls)
 {
-    struct cls_cursor cursor;
     struct fte *fte, *next;
 
-    fat_rwlock_wrlock(&cls->rwlock);
-    cls_cursor_init(&cursor, cls, NULL);
-    CLS_CURSOR_FOR_EACH_SAFE (fte, next, rule, &cursor) {
+    CLS_FOR_EACH_SAFE (fte, next, rule, cls) {
+        fat_rwlock_wrlock(&cls->rwlock);
         classifier_remove(cls, &fte->rule);
+        fat_rwlock_unlock(&cls->rwlock);
         fte_free(fte);
     }
-    fat_rwlock_unlock(&cls->rwlock);
     classifier_destroy(cls);
 }
 
@@ -2541,7 +2539,6 @@ ofctl_replace_flows(int argc OVS_UNUSED, char *argv[])
 {
     enum { FILE_IDX = 0, SWITCH_IDX = 1 };
     enum ofputil_protocol usable_protocols, protocol;
-    struct cls_cursor cursor;
     struct classifier cls;
     struct list requests;
     struct vconn *vconn;
@@ -2558,9 +2555,7 @@ ofctl_replace_flows(int argc OVS_UNUSED, char *argv[])
     list_init(&requests);
 
     /* Delete flows that exist on the switch but not in the file. */
-    fat_rwlock_rdlock(&cls.rwlock);
-    cls_cursor_init(&cursor, &cls, NULL);
-    CLS_CURSOR_FOR_EACH (fte, rule, &cursor) {
+    CLS_FOR_EACH (fte, rule, &cls) {
         struct fte_version *file_ver = fte->versions[FILE_IDX];
         struct fte_version *sw_ver = fte->versions[SWITCH_IDX];
 
@@ -2572,8 +2567,7 @@ ofctl_replace_flows(int argc OVS_UNUSED, char *argv[])
 
     /* Add flows that exist in the file but not on the switch.
      * Update flows that exist in both places but differ. */
-    cls_cursor_init(&cursor, &cls, NULL);
-    CLS_CURSOR_FOR_EACH (fte, rule, &cursor) {
+    CLS_FOR_EACH (fte, rule, &cls) {
         struct fte_version *file_ver = fte->versions[FILE_IDX];
         struct fte_version *sw_ver = fte->versions[SWITCH_IDX];
 
@@ -2582,7 +2576,6 @@ ofctl_replace_flows(int argc OVS_UNUSED, char *argv[])
             fte_make_flow_mod(fte, FILE_IDX, OFPFC_ADD, protocol, &requests);
         }
     }
-    fat_rwlock_unlock(&cls.rwlock);
     transact_multiple_noreply(vconn, &requests);
     vconn_close(vconn);
 
@@ -2612,7 +2605,6 @@ static void
 ofctl_diff_flows(int argc OVS_UNUSED, char *argv[])
 {
     bool differences = false;
-    struct cls_cursor cursor;
     struct classifier cls;
     struct ds a_s, b_s;
     struct fte *fte;
@@ -2624,9 +2616,7 @@ ofctl_diff_flows(int argc OVS_UNUSED, char *argv[])
     ds_init(&a_s);
     ds_init(&b_s);
 
-    fat_rwlock_rdlock(&cls.rwlock);
-    cls_cursor_init(&cursor, &cls, NULL);
-    CLS_CURSOR_FOR_EACH (fte, rule, &cursor) {
+    CLS_FOR_EACH (fte, rule, &cls) {
         struct fte_version *a = fte->versions[0];
         struct fte_version *b = fte->versions[1];
 
@@ -2644,7 +2634,6 @@ ofctl_diff_flows(int argc OVS_UNUSED, char *argv[])
             }
         }
     }
-    fat_rwlock_unlock(&cls.rwlock);
 
     ds_destroy(&a_s);
     ds_destroy(&b_s);
-- 
1.7.10.4




More information about the dev mailing list