[ovs-dev] [PATCH] classifier: Refactor interface for classifier_remove().

Ben Pfaff blp at ovn.org
Tue Jan 30 21:00:31 UTC 2018


Until now, classifier_remove() returned either null or the classifier rule
passed to it, which is an unusual interface.  This commit changes it to
return true if it succeeds or false on failure.

In addition, most of classifier_remove()'s callers know ahead of time that
it must succeed, even though most of them didn't bother with an assertion,
so this commit adds a classifier_remove_assert() function as a helper.

Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 lib/classifier.c        | 25 +++++++++++++++++--------
 lib/classifier.h        |  4 ++--
 lib/ovs-router.c        | 19 ++++++++-----------
 lib/tnl-ports.c         |  5 ++---
 ofproto/ofproto.c       | 14 ++++----------
 tests/test-classifier.c | 19 +++++++++----------
 tests/test-ovn.c        |  2 +-
 utilities/ovs-ofctl.c   |  2 +-
 8 files changed, 44 insertions(+), 46 deletions(-)

diff --git a/lib/classifier.c b/lib/classifier.c
index 16c451da1b30..9ad3710d61a1 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -695,15 +695,16 @@ classifier_insert(struct classifier *cls, const struct cls_rule *rule,
     ovs_assert(!displaced_rule);
 }
 
-/* Removes 'rule' from 'cls'.  It is the caller's responsibility to destroy
- * 'rule' with cls_rule_destroy(), freeing the memory block in which 'rule'
- * resides, etc., as necessary.
+/* If 'rule' is in 'cls', removes 'rule' from 'cls' and returns true.  It is
+ * the caller's responsibility to destroy 'rule' with cls_rule_destroy(),
+ * freeing the memory block in which 'rule' resides, etc., as necessary.
  *
- * Does nothing if 'rule' has been already removed, or was never inserted.
+ * If 'rule' is not in any classifier, returns false without making any
+ * changes.
  *
- * Returns the removed rule, or NULL, if it was already removed.
+ * 'rule' must not be in some classifier other than 'cls'.
  */
-const struct cls_rule *
+bool
 classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
 {
     struct cls_match *rule, *prev, *next, *head;
@@ -716,7 +717,7 @@ classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
 
     rule = get_cls_match_protected(cls_rule);
     if (!rule) {
-        return NULL;
+        return false;
     }
     /* Mark as removed. */
     ovsrcu_set(&CONST_CAST(struct cls_rule *, cls_rule)->cls_match, NULL);
@@ -820,7 +821,15 @@ check_priority:
     ovsrcu_postpone(cls_match_free_cb, rule);
     cls->n_rules--;
 
-    return cls_rule;
+    return true;
+}
+
+void
+classifier_remove_assert(struct classifier *cls,
+                         const struct cls_rule *cls_rule)
+{
+    bool OVS_UNUSED removed = classifier_remove(cls, cls_rule);
+    ovs_assert(removed);
 }
 
 /* Prefix tree context.  Valid when 'lookup_done' is true.  Can skip all
diff --git a/lib/classifier.h b/lib/classifier.h
index 71c2e507d7c3..31d4a1b08bd2 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -387,8 +387,8 @@ const struct cls_rule *classifier_replace(struct classifier *,
                                           ovs_version_t,
                                           const struct cls_conjunction *,
                                           size_t n_conjunctions);
-const struct cls_rule *classifier_remove(struct classifier *,
-                                         const struct cls_rule *);
+bool classifier_remove(struct classifier *, const struct cls_rule *);
+void classifier_remove_assert(struct classifier *, const struct cls_rule *);
 static inline void classifier_defer(struct classifier *);
 static inline void classifier_publish(struct classifier *);
 
diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index cd2ab15fb003..e6cc81fd0827 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -245,19 +245,14 @@ ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
     ovs_router_insert__(mark, plen, ip_dst, plen, output_bridge, gw);
 }
 
-static bool
-__rt_entry_delete(const struct cls_rule *cr)
+static void
+rt_entry_delete__(const struct cls_rule *cr)
 {
     struct ovs_router_entry *p = ovs_router_entry_cast(cr);
 
     tnl_port_map_delete_ipdev(p->output_bridge);
-    /* Remove it. */
-    cr = classifier_remove(&cls, cr);
-    if (cr) {
-        ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
-        return true;
-    }
-    return false;
+    classifier_remove_assert(&cls, cr);
+    ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
 }
 
 static bool
