Some of this would be a bit simpler if classifier_find_rule_exactly()
took a match and a priority instead of a cls_rule. May make sense to
insert that change into a new patch preceding this one.
Looks good, thanks.
Ethan
On Fri, Jul 20, 2012 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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 <b...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
