ofproto: Make rule construction and destruction more symmetric.

Before, ->rule_construct() both created the rule and inserted into the
flow table, but ->rule_destruct() only destroyed the rule.  This makes
->rule_destruct() also remove the rule from the flow table.

diff --git a/lib/classifier.c b/lib/classifier.c
index 42a2169..edaceb4 100644 (file)
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -692,7 +692,7 @@ classifier_count(const struct classifier *cls)
  * rule, even rules that cannot have any effect because the new rule matches a
  * superset of their flows and has higher priority. */
 struct cls_rule *
-classifier_insert(struct classifier *cls, struct cls_rule *rule)
+classifier_replace(struct classifier *cls, struct cls_rule *rule)
 {
     struct cls_rule *old_rule;
     struct cls_table *table;
@@ -710,6 +710,19 @@ classifier_insert(struct classifier *cls, struct cls_rule *rule)
     return old_rule;
 }
 
+/* Inserts 'rule' into 'cls'.  Until 'rule' is removed from 'cls', the caller
+ * must not modify or free it.
+ *
+ * 'cls' must not contain an identical rule (including wildcards, values of
+ * fixed fields, and priority).  Use classifier_find_rule_exactly() to find
+ * such a rule. */
+void
+classifier_insert(struct classifier *cls, struct cls_rule *rule)
+{
+    struct cls_rule *displaced_rule = classifier_replace(cls, rule);
+    assert(!displaced_rule);
+}
+
 /* Removes 'rule' from 'cls'.  It is the caller's responsibility to free
  * 'rule', if this is desirable. */
 void

diff --git a/lib/classifier.h b/lib/classifier.h
index d3121bf..08e2c0d 100644 (file)
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -121,7 +121,8 @@ void classifier_init(struct classifier *);
 void classifier_destroy(struct classifier *);
 bool classifier_is_empty(const struct classifier *);
 int classifier_count(const struct classifier *);
-struct cls_rule *classifier_insert(struct classifier *, struct cls_rule *);
+void classifier_insert(struct classifier *, struct cls_rule *);
+struct cls_rule *classifier_replace(struct classifier *, struct cls_rule *);
 void classifier_remove(struct classifier *, struct cls_rule *);
 struct cls_rule *classifier_lookup(const struct classifier *,
                                    const struct flow *);

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index aefaf59..7779cf5 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2449,7 +2449,7 @@ rule_construct(struct rule *rule_)
 {
     struct rule_dpif *rule = rule_dpif_cast(rule_);
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
-    struct cls_rule *displaced_rule;
+    struct rule_dpif *old_rule;
     int error;
 
     error = validate_actions(rule->up.actions, rule->up.n_actions,
@@ -2458,15 +2458,19 @@ rule_construct(struct rule *rule_)
         return error;
     }
 
+    old_rule = rule_dpif_cast(rule_from_cls_rule(classifier_find_rule_exactly(
+                                                     &ofproto->up.cls,
+                                                     &rule->up.cr)));
+    if (old_rule) {
+        ofproto_rule_destroy(&old_rule->up);
+    }
+
     rule->used = rule->up.created;
     rule->packet_count = 0;
     rule->byte_count = 0;
     list_init(&rule->facets);
+    classifier_insert(&ofproto->up.cls, &rule->up.cr);
 
-    displaced_rule = classifier_insert(&ofproto->up.cls, &rule->up.cr);
-    if (displaced_rule) {
-        ofproto_rule_destroy(rule_from_cls_rule(displaced_rule));
-    }
     ofproto->need_revalidate = true;
 
     return 0;
@@ -2479,20 +2483,11 @@ rule_destruct(struct rule *rule_)
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
     struct facet *facet, *next_facet;
 
-    ofproto->need_revalidate = true;
+    classifier_remove(&ofproto->up.cls, &rule->up.cr);
     LIST_FOR_EACH_SAFE (facet, next_facet, list_node, &rule->facets) {
         facet_revalidate(ofproto, facet);
     }
-}
-
-static void
-rule_remove(struct rule *rule_)
-{
-    struct rule_dpif *rule = rule_dpif_cast(rule_);
-    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
-
     ofproto->need_revalidate = true;
-    classifier_remove(&ofproto->up.cls, &rule->up.cr);
 }
 
 static void