@@ -277,8 +272,10 @@ rt_entry_delete(uint32_t mark, uint8_t priority,
     cr = classifier_find_rule_exactly(&cls, &rule, OVS_VERSION_MAX);
     if (cr) {
         ovs_mutex_lock(&mutex);
-        res = __rt_entry_delete(cr);
+        rt_entry_delete__(cr);
         ovs_mutex_unlock(&mutex);
+
+        res = true;
     }
 
     cls_rule_destroy(&rule);
@@ -476,7 +473,7 @@ ovs_router_flush(void)
     classifier_defer(&cls);
     CLS_FOR_EACH(rt, cr, &cls) {
         if (rt->priority == rt->plen) {
-            __rt_entry_delete(&rt->cr);
+            rt_entry_delete__(&rt->cr);
         }
     }
     classifier_publish(&cls);
diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
index 04d2b3f7c6cf..b814f7a0a50a 100644
--- a/lib/tnl-ports.c
+++ b/lib/tnl-ports.c
@@ -223,9 +223,8 @@ tnl_port_unref(const struct cls_rule *cr)
     struct tnl_port_in *p = tnl_port_cast(cr);
 
     if (cr && ovs_refcount_unref_relaxed(&p->ref_cnt) == 1) {
-        if (classifier_remove(&cls, cr)) {
-            ovsrcu_postpone(tnl_port_free, p);
-        }
+        classifier_remove_assert(&cls, cr);
+        ovsrcu_postpone(tnl_port_free, p);
     }
 }
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 4f17f79d286f..536636393850 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1520,10 +1520,8 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule)
         /* Make sure there is no postponed removal of the rule. */
         ovs_assert(cls_rule_visible_in_version(&rule->cr, OVS_VERSION_MAX));
 
-        if (!classifier_remove(&rule->ofproto->tables[rule->table_id].cls,
-                               &rule->cr)) {
-            OVS_NOT_REACHED();
-        }
+        classifier_remove_assert(&rule->ofproto->tables[rule->table_id].cls,
+                                 &rule->cr);
         ofproto_rule_remove__(rule->ofproto, rule);
         if (ofproto->ofproto_class->rule_delete) {
             ofproto->ofproto_class->rule_delete(rule);
@@ -2885,9 +2883,7 @@ remove_rule_rcu__(struct rule *rule)
     struct oftable *table = &ofproto->tables[rule->table_id];
 
     ovs_assert(!cls_rule_visible_in_version(&rule->cr, OVS_VERSION_MAX));
-    if (!classifier_remove(&table->cls, &rule->cr)) {
-        OVS_NOT_REACHED();
-    }
+    classifier_remove_assert(&table->cls, &rule->cr);
     if (ofproto->ofproto_class->rule_delete) {
         ofproto->ofproto_class->rule_delete(rule);
     }
@@ -5231,9 +5227,7 @@ replace_rule_revert(struct ofproto *ofproto,
     }
 
     /* Remove the new rule immediately.  It was never visible to lookups. */
-    if (!classifier_remove(&table->cls, &new_rule->cr)) {
-        OVS_NOT_REACHED();
-    }
+    classifier_remove_assert(&table->cls, &new_rule->cr);
     ofproto_rule_remove__(ofproto, new_rule);
     ofproto_rule_unref(new_rule);
 }
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index edaf4a393382..ef6693e61bcb 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -466,9 +466,8 @@ destroy_classifier(struct classifier *cls)
 
     classifier_defer(cls);
     CLS_FOR_EACH (rule, cls_rule, cls) {
-        if (classifier_remove(cls, &rule->cls_rule)) {
-            ovsrcu_postpone(free_rule, rule);
-        }
+        classifier_remove_assert(cls, &rule->cls_rule);
+        ovsrcu_postpone(free_rule, rule);
     }
     classifier_destroy(cls);
 }
@@ -816,7 +815,7 @@ test_single_rule(struct ovs_cmdl_context *ctx OVS_UNUSED)
         compare_classifiers(&cls, 0, OVS_VERSION_MIN, &tcls);
         check_tables(&cls, 1, 1, 0, 0, OVS_VERSION_MIN);
 
