[ovs-dev] [classifier-opt 26/28] classifier: Prepare for "struct cls_rule" needing to be destroyed.
Ben Pfaff
blp at nicira.com
Fri Jul 20 23:25:23 UTC 2012
Until now, "struct cls_rule" didn't own any data outside its own memory
block. An upcoming commit will make "struct cls_rule" sometimes own blocks
of memory, so it needs "destroy" and to a lesser extent "clone" functions.
This commit adds these in advance, even though they are mostly no-ops, to
make it possible to separately review the memory management.
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
lib/classifier.c | 17 +++++++++++++++++
lib/classifier.h | 2 ++
ofproto/ofproto.c | 22 +++++++++++++++++++---
tests/test-classifier.c | 46 ++++++++++++++++++++++++++++++++++------------
utilities/ovs-ofctl.c | 2 ++
5 files changed, 74 insertions(+), 15 deletions(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index cb66295..a482666 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -69,6 +69,23 @@ cls_rule_init(struct cls_rule *rule,
rule->priority = priority;
}
+/* Initializes 'dst' as a copy of 'src'. */
+void
+cls_rule_clone(struct cls_rule *dst, const struct cls_rule *src)
+{
+ *dst = *src;
+}
+
+/* Frees memory referenced by 'rule'. Doesn't free 'rule' itself (it's
+ * normally embedded into a larger structure).
+ *
+ * ('rule' must not currently be in a classifier.) */
+void
+cls_rule_destroy(struct cls_rule *rule OVS_UNUSED)
+{
+ /* Nothing to do yet. */
+}
+
/* Returns true if 'a' and 'b' match the same packets at the same priority,
* false if they differ in some way. */
bool
diff --git a/lib/classifier.h b/lib/classifier.h
index 8a11f05..74f9211 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -70,6 +70,8 @@ struct cls_rule {
void cls_rule_init(struct cls_rule *,
const struct match *, unsigned int priority);
+void cls_rule_clone(struct cls_rule *, const struct cls_rule *);
+void cls_rule_destroy(struct cls_rule *);
bool cls_rule_equal(const struct cls_rule *, const struct cls_rule *);
uint32_t cls_rule_hash(const struct cls_rule *, uint32_t basis);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index e55e89f..4cb56e2 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1428,6 +1428,8 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
cls_rule_init(&cr, match, priority);
rule = rule_from_cls_rule(classifier_find_rule_exactly(
&ofproto->tables[0].cls, &cr));
+ cls_rule_destroy(&cr);
+
if (!rule || !ofpacts_equal(rule->ofpacts, rule->ofpacts_len,
ofpacts, ofpacts_len)) {
struct ofputil_flow_mod fm;
@@ -1468,6 +1470,8 @@ ofproto_delete_flow(struct ofproto *ofproto,
cls_rule_init(&cr, target, priority);
rule = rule_from_cls_rule(classifier_find_rule_exactly(
&ofproto->tables[0].cls, &cr));
+ cls_rule_destroy(&cr);
+
if (!rule) {
/* No such rule -> success. */
return true;
@@ -1904,6 +1908,7 @@ static void
ofproto_rule_destroy__(struct rule *rule)
{
if (rule) {
+ cls_rule_destroy(&rule->cr);
free(rule->ofpacts);
rule->ofproto->ofproto_class->rule_dealloc(rule);
}
@@ -2456,7 +2461,8 @@ collect_rules_loose(struct ofproto *ofproto, uint8_t table_id,
cls_cursor_init(&cursor, &table->cls, &cr);
CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
if (rule->pending) {
- return OFPROTO_POSTPONE;
+ error = OFPROTO_POSTPONE;
+ goto exit;
}
if (!ofproto_rule_is_hidden(rule)
&& ofproto_rule_has_out_port(rule, out_port)
@@ -2465,7 +2471,10 @@ collect_rules_loose(struct ofproto *ofproto, uint8_t table_id,
}
}
}
- return 0;
+
+exit:
+ cls_rule_destroy(&cr);
+ return error;
}
/* Searches 'ofproto' for rules in table 'table_id' (or in all tables, if
@@ -2503,7 +2512,8 @@ collect_rules_strict(struct ofproto *ofproto, uint8_t table_id,
&cr));
if (rule) {
if (rule->pending) {
- return OFPROTO_POSTPONE;
+ error = OFPROTO_POSTPONE;
+ goto exit;
}
if (!ofproto_rule_is_hidden(rule)
&& ofproto_rule_has_out_port(rule, out_port)
@@ -2512,6 +2522,9 @@ collect_rules_strict(struct ofproto *ofproto, uint8_t table_id,
}
}
}
+
+exit:
+ cls_rule_destroy(&cr);
return 0;
}
@@ -2907,6 +2920,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
/* Serialize against pending deletion. */
if (is_flow_deletion_pending(ofproto, &cr, table - ofproto->tables)) {
+ cls_rule_destroy(&rule->cr);
ofproto->ofproto_class->rule_dealloc(rule);
return OFPROTO_POSTPONE;
}
@@ -2914,6 +2928,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
/* Check for overlap, if requested. */
if (fm->flags & OFPFF_CHECK_OVERLAP
&& classifier_rule_overlaps(&table->cls, &rule->cr)) {
+ cls_rule_destroy(&rule->cr);
ofproto->ofproto_class->rule_dealloc(rule);
return OFPERR_OFPFMFC_OVERLAP;
}
@@ -3629,6 +3644,7 @@ ofproto_collect_ofmonitor_refresh_rules(const struct ofmonitor *m,
ofproto_collect_ofmonitor_refresh_rule(m, rule, seqno, rules);
}
}
+ cls_rule_destroy(&target);
}
static void
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index f279bda..5e24dd2 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -95,6 +95,11 @@ test_rule_from_cls_rule(const struct cls_rule *rule)
return rule ? CONTAINER_OF(rule, struct test_rule, cls_rule) : NULL;
}
+static struct test_rule *make_rule(int wc_fields, unsigned int priority,
+ int value_pat);
+static void free_rule(struct test_rule *);
+static struct test_rule *clone_rule(const struct test_rule *);
+
/* Trivial (linear) classifier. */
struct tcls {
size_t n_rules;
@@ -138,8 +143,8 @@ tcls_insert(struct tcls *tcls, const struct test_rule *rule)
const struct cls_rule *pos = &tcls->rules[i]->cls_rule;
if (cls_rule_equal(pos, &rule->cls_rule)) {
/* Exact match. */
- free(tcls->rules[i]);
- tcls->rules[i] = xmemdup(rule, sizeof *rule);
+ free_rule(tcls->rules[i]);
+ tcls->rules[i] = clone_rule(rule);
return tcls->rules[i];
} else if (pos->priority < rule->cls_rule.priority) {
break;
@@ -154,7 +159,7 @@ tcls_insert(struct tcls *tcls, const struct test_rule *rule)
memmove(&tcls->rules[i + 1], &tcls->rules[i],
sizeof *tcls->rules * (tcls->n_rules - i));
}
- tcls->rules[i] = xmemdup(rule, sizeof *rule);
+ tcls->rules[i] = clone_rule(rule);
tcls->n_rules++;
return tcls->rules[i];
}
@@ -422,7 +427,7 @@ destroy_classifier(struct classifier *cls)
cls_cursor_init(&cursor, cls, NULL);
CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
classifier_remove(cls, &rule->cls_rule);
- free(rule);
+ free_rule(rule);
}
classifier_destroy(cls);
}
@@ -522,6 +527,24 @@ make_rule(int wc_fields, unsigned int priority, int value_pat)
return rule;
}
+static struct test_rule *
+clone_rule(const struct test_rule *src)
+{
+ struct test_rule *dst;
+
+ dst = xmalloc(sizeof *dst);
+ dst->aux = src->aux;
+ cls_rule_clone(&dst->cls_rule, &src->cls_rule);
+ return dst;
+}
+
+static void
+free_rule(struct test_rule *rule)
+{
+ cls_rule_destroy(&rule->cls_rule);
+ free(rule);
+}
+
static void
shuffle(unsigned int *p, size_t n)
{
@@ -584,7 +607,7 @@ test_single_rule(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
assert(tcls_is_empty(&tcls));
compare_classifiers(&cls, &tcls);
- free(rule);
+ free_rule(rule);
classifier_destroy(&cls);
tcls_destroy(&tcls);
}
@@ -619,7 +642,7 @@ test_rule_replacement(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
tcls_insert(&tcls, rule2);
assert(test_rule_from_cls_rule(
classifier_replace(&cls, &rule2->cls_rule)) == rule1);
- free(rule1);
+ free_rule(rule1);
check_tables(&cls, 1, 1, 0);
compare_classifiers(&cls, &tcls);
tcls_destroy(&tcls);
@@ -760,7 +783,7 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
tcls_destroy(&tcls);
for (i = 0; i < N_RULES; i++) {
- free(rules[i]);
+ free_rule(rules[i]);
}
} while (next_permutation(ops, ARRAY_SIZE(ops)));
assert(n_permutations == (factorial(N_RULES * 2) >> N_RULES));
@@ -838,7 +861,7 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
for (i = 0; i < N_RULES; i++) {
tcls_remove(&tcls, tcls_rules[i]);
classifier_remove(&cls, &rules[i]->cls_rule);
- free(rules[i]);
+ free_rule(rules[i]);
check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0);
compare_classifiers(&cls, &tcls);
@@ -897,18 +920,17 @@ test_many_rules_in_n_tables(int n_tables)
struct test_rule *target;
struct cls_cursor cursor;
- target = xmemdup(tcls.rules[rand() % tcls.n_rules],
- sizeof(struct test_rule));
+ target = clone_rule(tcls.rules[rand() % tcls.n_rules]);
cls_cursor_init(&cursor, &cls, &target->cls_rule);
CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
classifier_remove(&cls, &rule->cls_rule);
- free(rule);
+ free_rule(rule);
}
tcls_delete_matches(&tcls, &target->cls_rule);
compare_classifiers(&cls, &tcls);
check_tables(&cls, -1, -1, -1);
- free(target);
+ free_rule(target);
}
destroy_classifier(&cls);
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 7295cef..93cd7ac 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1778,6 +1778,7 @@ fte_free(struct fte *fte)
if (fte) {
fte_version_free(fte->versions[0]);
fte_version_free(fte->versions[1]);
+ cls_rule_destroy(&fte->rule);
free(fte);
}
}
@@ -1816,6 +1817,7 @@ fte_insert(struct classifier *cls, const struct match *match,
if (old) {
fte_version_free(old->versions[index]);
fte->versions[!index] = old->versions[!index];
+ cls_rule_destroy(&old->rule);
free(old);
}
}
--
1.7.2.5
More information about the dev
mailing list