From 08944c1db1ab2707e28deab838dc0937bf8de8ae Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 11 May 2011 14:06:48 -0700 Subject: [PATCH] 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. --- lib/classifier.c | 15 ++++++++++++++- lib/classifier.h | 3 ++- ofproto/ofproto-dpif.c | 26 ++++++++++---------------- ofproto/ofproto.c | 30 +++++++++--------------------- ofproto/private.h | 25 ++++++++++++------------- tests/test-classifier.c | 12 ++++++------ utilities/ovs-ofctl.c | 2 +- 7 files changed, 54 insertions(+), 59 deletions(-) diff --git a/lib/classifier.c b/lib/classifier.c index 42a216936..edaceb416 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,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 d3121bfb8..08e2c0dfa 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 aefaf59d0..7779cf5b9 100644 --- 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 1c6b8a90f..3d995e19f 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 *); @@ -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); } static int diff --git a/ofproto/private.h b/ofproto/private.h index bf054fd53..e74d4ad57 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 bb75dba1a..1cfd5cf6e 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 660fc3aed..b829ac727 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]; -- 2.43.0