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.
---
lib/classifier.c | 20 +++++++++++++++++++-
lib/classifier.h | 3 ++-
ofproto/ofproto-dpif.c | 25 ++++++++++---------------
ofproto/ofproto.c | 30 +++++++++---------------------
ofproto/private.h | 25 ++++++++++++-------------
tests/test-classifier.c | 12 ++++++------
utilities/ovs-ofctl.c | 2 +-
7 files changed, 59 insertions(+), 58 deletions(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index 42a2169..013de32 100644
--- 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,24 @@ 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.
+ *
+ * Returns NULL if 'cls' does not contain a rule with an identical key, after
+ * inserting the new rule. In this case, no rules are displaced by the new
+ * rule, even rules that cannot have any effect because the new rule matches a
+ * superset of their flows and has higher priority. */
+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
--- 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 baaa19b..f163ff4 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2442,7 +2442,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,
@@ -2451,15 +2451,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;
@@ -2472,19 +2476,11 @@ rule_destruct(struct rule *rule_)
struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
struct facet *facet, *next_facet;
+ 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
@@ -3867,7 +3863,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 028a50a..7f3ea6b 100644
--- 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 *);
@@ -882,9 +881,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
@@ -901,7 +898,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);
}
}
@@ -1261,14 +1258,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
@@ -1317,15 +1314,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 free 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
@@ -2353,7 +2341,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
@@ -2387,7 +2375,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);
}
static int
diff --git a/ofproto/private.h b/ofproto/private.h
index cbf2c2e..c6c18f1 100644
--- 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
--- 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
--- 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];
--
1.7.4.4
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