@@ -3877,7 +3872,6 @@ const struct ofproto_class ofproto_dpif_class = {
     rule_construct,
     rule_destruct,
     rule_dealloc,
-    rule_remove,
     rule_get_stats,
     rule_execute,
     rule_modify_actions,

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 1c6b8a9..3d995e1 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -78,7 +78,6 @@ static void ofproto_flush_flows__(struct ofproto *);
 
 static void ofproto_rule_destroy__(struct rule *);
 static void ofproto_rule_send_removed(struct rule *, uint8_t reason);
-static void ofproto_rule_remove(struct rule *);
 
 static void handle_openflow(struct ofconn *, struct ofpbuf *);
 
@@ -888,9 +887,7 @@ ofproto_delete_flow(struct ofproto *ofproto, const struct cls_rule *target)
 
     rule = rule_from_cls_rule(classifier_find_rule_exactly(&ofproto->cls,
                                                            target));
-    if (rule) {
-        ofproto_rule_remove(rule);
-    }
+    ofproto_rule_destroy(rule);
 }
 
 static void
@@ -907,7 +904,7 @@ ofproto_flush_flows__(struct ofproto *ofproto)
 
     cls_cursor_init(&cursor, &ofproto->cls, NULL);
     CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cr, &cursor) {
-        ofproto_rule_remove(rule);
+        ofproto_rule_destroy(rule);
     }
 }
 
@@ -1269,14 +1266,14 @@ ofproto_rule_destroy__(struct rule *rule)
     rule->ofproto->ofproto_class->rule_dealloc(rule);
 }
 
-/* Destroys 'rule' and removes it from the datapath.
- *
- * The caller must have already removed 'rule' from the classifier. */
+/* Destroys 'rule' and removes it from the flow table and the datapath. */
 void
 ofproto_rule_destroy(struct rule *rule)
 {
-    rule->ofproto->ofproto_class->rule_destruct(rule);
-    ofproto_rule_destroy__(rule);
+    if (rule) {
+        rule->ofproto->ofproto_class->rule_destruct(rule);
+        ofproto_rule_destroy__(rule);
+    }
 }
 
 /* Returns true if 'rule' has an OpenFlow OFPAT_OUTPUT or OFPAT_ENQUEUE action
@@ -1325,15 +1322,6 @@ rule_execute(struct rule *rule, uint16_t in_port, struct ofpbuf *packet)
     return rule->ofproto->ofproto_class->rule_execute(rule, &flow, packet);
 }
 
-/* Removes 'rule' from 'ofproto' and frees up the associated memory.  Removes
- * 'rule' from the classifier.  */
-void
-ofproto_rule_remove(struct rule *rule)
-{
-    rule->ofproto->ofproto_class->rule_remove(rule);
-    ofproto_rule_destroy(rule);
-}
-
 /* Returns true if 'rule' should be hidden from the controller.
  *
  * Rules with priority higher than UINT16_MAX are set up by ofproto itself
@@ -2360,7 +2348,7 @@ delete_flow(struct rule *rule, ovs_be16 out_port)
     }
 
     ofproto_rule_send_removed(rule, OFPRR_DELETE);
-    ofproto_rule_remove(rule);
+    ofproto_rule_destroy(rule);
 }
 
 static void
@@ -2394,7 +2382,7 @@ ofproto_rule_expire(struct rule *rule, uint8_t reason)
 {
     assert(reason == OFPRR_HARD_TIMEOUT || reason == OFPRR_IDLE_TIMEOUT);
     ofproto_rule_send_removed(rule, reason);
-    ofproto_rule_remove(rule);
+    ofproto_rule_destroy(rule);
 }
 \f
 static int

diff --git a/ofproto/private.h b/ofproto/private.h
index bf054fd..e74d4ad 100644 (file)
--- a/ofproto/private.h
+++ b/ofproto/private.h
@@ -472,32 +472,31 @@ struct ofproto_class {
      * flow table:
      *
      *   - If there was already a rule with exactly the same matching criteria
-     *     and priority in the classifier, then it should remove that rule from
-     *     the classifier and destroy it (with ofproto_rule_destroy()).
+     *     and priority in the classifier, then it should destroy it (with
+     *     ofproto_rule_destroy()).
+     *
+     *     To the greatest extent possible, the old rule should be destroyed
+     *     only if inserting the new rule succeeds; that is, ->rule_construct()
+     *     should be transactional.
+     *
+     *     The function classifier_find_rule_exactly() can locate such a rule.
      *
      *   - Insert the new rule into the ofproto's 'cls' classifier, and into
      *     the datapath flow table.
      *
-     *     (The function classifier_insert() both inserts a rule into the
-     *     classifier and removes any rule with identical matching criteria, so
-     *     this single call implements parts of both steps above.)
+     *     The function classifier_insert() inserts a rule into the classifier.
      *
      * Other than inserting 'rule->cr' into the classifier, ->rule_construct()
      * should not modify any base members of struct rule.
      *
-     * When ->rule_destruct() is called, 'rule' has already been removed from
-     * the classifier and the datapath flow table (by calling ->rule_remove()),
-     * so ->rule_destruct() should not duplicate that behavior. */
+     * ->rule_destruct() should remove 'rule' from the ofproto's 'cls'
+     * classifier (e.g. with classifier_remove()) and from the datapath flow
+     * table. */
     struct rule *(*rule_alloc)(void);
     int (*rule_construct)(struct rule *rule);
     void (*rule_destruct)(struct rule *rule);
     void (*rule_dealloc)(struct rule *rule);
 