-        classifier_remove(&cls, &rule->cls_rule);
+        classifier_remove_assert(&cls, &rule->cls_rule);
         tcls_remove(&tcls, tcls_rule);
         assert(classifier_is_empty(&cls));
         assert(tcls_is_empty(&tcls));
@@ -864,7 +863,7 @@ test_rule_replacement(struct ovs_cmdl_context *ctx OVS_UNUSED)
         compare_classifiers(&cls, 0, OVS_VERSION_MIN, &tcls);
         check_tables(&cls, 1, 1, 0, 0, OVS_VERSION_MIN);
         classifier_defer(&cls);
-        classifier_remove(&cls, &rule2->cls_rule);
+        classifier_remove_assert(&cls, &rule2->cls_rule);
 
         tcls_destroy(&tcls);
         destroy_classifier(&cls);
@@ -1018,7 +1017,7 @@ test_many_rules_in_one_list (struct ovs_cmdl_context *ctx OVS_UNUSED)
                         n_invisible_rules++;
                         removable_rule = &rules[j]->cls_rule;
                     } else {
-                        classifier_remove(&cls, &rules[j]->cls_rule);
+                        classifier_remove_assert(&cls, &rules[j]->cls_rule);
                     }
                     tcls_remove(&tcls, tcls_rules[j]);
                     tcls_rules[j] = NULL;
@@ -1039,7 +1038,7 @@ test_many_rules_in_one_list (struct ovs_cmdl_context *ctx OVS_UNUSED)
                     /* Removable rule is no longer visible. */
                     assert(cls_match);
                     assert(!cls_match_visible_in_version(cls_match, version));
-                    classifier_remove(&cls, removable_rule);
+                    classifier_remove_assert(&cls, removable_rule);
                     n_invisible_rules--;
                 }
             }
@@ -1139,7 +1138,7 @@ test_many_rules_in_one_table(struct ovs_cmdl_context *ctx OVS_UNUSED)
                                                    version);
                 n_invisible_rules++;
             } else {
-                classifier_remove(&cls, &rules[i]->cls_rule);
+                classifier_remove_assert(&cls, &rules[i]->cls_rule);
             }
             compare_classifiers(&cls, n_invisible_rules, version, &tcls);
             check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0,
@@ -1151,7 +1150,7 @@ test_many_rules_in_one_table(struct ovs_cmdl_context *ctx OVS_UNUSED)
 
         if (versioned) {
             for (i = 0; i < N_RULES; i++) {
-                classifier_remove(&cls, &rules[i]->cls_rule);
+                classifier_remove_assert(&cls, &rules[i]->cls_rule);
                 n_invisible_rules--;
 
                 compare_classifiers(&cls, n_invisible_rules, version, &tcls);
@@ -1250,7 +1249,7 @@ test_many_rules_in_n_tables(int n_tables)
 
             /* Remove rules that are no longer visible. */
             LIST_FOR_EACH_POP (rule, list_node, &list) {
-                classifier_remove(&cls, &rule->cls_rule);
+                classifier_remove_assert(&cls, &rule->cls_rule);
                 n_invisible_rules--;
 
                 compare_classifiers(&cls, n_invisible_rules, version,
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 539e778e1c30..85763718d401 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -972,7 +972,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
             struct test_rule *test_rule;
 
             CLS_FOR_EACH (test_rule, cr, &cls) {
-                classifier_remove(&cls, &test_rule->cr);
+                classifier_remove_assert(&cls, &test_rule->cr);
                 ovsrcu_postpone(free_rule, test_rule);
             }
             classifier_destroy(&cls);
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 3239d6fcb018..e1e4d37ed3af 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -3220,7 +3220,7 @@ fte_free_all(struct flow_tables *tables)
 
         classifier_defer(cls);
         CLS_FOR_EACH (fte, rule, cls) {
-            classifier_remove(cls, &fte->rule);
+            classifier_remove_assert(cls, &fte->rule);
             ovsrcu_postpone(fte_free, fte);
         }
         classifier_destroy(cls);
-- 
2.15.1



More information about the dev mailing list