[ovs-dev] [PATCH] classifier: refactor classifier_remove and introduce classifier_remove_assert

Yifeng Sun pkusunyifeng at gmail.com
Tue Jan 30 15:56:07 UTC 2018


The return type of classifier_remove is changed to bool. Besides,
classifier_remove_assert is introduced to assert that the classifier
must contain the rule. This patch is based on Ben's advice.

Signed-off-by: Yifeng Sun <pkusunyifeng at gmail.com>
---
 lib/classifier.c  | 21 +++++++++++++++++----
 lib/classifier.h  |  6 ++++--
 lib/ovs-router.c  | 15 +++++----------
 lib/tnl-ports.c   |  5 ++---
 ofproto/ofproto.c | 14 ++++----------
 5 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/lib/classifier.c b/lib/classifier.c
index 16c451da1b30..4a4aacfd6208 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -701,9 +701,9 @@ classifier_insert(struct classifier *cls, const struct cls_rule *rule,
  *
  * Does nothing if 'rule' has been already removed, or was never inserted.
  *
- * Returns the removed rule, or NULL, if it was already removed.
+ * Returns true on success, or false, if it was already removed.
  */
-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 +716,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 +820,20 @@ check_priority:
     ovsrcu_postpone(cls_match_free_cb, rule);
     cls->n_rules--;
 
-    return cls_rule;
+    return true;
+}
+
+/* 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.
+ *
+ * Asserts that the rule must be in the classifier.
+ */
+void
+classifier_remove_assert(struct classifier *cls,
+                         const struct cls_rule *cls_rule)
+{
+    ovs_assert(classifier_remove(cls, cls_rule));
 }
 
 /* Prefix tree context.  Valid when 'lookup_done' is true.  Can skip all
diff --git a/lib/classifier.h b/lib/classifier.h
index f0ea5a9cb8b2..7699d58e1b07 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -387,8 +387,10 @@ 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..a7d55c754d16 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -245,19 +245,15 @@ 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
+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, p);
 }
 
 static bool
@@ -267,7 +263,6 @@ rt_entry_delete(uint32_t mark, uint8_t priority,
     const struct cls_rule *cr;
     struct cls_rule rule;
     struct match match;
-    bool res = false;
 
     rt_init_match(&match, mark, ip6_dst, plen);
 
@@ -277,12 +272,12 @@ 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);
     }
 
     cls_rule_destroy(&rule);
-    return res;
+    return (cr != NULL);
 }
 
 static bool
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);
 }
-- 
2.7.4



More information about the dev mailing list