-    /* Removes 'rule' from 'rule->ofproto->cls' and from the datapath.
-     *
-     * 'rule' will be destructed, with ->rule_destruct(), soon after. */
-    void (*rule_remove)(struct rule *rule);
-
     /* Obtains statistics for 'rule', storing the number of packets that have
      * matched it in '*packet_count' and the number of bytes in those packets
      * in '*byte_count'. */

diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index bb75dba..1cfd5cf 100644 (file)
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -526,7 +526,7 @@ test_single_rule(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         tcls_init(&tcls);
 
         tcls_rule = tcls_insert(&tcls, rule);
-        assert(!classifier_insert(&cls, &rule->cls_rule));
+        classifier_insert(&cls, &rule->cls_rule);
         check_tables(&cls, 1, 1, 0);
         compare_classifiers(&cls, &tcls);
 
@@ -562,7 +562,7 @@ test_rule_replacement(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         classifier_init(&cls);
         tcls_init(&tcls);
         tcls_insert(&tcls, rule1);
-        assert(!classifier_insert(&cls, &rule1->cls_rule));
+        classifier_insert(&cls, &rule1->cls_rule);
         check_tables(&cls, 1, 1, 0);
         compare_classifiers(&cls, &tcls);
         tcls_destroy(&tcls);
@@ -570,7 +570,7 @@ test_rule_replacement(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         tcls_init(&tcls);
         tcls_insert(&tcls, rule2);
         assert(test_rule_from_cls_rule(
-                   classifier_insert(&cls, &rule2->cls_rule)) == rule1);
+                   classifier_replace(&cls, &rule2->cls_rule)) == rule1);
         free(rule1);
         check_tables(&cls, 1, 1, 0);
         compare_classifiers(&cls, &tcls);
@@ -681,7 +681,7 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
 
                     tcls_rules[j] = tcls_insert(&tcls, rules[j]);
                     displaced_rule = test_rule_from_cls_rule(
-                        classifier_insert(&cls, &rules[j]->cls_rule));
+                        classifier_replace(&cls, &rules[j]->cls_rule));
                     if (pri_rules[pris[j]] >= 0) {
                         int k = pri_rules[pris[j]];
                         assert(displaced_rule != NULL);
@@ -781,7 +781,7 @@ 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]);
-            assert(!classifier_insert(&cls, &rules[i]->cls_rule));
+            classifier_insert(&cls, &rules[i]->cls_rule);
 
             check_tables(&cls, 1, i + 1, 0);
             compare_classifiers(&cls, &tcls);
@@ -839,7 +839,7 @@ test_many_rules_in_n_tables(int n_tables)
             int value_pat = rand() & ((1u << CLS_N_FIELDS) - 1);
             rule = make_rule(wcf, priority, value_pat);
             tcls_insert(&tcls, rule);
-            assert(!classifier_insert(&cls, &rule->cls_rule));
+            classifier_insert(&cls, &rule->cls_rule);
             check_tables(&cls, -1, i + 1, -1);
             compare_classifiers(&cls, &tcls);
         }

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 660fc3a..b829ac7 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1000,7 +1000,7 @@ fte_insert(struct classifier *cls, const struct cls_rule *rule,
     fte->rule = *rule;
     fte->versions[index] = version;
 
-    old = fte_from_cls_rule(classifier_insert(cls, &fte->rule));
+    old = fte_from_cls_rule(classifier_replace(cls, &fte->rule));
     if (old) {
         fte_version_free(old->versions[index]);
         fte->versions[!index] = old->versions[!index];